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

Do not propagate @tailrec to exported methods #19509

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

nicolasstucki
Copy link
Contributor

Fixes #19505

forwarder.addAnnotations(sym.annotations.filterConserve(_.symbol != defn.BodyAnnot))
forwarder.addAnnotations(sym.annotations.filterConserve { annot =>
annot.symbol != defn.BodyAnnot
&& annot.symbol != defn.TailrecAnnot
Copy link
Member

@bishabosha bishabosha Jan 22, 2024

Choose a reason for hiding this comment

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

alternatively, could the TailRec check ignore exported methods? - this filtering approach might not be scalable for e.g. macro annotations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, we also need to filter macro annotations.

I think that disabling the behavior where it is checked will be less scalable. For example, I disable the macro annotations on exports I have to find one or more places where this restriction needs to be taken into account. And those places are usually code generation details like exports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also added @main. Might might still need to filter others.

Copy link
Contributor Author

@nicolasstucki nicolasstucki Jan 23, 2024

Choose a reason for hiding this comment

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

I wonder if we should have a whitelist approach with this one. This would make sure that we never accidentally keep an annotation we do not want.

I tested this on scala3-bootstrapped/testCompilation and it only required the @targetName annotation. Others that should be required are @experimental, @publicInBinary, and showAsInfix.

Copy link
Member

Choose a reason for hiding this comment

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

The thing is that we will never know what should be the behavior for custom annotations.

The only holistic solution here is to have an @meta-based indication of what to do. This is the approach used to determine whether annotations should go to fields/getters/setters. It would also be appropriate to determine whether it should go to exports/bridges/etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need ExportableAnnotation trait that all such annotations should extend to get exported.

@bishabosha bishabosha merged commit fff24b1 into scala:main Jan 23, 2024
19 checks passed
@bishabosha bishabosha deleted the fix-19505 branch January 23, 2024 16:49
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
WojciechMazur added a commit that referenced this pull request Jun 28, 2024
…0851)

Backports #19509 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot export @tailrec method
5 participants