Add checks for attributes in types #144195

Kivooeo · 2025-07-19T22:02:56Z

r? compiler

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

compiler-errors · 2025-07-20T16:58:04Z

compiler/rustc_parse/src/parser/ty.rs

+            while self.token != token::CloseBracket && self.token != token::Eof {
+                self.bump();
+            }


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?

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

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

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

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

compiler-errors · 2025-07-20T16:58:21Z

compiler/rustc_parse/src/parser/ty.rs

+
+            let span = lo_attr.to(self.prev_token.span);
+
+            if self.token.is_ident() {


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

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 · 2025-07-20T17:00:23Z

r? compiler-errors

compiler/rustc_parse/src/parser/path.rs

Kivooeo · 2025-07-21T12:45:09Z

~~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`.

compiler/rustc_parse/src/parser/path.rs

tests/ui/parser/attribute-on-type.stderr

tests/ui/parser/attribute-on-type.rs

compiler/rustc_parse/src/parser/attr_wrapper.rs

Kivooeo

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

tests/ui/parser/attribute-on-type.stderr

tests/ui/parser/recover/recover-where-clause-before-tuple-struct-body-0.fixed

tests/ui/parser/attribute-on-type.rs

Kivooeo · 2025-07-29T20:48:27Z

@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.

fmease · 2025-07-30T03:57:01Z

Thanks for making an effort to address my comments, that's highly appreciated!

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?

Instead of returning a poisoned type (so, an Ok(…TyKind::Err…)) simply don't recover and bail out with a normal parse error Err(…). For details, see this review comment I just added.

compiler/rustc_parse/src/parser/path.rs

compiler/rustc_parse/messages.ftl

fmease · 2025-07-30T03:48:08Z

compiler/rustc_parse/src/parser/path.rs

+            let guar = self.dcx().emit_err(AttributeOnEmptyGeneric { span: attr_span });
+            return Ok(Some(GenericArg::Type(self.mk_ty(attr_span, TyKind::Err(guar)))));


Instead of returning a poisoned type (which is undesirable as discussed), simply bail out.

Suggested change

let guar = self.dcx().emit_err(AttributeOnEmptyGeneric { span: attr_span });

return Ok(Some(GenericArg::Type(self.mk_ty(attr_span, TyKind::Err(guar)))));

let diag = self.dcx().create_err(AttributeOnEmptyGeneric { span: attr_span });

return Err(diag);

This will probably lead to the parser stopping entirely after that which is slightly sad but still absolutely acceptable. By its very nature, it flat out prevents nonsensical consequential errors like arg kind mismatches.

I think you tried the non-fatal return Ok(None) before (I might be misremembering) which led to "nonsensical" arg count mismatches, right? I don't think they're that nonsensical, after all the "slot" is basically empty. So I'd say return Ok(None) would be a viable alternative to return Err(…) (unless I'm missing something) if you prefer recovery over "surrender".

There is a problem with both approaches

return Err(diag) stops parsing after the first error, which is slightly overkill because making 5 tests for each case is fundamental wrong

(first error was emitted correctly and now following ones below is not found because parser died :c) tests/ui/parser/attribute-on-empty.rs:12:5: ERROR: attributes cannot be applied here tests/ui/parser/attribute-on-empty.rs:15:5: ERROR: attributes cannot be applied here tests/ui/parser/attribute-on-empty.rs:18:5: ERROR: attributes cannot be applied here

And it can't pass current test

return Ok(None) is also wrong (which I've tried but worth to mention), it gives suggestion that is makes no sense

error[E0107]: struct takes 1 generic argument but 0 generic arguments were supplied --> $DIR/attribute-on-empty.rs:12:12 | LL | let _: Foo<#[cfg(not(wrong))]> = todo!(); | ^^^ expected 1 generic argument | note: struct defined here, with 1 generic parameter: `T` --> $DIR/attribute-on-empty.rs:4:8 | LL | struct Foo<T>(T); | ^^^ - help: add missing generic argument | LL | let _: Foo<T#[cfg(not(wrong))]> = todo!(); | +

return Err(diag) stops parsing after the first error, which is slightly overkill because making 5 tests for each case is fundamental wrong

return Err(diag); is the least hacky and the simplest solution. We can always iterate from here via follow-up PRs. Three test cases are basically identical:

let _: Baz<#[cfg(any())]> = todo!();,

let _: Foo<#[cfg(not(wrong))]> = todo!(); and

let _: Bar<#[cfg(any())]> = todo!();.

They should just be combined into one. Lastly, you can move the resulting unified "empty gen arg" case to the end of the file, so rustc still emits the other errors before it bails out. No need for more test files.

To explain myself a bit more, the "empty gen args" case is a lot less likely to happen in practice compared to all the other cases where there's an arg present, so in my eyes it's absolutely justifiable that its logic is more conservative. We can rejoice, free of any headaches, since we know there won't be any invalid or weird consequential diagnostics and since the impl is simple.

So completly kill parser?

Yes, indeed. That's exactly what happens for unexpected tokens, so it's completely normal. Parse error recovery is cool and all but if it's infeasible to do robustly it just doesn't make sense to force it. As I said, it can always be improved upon in the future (tweaking spans, intro'ing GenericArg::Error, …).

Sure, then I just keep this Err(diag) and also check if keeping returning poisoned arg is fine there

If that's what we are fine with, then I'm pushing this last changes from @estebank and you and that's it I guess

fmease · 2025-07-30T03:55:20Z

compiler/rustc_parse/src/parser/path.rs

+            return Ok(Some(match arg {
+                GenericArg::Type(_) => GenericArg::Type(self.mk_ty(attr_span, TyKind::Err(guar))),
+                GenericArg::Const(_) => {
+                    // Create error const expression
+                    let error_expr = self.mk_expr(attr_span, ExprKind::Err(guar));
+                    GenericArg::Const(AnonConst { id: ast::DUMMY_NODE_ID, value: error_expr })
+                }
+                GenericArg::Lifetime(lt) => GenericArg::Lifetime(lt), // Less common case
+            }));


I don't think we need to taint or poison the arg just because it has attrs. Emitting an error is already sufficient for guaranteeing a failed compilation. This whole return could be removed.

Surprisingly it is worked

Not fully as wanted, check this #144195 (comment)

compiler/rustc_parse/src/errors.rs

Kivooeo · 2025-07-30T13:20:11Z

tests/ui/parser/issues/issue-103143.rs

Removing a return with poisoned type now says this,

--- reported in JSON output but not expected in test file --- tests/ui/parser/issues/issue-103143.rs:2:13: ERROR: cannot find type `y` in this scope [E0412] tests/ui/parser/issues/issue-103143.rs:2:17: ERROR: cannot find type `z` in this scope [E0412] ---

Instead of just cannot find value x in this scope which small but a regression I'd say

It's more verbose for sure but personally I don't mind it. After all, these new diagnostics are correct. On the other hand, it's conceivable that #[a] is meant to refer to an attribute proc macro that replaces the path with sth. that does resolve (but you typoed or forgot to import), so these extra diagnostics would be noise. I'm neutral 🤷

If returning this dummy types is not considered as a bad practice I'd keep it just for consistency

compiler/rustc_parse/src/errors.rs

Kivooeo · 2025-08-01T12:11:00Z

@rustbot ready

rustbot assigned lcnr Jul 19, 2025

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 changed the title ~~Added checks for attributes in types~~ Add checks for attributes in types Jul 19, 2025

workingjubilee closed this Jul 20, 2025

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2025

workingjubilee reopened this Jul 20, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2025

compiler-errors requested changes Jul 20, 2025

View reviewed changes

rustbot assigned compiler-errors and unassigned lcnr Jul 20, 2025

Kivooeo force-pushed the bad-attr branch from 1119298 to 5d9c713 Compare July 20, 2025 18:34

This comment has been minimized.

Sign in to view

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

fmease reviewed Jul 20, 2025

View reviewed changes

compiler/rustc_parse/src/parser/path.rs Outdated Show resolved Hide resolved

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

Kivooeo commented Jul 21, 2025

View reviewed changes

Kivooeo force-pushed the bad-attr branch from 516db8d to b107f87 Compare July 21, 2025 16:54

Kivooeo commented Jul 21, 2025

View reviewed changes

tests/ui/parser/attribute-on-type.stderr Outdated Show resolved Hide resolved

estebank reviewed Jul 23, 2025

View reviewed changes

tests/ui/parser/recover/recover-where-clause-before-tuple-struct-body-0.fixed Show resolved Hide resolved

estebank reviewed Jul 23, 2025

View reviewed changes

tests/ui/parser/attribute-on-type.rs Show resolved Hide resolved

fmease reviewed Jul 23, 2025

View reviewed changes

tests/ui/parser/attribute-on-type.rs Outdated Show resolved Hide resolved

Kivooeo force-pushed the bad-attr branch from b107f87 to 9169c32 Compare July 23, 2025 17:09

This comment has been minimized.

Sign in to view

Kivooeo force-pushed the bad-attr branch from 9169c32 to cdc8db4 Compare July 29, 2025 20:45

fmease reviewed Jul 30, 2025

View reviewed changes

compiler/rustc_parse/src/errors.rs Outdated Show resolved Hide resolved

Kivooeo force-pushed the bad-attr branch from cdc8db4 to e47dac2 Compare July 30, 2025 13:25

Kivooeo commented Jul 30, 2025

View reviewed changes

estebank reviewed Jul 30, 2025

View reviewed changes

compiler/rustc_parse/src/errors.rs Outdated Show resolved Hide resolved

compiler/rustc_parse/src/errors.rs Outdated Show resolved Hide resolved

Added checks for attribute in type case

2b75dbd

Kivooeo force-pushed the bad-attr branch from e47dac2 to 2b75dbd Compare August 1, 2025 09:51


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

		if self.token.is_ident() {

		let guar = self.dcx().emit_err(AttributeOnEmptyGeneric { span: attr_span });
		return Ok(Some(GenericArg::Type(self.mk_ty(attr_span, TyKind::Err(guar)))));

Add checks for attributes in types #144195

Are you sure you want to change the base?

Add checks for attributes in types #144195

Conversation

Kivooeo commented Jul 19, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Jul 20, 2025

Uh oh!

This comment has been minimized.

Uh oh!

Kivooeo commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kivooeo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Kivooeo commented Jul 29, 2025

Uh oh!

fmease commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fmease Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kivooeo Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmease Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kivooeo Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kivooeo Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmease Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kivooeo Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Kivooeo commented Jul 19, 2025 •

edited by rustbot

Loading

Kivooeo commented Jul 21, 2025 •

edited

Loading

fmease commented Jul 30, 2025 •

edited

Loading

fmease Jul 30, 2025 •

edited

Loading

Kivooeo Jul 30, 2025 •

edited

Loading

fmease Jul 30, 2025 •

edited

Loading

Kivooeo Jul 30, 2025 •

edited

Loading

Kivooeo Jul 30, 2025 •

edited

Loading

fmease Jul 30, 2025 •

edited

Loading

Kivooeo Jul 30, 2025 •

edited

Loading

fmease Aug 1, 2025 •

edited

Loading