-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: Improve proc macro errors a bit #12629
fix: Improve proc macro errors a bit #12629
Conversation
Distinguish between - there is no build data (for some reason?) - there is build data, but the cargo package didn't build a proc macro dylib - there is a proc macro dylib, but it didn't contain the proc macro we expected - the name did not resolve to any macro (this is now an unresolved_macro_call even for attributes) I changed the handling of disabled attribute macro expansion to immediately ignore the macro and report an unresolved_proc_macro, because otherwise they would now result in loud unresolved_macro_call errors. I hope this doesn't break anything. Also try to improve error ranges for unresolved_macro_call / macro_error by reusing the code for unresolved_proc_macro. It's not perfect but probably better than before.
if !self.db.enable_proc_attr_macros() { | ||
self.def_map.diagnostics.push(DefDiagnostic::unresolved_proc_macro( | ||
directive.module_id, | ||
loc.kind, | ||
Some(loc.def.krate), | ||
)); | ||
return recollect_without(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 changed the handling of disabled attribute macro expansion to
immediately ignore the macro and report an unresolved_proc_macro,
because otherwise they would now result in loud unresolved_macro_call
errors. I hope this doesn't break anything.
I'm not sure if this breaks anything or not, there is the comment on reseeed_with_unresolved_attribute
:
/// When the fixed-point loop reaches a stable state, we might still have some unresolved
/// attributes (or unexpanded attribute proc macros) left over. This takes one of them, and
/// feeds the item it's applied to back into name resolution.
///
/// This effectively ignores the fact that the macro is there and just treats the items as
/// normal code.
///
/// This improves UX when proc macros are turned off or don't work, and replicates the behavior
/// before we supported proc. attribute macros.
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.
Ah yeah, I should at least update that comment. IMO it shouldn't change anything; if attribute expansion is disabled, known proc macro attributes will just get 'reseeded' immediately instead of after going through everything else.
We already do the same when proc macros are enabled, but the proc macro expander can't be found (with the is_dummy
check further down).
@bors r+ |
📌 Commit 45fd5e6 has been approved by |
☀️ Test successful - checks-actions |
Distinguish between
unresolved_macro_call even for attributes)
I changed the handling of disabled attribute macro expansion to
immediately ignore the macro and report an unresolved_proc_macro,
because otherwise they would now result in loud unresolved_macro_call
errors. I hope this doesn't break anything.
Also try to improve error ranges for unresolved_macro_call / macro_error
by reusing the code for unresolved_proc_macro. It's not perfect but
probably better than before.