Port `diagnostic::do_not_recommend` to new attr parsing #151056

mejrs · 2026-01-13T13:03:16Z

r? @jdonszelmann

rustbot · 2026-01-13T13:03:20Z

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

rust-bors · 2026-01-13T14:30:11Z

☔ The latest upstream changes (presumably #151051) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2026-01-13T15:01:06Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

JonathanBrouwer · 2026-01-13T18:10:41Z

compiler/rustc_attr_parsing/src/parser.rs

-                if parts == &[sym::rustc_dummy] {
+                // The arguments of rustc_dummy and diagnostic attributes are not validated
+                // if the arguments are delimited
+                if parts == &[sym::rustc_dummy] || parts[0] == sym::diagnostic {


Hmmm the special casting of this attribute is a little ugly here. Especially since this will affect all diagnostic attributes, and we're introducing a new one soon (#150935) which should not suffer the mistakes from the past

Could we perhaps do a crater run and see how bad making a breaking change is for this? Since these attributes are so niche I expect making the breaking change will be fine

which should not suffer the mistakes from the past

There were no mistakes, this is the intended behavior. That is, any syntactically valid input is allowed, similar to attribute macros. I do not want to shut the door on being able to use non-metaitem syntax going forward. For example in the future I'd like to expand these attributes to reference types and traits, and something like From<i32> is not valid metaitem syntax.

If we want to do this in the future, we would need to add support for parsing types and traits to the attribute parser. As I see it, that is even more an argument for removing this special case now, since this special case skips the attribute parser for this attribute.

I.e. If this attribute skips the attribute parser now, we can not use the attribute parser to parse any arguments in the future

If we want to do this in the future, we would need to add support for parsing types and traits to the attribute parser.

Yes, we would need to add some kind of support for that. But the semantics of the diagnostic namespace are that any such future extensions are ignored by older compilers and do not raise errors.

In other words: to extend the attribute in the future, we must be maximally permissive now.

See also the rfc: https://github.com/weiznich/rfcs/blob/diagnostic_attribute_namespace/text/3366-diagnostic-attribute-namespace.md

I see, that makes sense, thanks for explaining!
Could you add a comment to https://doc.rust-lang.org/reference/attributes/diagnostics.html#r-attributes.diagnostic.namespace.unknown-invalid-syntax on this check then?

This still doesn't really solve the usecase for diagnostic attributes that do need to have arguments parsed. Ideally the attribute parser would run, but any errors should be warnings instead. Not sure if we should solve this problem in this PR or when it comes up tho

Not sure if we should solve this problem in this PR or when it comes up tho

This will be an issue with the other diagnostic attributes, so I'll see if I can figure something out.

compiler/rustc_attr_parsing/src/attributes/do_not_recommend.rs

compiler/rustc_hir/src/attrs/data_structures.rs

JonathanBrouwer

Nice job! Some minor comments :)

View changes since this review

rustbot · 2026-01-13T18:19:22Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

JonathanBrouwer · 2026-01-13T18:20:14Z

(Jana feel free to steal the assign back, since I think you two might know eachother?)

mejrs · 2026-01-13T23:00:01Z

I've pushed the link , I'll need to think more about a proper fix for the other diagnostic attributes. But that doesn't matter today.

@rustbot ready

JonathanBrouwer · 2026-01-14T07:38:54Z

Don't expect this to have a significant performance impact, but attr parsing has been surprisingly perf sensitive so let's see
@bors try @rust-timer queue

Port `diagnostic::do_not_recommend` to new attr parsing

rust-bors · 2026-01-14T09:53:31Z

☀️ Try build successful (CI)
Build commit: f1840ed (f1840edf4b3cea3df43ffa5d38ac815208afedae, parent: 4931e09e3ac3182d2a00f38cccfdf68e8e385e1c)

rust-timer · 2026-01-14T10:34:26Z

Finished benchmarking commit (f1840ed): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.8%, secondary -3.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-3.8%	[-3.8%, -3.8%]	1
All ❌✅ (primary)	-0.8%	[-0.8%, -0.8%]	1

Cycles

Results (primary -2.7%, secondary 3.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.7%	[3.7%, 3.7%]	1
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.7%	[-2.7%, -2.7%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 474.176s -> 474.127s (-0.01%)
Artifact size: 383.16 MiB -> 383.18 MiB (0.00%)

rustbot assigned jdonszelmann Jan 13, 2026

mejrs force-pushed the do_not_recommend branch from 787588b to b54dd13 Compare January 13, 2026 15:01

This comment has been minimized.

Sign in to view

mejrs force-pushed the do_not_recommend branch from b54dd13 to d53e06b Compare January 13, 2026 15:29

This comment has been minimized.

Sign in to view

JonathanBrouwer reviewed Jan 13, 2026

View reviewed changes

compiler/rustc_attr_parsing/src/attributes/do_not_recommend.rs Show resolved Hide resolved

JonathanBrouwer reviewed Jan 13, 2026

View reviewed changes

compiler/rustc_attr_parsing/src/attributes/do_not_recommend.rs Show resolved Hide resolved

JonathanBrouwer reviewed Jan 13, 2026

View reviewed changes

compiler/rustc_hir/src/attrs/data_structures.rs Show resolved Hide resolved

JonathanBrouwer requested changes Jan 13, 2026

View reviewed changes

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 Jan 13, 2026

JonathanBrouwer assigned JonathanBrouwer and unassigned jdonszelmann Jan 13, 2026

mejrs added 2 commits January 13, 2026 20:43

Port do_not_recommend to new attr parsing

a1e2cea

Keep allowing invalid metaitem syntax in diagnostic attributes

ed6e630

mejrs force-pushed the do_not_recommend branch from d53e06b to ed6e630 Compare January 13, 2026 19:43

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 Jan 13, 2026

This comment has been minimized.

Sign in to view

rust-bors bot pushed a commit that referenced this pull request Jan 14, 2026

Auto merge of #151056 - mejrs:do_not_recommend, r=<try>

f1840ed

Port `diagnostic::do_not_recommend` to new attr parsing

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 14, 2026

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 14, 2026

Uh oh!

Port diagnostic::do_not_recommend to new attr parsing #151056

Are you sure you want to change the base?

Port diagnostic::do_not_recommend to new attr parsing #151056

Uh oh!

Conversation

mejrs commented Jan 13, 2026

Uh oh!

rustbot commented Jan 13, 2026

Uh oh!

rust-bors bot commented Jan 13, 2026

Uh oh!

rustbot commented Jan 13, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

JonathanBrouwer Jan 13, 2026 • 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!

JonathanBrouwer Jan 13, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JonathanBrouwer left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Jan 13, 2026

Uh oh!

JonathanBrouwer commented Jan 13, 2026

Uh oh!

mejrs commented Jan 13, 2026

Uh oh!

JonathanBrouwer commented Jan 14, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors bot commented Jan 14, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Jan 14, 2026

Overall result: ❌✅ regressions and improvements - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Port `diagnostic::do_not_recommend` to new attr parsing #151056

Port `diagnostic::do_not_recommend` to new attr parsing #151056

JonathanBrouwer Jan 13, 2026 •

edited

Loading

JonathanBrouwer Jan 13, 2026 •

edited

Loading

JonathanBrouwer left a comment •

edited by rustbot

Loading