-
Notifications
You must be signed in to change notification settings - Fork 382
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
Emit error for assertMacroExpansion
when applying an attached member macro to declaration that can’t have members
#2382
base: main
Are you sure you want to change the base?
Conversation
assertMacroExpansion
when applying an attached member macro to declaration that can’t have members
assertMacroExpansion
when applying an attached member macro to declaration that can’t have membersassertMacroExpansion( | ||
""" | ||
@Test var x: Int | ||
""", | ||
expandedSource: """ | ||
var x: Int | ||
""", | ||
macros: [ | ||
"Test": TestMacro.self | ||
] | ||
) |
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.
I think you have it the wrong way round. From the issue description
the following test case should emit an error.
Also: Did you run all the other tests? I suspect all member tests will fail with your current changes.
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.
I think you have it the wrong way round. From the issue description
You mean the test itself is wrong or it's in the wrong place?
Also: Did you run all the other tests? I suspect all member tests will fail with your current changes.
Well yes, as I said other member tests are failing.
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.
You mean the test itself is wrong or it's in the wrong place?
Yes, having an attached member macro on a variable should emit an error.
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.
Ok so I had mistakenly put a guard
instead of if
, I am yet to find the trigger
for VariableDeclSyntax
so I can see the test failing and then add the error message, but the previous errors and tests are good which weren't supposed to fail.
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.
What diagnostic
do you recommend adding here? Is there an existing one for such case or we need a custom addition to the enum
which we then throw
during detection?
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.
I would suggest that you add a new diagnostic with any wording you like and then we can iterate from there.
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.
Which property should be checked under MacroExpansion
in the below case (directed here from MacroSystem
-> expandAttachedMacro
-> expandAttachedMacroWithoutCollapsing
) if I am under the right place:
case (let attachedMacro as MemberMacro.Type, .member):
// ...
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.
In the issue I wrote
specifically, if the declaration is not a
DeclGroupSyntax
So that’s the syntax node type I would check.
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.
@ahoppen
According to the issue, as you mentioned:
specifically, if the declaration is not a DeclGroupSyntax
But, the following check exists:
guard let declGroup = declarationNode.asProtocol(DeclGroupSyntax.self)
How does it not throw an error here if the node isn't a DeclGroupSyntax
?
f8a8212
to
4f2eff0
Compare
…r macro to declaration that can’t have members Resolves apple#2206
4f2eff0
to
d919134
Compare
assertMacroExpansion
when applying an attached member macro to declaration that can’t have members
Resolves Issue #2206
When we apply an attached
member macro
to a declaration that cannot have members,assertMacroExpansion
fails to emit appropriatediagnostic
message.