Repository navigation
Move check_rustc_must_implement_one_of validation to attribute parser - #163980
darkraider01 wants to merge 2 commits into
Conversation
rustbot
added
A-attributes
2828fd1 to
5792ac6
Compare
darkraider01
changed the title
5792ac6 to
ab04b05
Compare
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_attr_parsing |
rustbot
added
S-waiting-on-review
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use Why was this reviewer chosen?The reviewer was selected based on:
|
rustbot
added
S-waiting-on-author
| match ctxt { | ||
| visit::AssocCtxt::Trait => { | ||
| self.replace_with_dummy(item, AssocItemKind::MacCall, AstOwner::TraitItem) | ||
| self.insert(item.id, AstOwner::TraitItem(Box::new(item.clone()))); |
There was a problem hiding this comment.
The old replace_with_dummy was replacing the trait items with MacCall placeholders, so finalize_check couldn't actually inspect them through target_item.
This keeps the original items in the trait while still indexing the cloned ones separately for lowering.
The downside is that we're cloning trait items even when the attribute isn't used. I couldn't find a simpler way to reuse the existing infrastructure here, but I'm open to suggestions.
There was a problem hiding this comment.
Edit: (This approach is suggested by llm, since i couldn't come with a better/simplied solution) Maybe another option could be to keep the original trait items only for traits with #[rustc_must_implement_one_of] , and use the existing placeholders everywhere else. That would still involve cloning, but limit it to traits that actually need the check. Not sure how neatly it would fit into the indexer, especially with nested traits, but it might be worth exploring.
| _ => { | ||
| cx.emit_err(diagnostics::MustImplementNotFunction { | ||
| span: item.span, | ||
| span_note: diagnostics::MustImplementNotFunctionSpanNote { |
There was a problem hiding this comment.
Keeping delegation as a valid case here since it lowers to TraitFn::Provided, which the old check already accepted.
I added tests for both normal and renamed delegation as well.
| panic!("expected trait AST target item for Target::Trait"); | ||
| }; | ||
|
|
||
| let fn_names = |
There was a problem hiding this comment.
Kept the full Ident comparison here rather than just matching names, since the old check did the same.
Added a couple of macro tests to make sure the hygiene behavior stays the same.
rustbot
added
S-waiting-on-review
This comment was marked as outdated.
This comment was marked as outdated.
|
r? me |
|
☔ The latest upstream changes (presumably #164230) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
This branch has not been deployed
5 participants
Part of #153101 and #131229.
Moves the remaining
rustc_must_implement_one_ofvalidation fromrustc_passes::check_attrintoRustcMustImplementOneOfParser::finalize_checkusing AST trait information available throughFinalizeCheckContext::target_item.The duplicate-name validation was already migrated in #160412. This completes the remaining associated-item and default-implementation checks.
Related: #153101, #131229, #160412