Extract TraitImplHeader in AST/HIR #144386

camsteffen · 2025-07-24T02:58:40Z

Several fields of Impl are only applicable when it's a trait impl. This moves those fields into a new struct that is only present for trait impls.

rustbot · 2025-07-24T02:58:45Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-07-24T02:58:48Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

HIR ty lowering was modified

cc @fmease

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

cc @jdonszelmann

camsteffen · 2025-07-24T03:27:58Z

HIR ty lowering was modified

I don't think that's true fwiw.

xizheyin · 2025-07-24T04:19:09Z

HIR ty lowering was modified

This means this PR change the code in compiler/rustc_hir_analysis/src/hir_ty_lowering/, e.g. https://github.com/rust-lang/rust/pull/144386/files#diff-ce4273e1e949bf3052de7a08466c4dad785890b24091122954541b615b0ba199

camsteffen · 2025-07-24T04:39:34Z

@rustbot author

rustbot · 2025-07-24T04:39:38Z

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

compiler/rustc_parse/messages.ftl

compiler/rustc_ast/src/ast.rs

fmease · 2025-07-24T15:52:07Z

I'll perf this due to the added indirection for impl "modifiers" + extra alloc + size changes.

@bors try @rust-timer queue

rust-bors · 2025-07-24T15:52:11Z

⌛ Trying commit c0c102d with merge 513dd2b…

To cancel the try build, run the command @bors try cancel.

Extract ImplOfTrait in AST/HIR

fmease

Unfortunately this PR can't proceed without T-lang involvement and crater. This restricts Rust's stable grammar which is a breaking change.

Moreover, speaking from my experience it's quite unlikely that T-lang will approve narrowing the grammar since the general tendency has been to relax the grammar.

fmease · 2025-07-24T16:00:54Z

compiler/rustc_parse/src/parser/item.rs

+                let self_ty = ty_first;
+                let error = |annotation_span, annotation, only_trait| errors::InherentImplCannot {
+                    span: self_ty.span,
+                    annotation_span,
+                    annotation,
+                    self_ty: self_ty.span,
+                    only_trait,
+                };
+
+                if let Safety::Unsafe(span) = safety {
+                    self.dcx().emit_err(errors::InherentImplCannotUnsafe {
+                        span: self_ty.span,
+                        annotation_span: span,
+                        annotation: "unsafe",
+                        self_ty: self_ty.span,
+                    });
+                }
+                if let ImplPolarity::Negative(span) = polarity {
+                    self.dcx().emit_err(error(span, "negative", false));
+                }
+                if let Defaultness::Default(def_span) = defaultness {
+                    self.dcx().emit_err(error(def_span, "`default`", true));
+                }
+                if let Const::Yes(span) = constness {
+                    self.dcx().emit_err(error(span, "`const`", true));
+                }


Moving checks from AST validation (or later stages in general) into the parser is a breaking change! This will regress the following code:

#![feature(const_trait_impl)] // only necessary for the 2nd case #[cfg(false)] unsafe impl Local {} #[cfg(false)] impl const Local {} #[cfg(false)] impl !Local {} #[cfg(false)] default impl Local {} struct Local;

Oh! Shucks. Well obviously I'll wait for T-lang but I feel like this is defensible.

If not, perhaps we could do the change in the HIR only.

As an example, this is accepted today https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=888b72972d52d0597edbf7e7f2bc7b2e

#[cfg(false)] trait Foo { pub fn bar(); }

even though pub "isn't" accepted there.

I think really what you're finding here is that we should have had the type first (before the optional trait), because that would have been much nicer to deal with. Or it should have been something like impl<T> trait Clone for Foo<T> so there's a warning-of-front to make this LL(1).

I don't know what the rest of lang would say about this, whether we might take it under a "it's a bug that this was ever accepted" or say the parser should just support this under "well we should just item accept modifiers in the parser, even if they're rejected later as not being applicable".

Ah Scott, glad to have you here! I have a try-build ready, I could queue a crater run if you think that there's a non-zero chance of T-lang accepting this grammar change.

trait Foo { pub fn bar(); }

Also quite notable and quite related: Syntactically we permit top-level fns without bodies. This and Scott's example is of course thanks to / an instance of Centril's syntactic unification of "everything fn" and other grammar relaxations I was alluding to earlier. See e.g., RUST-70261, RUST-68788, RUST-69366, RUST-69361, RUST-69194, RUST-69023, RUST-68764, RUST-68728.

Side note for [@]camsteffen (unless you already know this), the #[cfg(false)] is only a representative of / a stand-in for macros in general, meaning this detail can be observed by decl & proc macros, not just #[cfg]. Pre-existing user-written macros can break due to this change (users may exploit this for their DSLs).

However, it's not as clear-cut. For example, T-lang recently restricted the stable grammar here: RUST-127054.

What about buffer_lint? It's not a lint but it seems like that's basically what we want, if we want to avoid the breakage?

I've now issued a crater run so we can get some data. So we can check the actual fallout. Let's wait for that.

