Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support use-site meta-annotations #16445

Merged
merged 16 commits into from
Dec 14, 2022
Merged

Support use-site meta-annotations #16445

merged 16 commits into from
Dec 14, 2022

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Nov 30, 2022

Fixes #12492
Fixes #15318

@dwijnand
Copy link
Member Author

Doesn't have a test for #15318 actually, and looking at it it uses SymDenotation#getAnnotation so it's not going to find the use-site defined annotations. That needs doing.

Co-Authored-By: Guillaume Martres <[email protected]>
@dwijnand
Copy link
Member Author

Next to fix the symbol positions of the copied annotations under -Ytest-pickler.........

@dwijnand dwijnand marked this pull request as ready for review December 1, 2022 22:05
@dwijnand dwijnand requested a review from sjrd December 1, 2022 22:05
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a removeUnwantedAnnotations in Memoize.scala as well. Should it be affected?

tests/run/i15318.check Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/Annotations.scala Outdated Show resolved Hide resolved
@sjrd sjrd assigned dwijnand and unassigned sjrd Dec 2, 2022
@dwijnand dwijnand assigned lrytz and unassigned dwijnand Dec 12, 2022
@dwijnand dwijnand requested a review from lrytz December 12, 2022 17:12
@dwijnand dwijnand assigned smarter and unassigned lrytz Dec 13, 2022
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise

!annotSym.annotations.exists(metaAnnot => defn.FieldAccessorMetaAnnots.contains(metaAnnot.symbol))
})
annot.hasOneOfMetaAnnotation(metaAnnotSym, metaAnnotSymBackup)
|| keepIfNoRelevantAnnot && !annot.hasOneOfMetaAnnotation(defn.NonBeanMetaAnnots.toList*)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why exclude "bean" meta annotations here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runs before bean getters and setters are created. So if we lose the @beanGetter/@beanSetter-annotated annotations on the regular class vals, e.g.:

@(JsonProperty @beanGetter) @BeanProperty val beanGet: String = ""

Then they'll never be copied over onto the def getBeanGet: String = ... we're about to syntheses.

Copy link
Member Author

@dwijnand dwijnand Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Memoize, where we split vals into fields and re-written getters/setters, we now also drop annotations that are beanGetter/Setter annotated, from the field, and keep only @getter/@setter-annotated annotations on the (non-bean) getters/setters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks

@lrytz lrytz merged commit d3070f6 into scala:main Dec 14, 2022
@dwijnand dwijnand deleted the meta-annotations branch December 14, 2022 09:15
magro added a commit to inoio/solrs that referenced this pull request Jan 8, 2023
Workaround issue with annotations covered by
scala/scala3#12492, fixed with
scala/scala3#16445

This commit can be reverted as soon as the fix is released (according to
https://github.com/lampepfl/dotty/releases this is not yet the case)
magro added a commit to inoio/solrs that referenced this pull request Jan 8, 2023
Workaround issue with annotations covered by
scala/scala3#12492, fixed with
scala/scala3#16445

This commit can be reverted as soon as the fix is released (according to
https://github.com/lampepfl/dotty/releases this is not yet the case)
magro added a commit to inoio/solrs that referenced this pull request Jan 8, 2023
Workaround issue with annotations covered by
scala/scala3#12492, fixed with
scala/scala3#16445

This commit can be reverted as soon as the fix is released (according to
https://github.com/lampepfl/dotty/releases this is not yet the case)
@Kordyjan Kordyjan added this to the 3.3.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants