feat: improve support for ignored proc macros#15923
Conversation
rustbot
added
the
S-waiting-on-review
| Ok(subtree.clone()) | ||
| } | ||
|
|
||
| fn should_expand(&self) -> bool { |
There was a problem hiding this comment.
We can just combine IdentityExpander and EmptyExpander now and have the expand function return an error since they are properly ignored now, so they should enver be expanded anyways.
There was a problem hiding this comment.
so they should never be expanded anyways
Is this certain? I focused on attribute macros and a specific use-case (ignored macros) in this PR, so I don't know where else these expanders might be used. If empty or identity expansions aren't needed anywhere, I can merge them into a NoopExpander or something that will always return an error (or alternaitively identity/empty tree upon agreement).
There was a problem hiding this comment.
Perhaps we could keep the IdentityExpander for unresolved macros and other unhappy paths only, and then keep regular macros as-is but also augment ignored ones so that should_expand returns false, this way we can still have the ability to expand even ignored macros on demand (e.g. via a command).
There was a problem hiding this comment.
Is this certain?
Well disabled means disabled, if we still call this somewhere that's more of a bug imo
| .and_then(|crate_name| { | ||
| dummy_replacements.get(crate_name).map(|v| &**v) | ||
| ignored_proc_macros.iter().find_map(|c| { | ||
| if eq_ignore_underscore(&*c.0, crate_name) { |
There was a problem hiding this comment.
👍 we should've done this from the start tbh
Veykril
added
S-waiting-on-author
Veykril
force-pushed
the
feat/better-ignored-macros2
branch
from
62348be to
d285764
Compare
|
Went ahead and rebased this on current master, I might look into bringing this over the finish line later. |
|
☔ The latest upstream changes (presumably #16404) made this pull request unmergeable. Please resolve the merge conflicts. |
Veykril
force-pushed
the
feat/better-ignored-macros2
branch
from
d285764 to
e2a985e
Compare
|
Alright I came back to this, fixed some stuff up and should be ready to merge now. |
|
☀️ Test successful - checks-actions |
lnicola
changed the title
4 participants
Supersedes #15117, I was having some conflicts after a rebase and since I didn't remember much of it I started clean instead.
The end result is pretty much the same as the linked PR, but instead of proc macro lookups, I marked the expanders that explicitly cannot be expanded and we shouldn't even attempt to do so.
Unresolved questions
DISABLED_IDnext toDUMMY_IDinhir-expand'sProcMacroExpander, that is effectively exactly the same thing with slightly different semantics, dummy macros are not (yet) expanded probably due to errors, while not expanding disabled macros is part of the usual flow. I'm not sure if it's the right way to handle this, I also thought of just adding a flag instead of replacing the macro ID, so that the disabled macro can still be expanded for any reason if needed.