Regarding buffering. I'm on my phone, so can't check. I only know early lint buffering for phases where we haven't resolved names yet. I'm not sure if you can buffer lints from within the parser (like if there's a buffer there already), I don't remember there being one but I might be mistaken.

fmease · 2025-07-24T16:04:26Z

@bors try cancel

fmease · 2025-07-29T16:19:28Z

Hello T-lang 👋,

this PR renders "trait impl modifiers" in inherent impls ungrammatical / syntactically invalid. Presently they're merely semantically malformed. Stable code like the following will now be rejected:

#[cfg(false)] unsafe impl Type {}
#[cfg(false)] impl !Type {}
#[cfg(false)] default impl Type {}

The #[cfg(false)] is just a stand-in for arbitrary decl macros and bang/attribute proc macros. It's conceivable that users might want to use this for their DSLs but realistically speaking it's relatively unlikely. We've cratered this and found no regressions (via).

The motivation for this change is the simplification of the compiler alongside small perf improvements likely due to the better data representation/layout this change enables (via).

src/tools/clippy/clippy_utils/src/visitors.rs

traviscross · 2025-07-30T05:46:06Z

Interestingly, if we decide to accept this PR, the Reference is already correct (here) (it does mean to specify what's accepted pre-expansion).

Based on that and on my general inclination to match parsing to our semantics where reasonable, I proposed to do this.

@rfcbot fcp merge

That said, though I appreciate the optimism, this is not the kind of thing that's an easy decision for us.

@rustbot labels -I-lang-easy-decision

rfcbot · 2025-07-30T05:46:08Z

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

tmandry · 2025-07-30T15:30:33Z

@rfcbot reviewed

rfcbot · 2025-07-30T15:30:41Z

🔔 This is now entering its final comment period, as per the review above. 🔔

This is a technically a breaking change for what can be parsed in `#[cfg(false)]`.

rustbot assigned SparrowLii Jul 24, 2025

This comment has been minimized.

Sign in to view

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

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 24, 2025

xizheyin reviewed Jul 24, 2025

View reviewed changes

compiler/rustc_parse/messages.ftl Outdated Show resolved Hide resolved

fbstj reviewed Jul 24, 2025

View reviewed changes

compiler/rustc_ast/src/ast.rs Show resolved Hide resolved

camsteffen force-pushed the imploftrait branch from 2b8e50d to 221aefa Compare July 24, 2025 15:04

This comment has been minimized.

Sign in to view

camsteffen force-pushed the imploftrait branch from 221aefa to c0c102d Compare July 24, 2025 15:17

fmease reviewed Jul 24, 2025

View reviewed changes

compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Jul 24, 2025

Auto merge of #144386 - camsteffen:imploftrait, r=<try>

513dd2b

Extract ImplOfTrait in AST/HIR

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 24, 2025

This comment has been minimized.

Sign in to view

fmease requested changes Jul 24, 2025

View reviewed changes

fmease added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2025

fmease added I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination labels Jul 29, 2025

blyxyas reviewed Jul 29, 2025

View reviewed changes

src/tools/clippy/clippy_utils/src/visitors.rs Show resolved Hide resolved

traviscross added the P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang label Jul 29, 2025

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 30, 2025

rustbot removed the I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination label Jul 30, 2025

rfcbot added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Jul 30, 2025

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 30, 2025

Factor out InherentImplCannotUnsafe

ef05f9e

camsteffen force-pushed the imploftrait branch from 2d27737 to fe480b1 Compare July 30, 2025 22:52

rustbot added the T-clippy Relevant to the Clippy team. label Jul 30, 2025

camsteffen added 4 commits July 30, 2025 17:59

Move trait impl modifier errors to parsing

1ee8579

This is a technically a breaking change for what can be parsed in `#[cfg(false)]`.

Tweak trait modifier errors

3dc0602

Extract ast TraitImplHeader

7ef7c7b

Propagate TraitImplHeader to hir

ae4cbbf

camsteffen force-pushed the imploftrait branch from fe480b1 to ae4cbbf Compare July 30, 2025 23:01

camsteffen changed the title ~~Extract ImplOfTrait in AST/HIR~~ Extract TraitImplHeader in AST/HIR Jul 30, 2025

fmease added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 31, 2025

Extract TraitImplHeader in AST/HIR #144386

Are you sure you want to change the base?

Extract TraitImplHeader in AST/HIR #144386

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rustbot commented Jul 24, 2025

Uh oh!

rustbot commented Jul 24, 2025

Uh oh!

This comment has been minimized.

camsteffen commented Jul 24, 2025

Uh oh!

This comment has been minimized.

xizheyin commented Jul 24, 2025

Uh oh!

camsteffen commented Jul 24, 2025

Uh oh!

rustbot commented Jul 24, 2025

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

fmease commented Jul 24, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Jul 24, 2025

Uh oh!

This comment has been minimized.

fmease left a comment

Choose a reason for hiding this comment

Uh oh!

fmease Jul 24, 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!

camsteffen Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

scottmcm Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

fmease Jul 25, 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!

camsteffen Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

fmease Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

fmease commented Jul 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

tmandry commented Jul 30, 2025

Uh oh!

rfcbot commented Jul 30, 2025

Uh oh!

Uh oh!

camsteffen commented Jul 24, 2025 •

edited

Loading

fmease Jul 24, 2025 •

edited

Loading

fmease Jul 25, 2025 •

edited

Loading

fmease commented Jul 29, 2025 •

edited

Loading

traviscross commented Jul 30, 2025 •

edited

Loading

rfcbot commented Jul 30, 2025 •

edited

Loading