-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Set Span for top level annotations generated in PostTyper #16378
Conversation
This should help avoid macros throwing assertion errors in the case of annotations not having assigned Spans/positions. Since the annotation is generated and does not have a position corresponding to the source file, it inherits Span from the tree to which it is assigned.
cc74d95
to
db5b398
Compare
Hi @nicolasstucki apologies for being so late with this. I was not entirely sure on what you meant in the above comment, so in the end I decided to add two commits. The first one fixes the issue by adding spans to all generated annotations in PostTyper, like before. This alone fixes the #16318, and adds annotations to two additional places where the issue has a chance to reappear (so this includes every annotation generated in PostTyper). In the second commit, I make sure to add the span to every generated annotation in the compiler. For that I adjusted Annotation.apply methods to always require a span argument. This was obviously helpful for me, as it allowed me to easily locate span-less Annotations, and hopefully will be for others later, even if it is not futureproof (one can still pass a NoSpan, if necessary, in general it’s meant to be more of a remainder). However, if the contents of the second commit end up being unnecessary, I can easily remove it from the PR and we can proceed with just the first commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
compiler/test/dotty/tools/dotc/transform/TypeTestsCastsTest.scala
Outdated
Show resolved
Hide resolved
The contents of the second commit are necessary. That is what I felt was missing before. |
Achieved by removing the apply methods without a span argument, which would generate Annotation trees with empty spans. One can still put NoSpan as the span argument, so this will not make much of a difference, but will hopefully provide an additional incentive to always add one.
Thank you for the review! I also just realized I @ the wrong person in my comment above before, for which I apologize, should be edited correctly now |
This should help avoid macros throwing assertion errors in the case of annotations not having assigned Spans/positions. Since the annotation is generated and does not have a position corresponding to the source file, it inherits Span from the tree to which it is assigned.
Fixes #16318