Skip to content

Parser: Recover from attributes applied to types and generic args #144195

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

Merged
merged 1 commit into from
Aug 7, 2025

Conversation

Kivooeo
Copy link
Contributor

@Kivooeo Kivooeo commented Jul 19, 2025

r? compiler

Add clearer error messages for invalid attribute usage in types or generic types

fixes #135017
fixes #144132

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 19, 2025
@Kivooeo Kivooeo changed the title Added checks for attributes in types Add checks for attributes in types Jul 19, 2025
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2025
Comment on lines 382 to 384
while self.token != token::CloseBracket && self.token != token::Eof {
self.bump();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is pretty strange.

Why wouldn't we just go through the normal parsing routine to parse an attr, then go through the normal parsing routine to parse a type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, honestly didn't know about "normal parsing routine" and just go manually parsing, do you mean "parse_outer_attributes"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah probably something like that. Look at how we parse item attrs for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if I'm on a right way, firstly implemented it for generic then just copied for regular type, based on tests it seems works fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it now, maybe I could yeet that logic into a helper function and not duplicate code, but first need your opinion on logic itself, but I'm out for now


let span = lo_attr.to(self.prev_token.span);

if self.token.is_ident() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically, this only works if the type is an ident. We have lots of other types to handle here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just try parsing an attr eagerly at the beginning of parse_ty_common, and then there doesn't need to be a special branch in this if chain?

@compiler-errors
Copy link
Member

r? compiler-errors

@rust-log-analyzer

This comment has been minimized.

@Kivooeo Kivooeo force-pushed the bad-attr branch 2 times, most recently from 5502794 to 2b4000d Compare July 20, 2025 20:12
@Kivooeo
Copy link
Contributor Author

Kivooeo commented Jul 21, 2025

I'm genuinely curious about case where we have attribute in generic but don't have argument, what would ideal error in such case?

I'm not sure if we want to recover after finding a attribute, becuase it's always a wrong code and it's lead to case like this

Well I added a branch to handle this (you can see it in review below)

error[E0107]: struct takes 1 generic argument but 0 generic arguments were supplied
 --> src/main.rs:4:12
  |
4 |     let _: Baz<#[cfg(any())]> = todo!();
  |            ^^^ expected 1 generic argument
  |
note: struct defined here, with 1 generic parameter: `N`
 --> src/main.rs:1:8
  |
1 | struct Baz<const N: usize>(i32);
  |        ^^^ --------------
help: add missing generic argument
  |
4 |     let _: Baz<N#[cfg(any())]> = todo!();
  |                +

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0107`.

@Kivooeo Kivooeo force-pushed the bad-attr branch 2 times, most recently from e267a23 to 516db8d Compare July 21, 2025 14:47
Copy link
Contributor Author

@Kivooeo Kivooeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've called out the potentially surprising bits of this implementation with explanations to help reviewers in mine this and above reviews

@rust-log-analyzer

This comment has been minimized.

@Kivooeo
Copy link
Contributor Author

Kivooeo commented Jul 29, 2025

@fmease, I spent significant time trying to address the secondary error you mentioned. Good news: I solved it for cases where there's something inside the generic. When we have Baz<#[cfg(any())] 42>, we can now match on what we parsed and return the right GenericArg type. This eliminates the secondary error problem.

But I hit a wall with empty generics like Baz<#[cfg(any())]>.

The problem is ty_generics is always None during parsing. The parser hasn't done name resolution yet as far I understand that process correctly, so it doesn't know what Baz actually expects. When we see an empty generic with just an attribute, we have to guess whether to return type, const or lifetime.

Whatever we pick will be wrong sometimes. If I default to GenericArg::Type, then Baz<#[attr]> gives "type provided when constant expected." If I default to GenericArg::Const, then Foo<#[attr]> gives "constant provided when type expected."

I can pick a statistical default (types are most common), but some cases will still get secondary errors. Is that acceptable, or do you see another approach I'm missing?

The architectural issue seems fundamental since parsing happens before we know what types expect what kinds of generics.

@Kivooeo
Copy link
Contributor Author

Kivooeo commented Aug 1, 2025

@rustbot ready

@fmease fmease self-assigned this Aug 4, 2025
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final stretch, after that I'll approve.

@@ -934,6 +941,9 @@ impl<'a> Parser<'a> {
}
} else if self.token.is_keyword(kw::Const) {
return self.recover_const_param_declaration(ty_generics);
} else if let Some(attr_span) = attr_span {
let diag = self.dcx().create_err(AttributeOnEmptyType { span: attr_span });
return Err(diag);
} else {
// Fall back by trying to parse a const-expr expression. If we successfully do so,
// then we should report an error that it needs to be wrapped in braces.
Copy link
Member

@fmease fmease Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two lines below, there's a preexisting let attrs = self.parse_outer_attributes()?;. We should be able to drop it and pass the already parsed attrs to the parse_expr_res.

It's not a blocking concern tho ig and it would stop us from recovering from /// and /** */ on gen args. Can be done in a follow-up.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 5, 2025

⚠️ Warning ⚠️

@Kivooeo
Copy link
Contributor Author

Kivooeo commented Aug 5, 2025

Small breakdown what I've changed according to last reviews

  • Removed reduant comments
  • Renamed AttributeOnGenericType to AttributeOnGenericArg (and parse_attribute_on_generic_type to parse_attribute_on_generic_arg)
  • Deleted (my favourite function so far) attrs()
  • Use take_for_recovery instead of attrs
  • Fixed .fixed in some test with really long name
  • Exaplained on why we need this check on # and [

What I did not made

@Kivooeo
Copy link
Contributor Author

Kivooeo commented Aug 5, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 5, 2025
@fmease
Copy link
Member

fmease commented Aug 6, 2025

Thanks a lot!
@bors r=fmease,compiler-errors rollup

@bors
Copy link
Collaborator

bors commented Aug 6, 2025

📌 Commit d09cf61 has been approved by fmease,compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2025
@fmease fmease changed the title Add checks for attributes in types Parser: Recover from attributes applied to types and generic args Aug 6, 2025
bors added a commit that referenced this pull request Aug 6, 2025
Rollup of 15 pull requests

Successful merges:

 - #144195 (Parser: Recover from attributes applied to types and generic args)
 - #144794 (Port `#[coroutine]` to the new attribute system)
 - #144835 (Anonymize binders in tail call sig)
 - #144861 (Stabilize `panic_payload_as_str` feature)
 - #144917 (Enforce tail call type is related to body return type in borrowck)
 - #144948 (we only merge candidates for trait and normalizes-to goals)
 - #144956 (Gate const trait syntax)
 - #144970 (rustdoc: fix caching of intra-doc links on reexports)
 - #144972 (add code example showing that file_prefix treats dotfiles as the name of a file, not an extension)
 - #144975 (`File::set_times`: Update documentation and example to support setting timestamps on directories)
 - #144977 (Fortify generic param default checks)
 - #144996 (simplifycfg: Mark as changed when start is modified in collapse goto chain)
 - #144998 (mir: Do not modify NonUse in `super_projection_elem`)
 - #145000 (Remove unneeded `stage` parameter when setting up stdlib Cargo)
 - #145008 (Fix rustdoc scrape examples crash)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d180873 into rust-lang:master Aug 7, 2025
10 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Aug 7, 2025
rust-timer added a commit that referenced this pull request Aug 7, 2025
Rollup merge of #144195 - Kivooeo:bad-attr, r=fmease,compiler-errors

Parser: Recover from attributes applied to types and generic args

r? compiler

Add clearer error messages for invalid attribute usage in types or generic types

fixes #135017
fixes #144132
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing diagnostic for attributes on types Placing an attribute on a generic argument confuses the parser
9 participants