-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Do not Ycheck inlined positions for macro annotation classes #18550
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
Conversation
@@ -38,6 +38,8 @@ class YCheckPositions extends Phase { | |||
// Recursivlely check children while keeping track of current source | |||
reporting.trace(i"check pos ${tree.getClass} ${tree.source} ${sources.head} $tree") { | |||
tree match { | |||
case tree: TypeDef if tree.isClassDef && MacroAnnotations.hasMacroAnnotation(tree.symbol) => |
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.
We should probably do this for any DefTree
that has a macro annotation.
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.
A test case for this could be
class Test:
@annotation def method = inlineMethod(42)
where the annotation modified the def
in the same way.
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.
Good point, I added this behavior for both DefDef
s and ValDef
s, and added both to the test
In macro annotation definitions, as part of the macro expansion, users can mess with `Inlined` nodes, which can break YCheckPositions checks. Similarly to how it is done for macro methods, we now skip checking definitions with a macro annotation.
We must make sure that users create correct trees when there are inlined positions involved. We had to disable this in macros only because we got the checks too late. Now it is harder to reenable the tests, but we must do it sooner rather than later. We should not disable these checks on macro annotations to force users to write the correct code when writing new macro annotations. |
What we need to do is improve the error reporting to make it easier for users to know where they went wrong. Maybe we can improve |
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.
See previous comments
Needs a different approach |
In macro annotation classes, as part of the macro expansion, users can mess with
Inlined
nodes, which can break YCheckPositions checks. Similarly to how it is done for macro methods, we now skip checking classes with a macro annotation.Fixes #17007
In the issue specifically, the problem was that the "outer"
Inlined
node was removed, which YCheckPositions expected and aimed to retrieve the original source file from it, so when it reached the unaffected "inner"Inlined
node the compiler would crash.