Introduce `impl` restrictions to AST, lower to `rustc_middle` #141754

jhpratt · 2025-05-30T05:34:29Z

RFC approved in RFC 3323; part of tracking issue #105077 and a 2025H1 project goal.

This is likely easiest to review commit-by-commit.

First, the impl restriction is introduced to the AST, followed by actually parsing it into the AST on trait definitions (the only place it's permitted). Next we add the ability for rustfmt to handle it in the most logical manner; unstable features are permitted to choose a sane format without a team decision. After placing it behind a feature gate, the restriction is lowered to rustc_middle with identical limitations to visibility. Finally, the diagnostics are partially moved into Fluent to make them a bit more translatable.

This is the groundwork for the restriction and does not actually enforce anything; that is coming in another PR soon.

r? @Urgau as discussed

rustbot · 2025-05-30T05:34:35Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

jhpratt · 2025-05-30T07:46:50Z

@bors try

bors · 2025-05-30T07:48:01Z

⌛ Trying commit 12ce9a9 with merge 9a8c8a3...

Introduce `impl` restrictions to AST, lower to `rustc_middle` RFC approved in [RFC 3323](https://rust-lang.github.io/rfcs/3323-restrictions.html); part of tracking issue #105077 and a [2025H1 project goal](rust-lang/rust-project-goals#257). This is likely easiest to review commit-by-commit. First, the `impl` restriction is introduced to the AST, followed by actually parsing it into the AST on trait definitions (the only place it's permitted). Next we add the ability for `rustfmt` to handle it in the most logical manner; unstable features are permitted to choose a sane format without a team decision. After placing it behind a feature gate, the restriction is lowered to `rustc_middle` with identical limitations to visibility. Finally, the diagnostics are partially moved into Fluent to make them a bit more translatable. This is the groundwork for the restriction and does not actually enforce anything; that is coming in another PR soon. r? `@Urgau` as discussed

bors · 2025-05-30T10:09:20Z

☀️ Try build successful - checks-actions
Build commit: 9a8c8a3 (9a8c8a375188339cb3cc6330dde9280c67575cd9)

Urgau

Looks pretty good. Only have some small things.

Urgau · 2025-05-31T10:31:18Z

compiler/rustc_ast/src/ast.rs

+    Unrestricted,
+    Restricted { path: P<Path>, id: NodeId, shorthand: bool },
+    Implied,


What's the difference between Unrestricted and Implied?

As far as I can see from the parser, Implied seems to mean no ~~impl~~ at all, while Unrestricted seems to mean impl without parentheses or anything else, is that correct?

Looking at the RFC I can't seems to find any reference to just impl for restrictions, it's always supposed to be followed but parentheses¹.

And in ty::Restriction there is only Restricted and Unrestricted (your Implied).

What's the reasoning for that? Shouldn't we just have Restricted and Unrestricted? Like so?

Suggested change

Unrestricted,

Restricted { path: P<Path>, id: NodeId, shorthand: bool },

Implied,

/// No restriction.

Unrestricted,

/// Restricted, either `kw(crate)`, `kw(self)`, `kw(super)`, `kw(in path)`

Restricted { path: P<Path>, id: NodeId, shorthand: bool },

EDIT: Looking at the previous PR, it seems to be for the visiblity, which where pub alone is valid, https://github.com/rust-lang/rust/pull/106074/files#diff-00567f9be31678eec46651e6157f0eaaa47d1f344eb61b8a63f87edfa7bf6b14R2799-R2811.

Let's defer any merging with visiblity for a future PR.

Footnotes

https://rust-lang.github.io/rfcs/3323-restrictions.html#syntax ↩

While it will ultimately be used to parse visibility, this is also intended to reduce friction for any future restrictions that are added and have keyword-only syntax permitted.

As to the difference between Unrestricted and Implied, Unrestricted is an explicit keyword-only modifier, while Implied is "figure this out some other way". The reason Implied doesn't exist later on is that it is determined what the appropriate level is. In reality this means unrestricted, as if something is inaccessible (from visibility) then we don't care about the impl restriction.

For both impl and mut unrestricted isn't permitted (at least for now), but being able to parse this is plausibly useful to provide improved diagnostics.

Of course, if you feel strongly about this, I can leave it out, but it'll almost certainly be re-introduced in the future.

Looking back at the original PR, I missed one bit that needed to be carried over that would reject Unrestricted during parsing. The final bit about it being re-introduced later even if left out now remains true, so I think it's reasonable to leave it in regardless.

it's reasonable to leave it in regardless.

I don't feel strongly about it, so if you prefer to leave it, then leave it; but please add doc-comments explaining what each Restriction variant represent.

Urgau · 2025-05-31T10:41:31Z

compiler/rustc_ast/src/ast.rs

+    pub impl_restriction: Restriction,
    pub safety: Safety,
    pub is_auto: IsAuto,


Looking at the synctax changes from the RFC, impl restrictions are supposed to be after safety (and auto?), so let's keep them in their order of appearance in the source code.

Suggested change

pub impl_restriction: Restriction,

pub safety: Safety,

pub is_auto: IsAuto,

pub safety: Safety,

pub is_auto: IsAuto,

pub impl_restriction: Restriction,

Argh…you're right, though I do think pub impl(crate) unsafe auto is the right order ultimately. I'll update it per the RFC while noting that it's an unresolved question.

Urgau · 2025-05-31T10:42:22Z

compiler/rustc_ast_pretty/src/pprust/state/item.rs

+                self.print_restriction("impl", impl_restriction);
                self.print_safety(*safety);
                self.print_is_auto(*is_auto);


Should be re-ordered.

Suggested change

self.print_restriction("impl", impl_restriction);

self.print_safety(*safety);

self.print_is_auto(*is_auto);

self.print_safety(*safety);

self.print_is_auto(*is_auto);

self.print_restriction("impl", impl_restriction);

Urgau · 2025-05-31T10:49:45Z

compiler/rustc_feature/src/unstable.rs

@@ -534,6 +534,8 @@ declare_features! (
    (unstable, half_open_range_patterns_in_slices, "1.66.0", Some(67264)),
    /// Allows `if let` guard in match arms.
    (unstable, if_let_guard, "1.47.0", Some(51114)),
+    /// Allows `impl(crate) trait Foo` restrictions
+    (unstable, impl_restriction, "CURRENT_RUSTC_VERSION", Some(105077)),


The feature should be marked incomplete for now.

Suggested change

(unstable, impl_restriction, "CURRENT_RUSTC_VERSION", Some(105077)),

(incomplete, impl_restriction, "CURRENT_RUSTC_VERSION", Some(105077)),

Urgau · 2025-05-31T10:55:11Z

compiler/rustc_ast_lowering/src/item.rs

@@ -415,7 +415,15 @@ impl<'hir> LoweringContext<'_, 'hir> {
                    items: new_impl_items,
                }))
            }
-            ItemKind::Trait(box Trait { is_auto, safety, ident, generics, bounds, items }) => {
+            ItemKind::Trait(box Trait {
+                impl_restriction: _,


Suggested change

impl_restriction: _,

// FIXME(impl_restrictions): lower to HIR

impl_restriction: _,

What's the benefit of this?

It's just a nit. To make it explicit that it's not yet handle but should be. You can leave it if you prefer.

I meant what's the benefit of lowering it, not of the comment. Given that it's not handled by anything in HIR, I don't see why it needs to be present. Though perhaps I'm missing something obvious here.

For linting purpose it maybe be good to lower the span, like is done with the visibility span on Item. Otherwise I agree that lowering the Restriction it-self is not useful.

Urgau · 2025-05-31T11:20:41Z

src/tools/clippy/clippy_utils/src/ast_utils/mod.rs

@@ -444,6 +444,7 @@ pub fn eq_item_kind(l: &ItemKind, r: &ItemKind) -> bool {
        },
        (
            Trait(box ast::Trait {
+                impl_restriction: _, // does not affect item kind


If I have impl(self) trait Trait {} and impl(super) trait Trait {} they are different right?

We should probably change this method to compare them.

It would be impl(self) trait Foo {} (the trait definition rather than implementation). I don't think this would change anything, just as visibility doesn't. It changes what downstream users (incl. other modules) can do but nothing else.

Except that the visibility is also compared, in eq_item (as it's on the Item rather than ItemKind), so yeah I think we need to also compare the impl_restrictions here.

Urgau · 2025-05-31T11:21:28Z

tests/ui/macros/stringify.rs

@@ -793,6 +793,16 @@ fn test_vis() {
    // Attributes are not allowed on visibilities.
 }

+#[test]
+fn test_impl_restriction() {


Could you test additional tests with unsafe and auto.

Urgau · 2025-05-31T11:21:54Z

tests/ui/restrictions/feature-gate-impl-restriction.rs

+
+#![cfg_attr(with_gate, feature(impl_restriction))]
+
+pub impl(crate) trait Bar {} //[without_gate]~ ERROR


Suggested change

pub impl(crate) trait Bar {} //[without_gate]~ ERROR

pub impl(crate) trait Bar {} //[without_gate]~ ERROR impl restrictions are experimental

Urgau · 2025-05-31T11:22:02Z

tests/ui/restrictions/feature-gate-impl-restriction.rs

+pub impl(crate) trait Bar {} //[without_gate]~ ERROR
+
+mod foo {
+    pub impl(in crate::foo) trait Baz {} //[without_gate]~ ERROR


Suggested change

pub impl(in crate::foo) trait Baz {} //[without_gate]~ ERROR

pub impl(in crate::foo) trait Baz {} //[without_gate]~ ERROR impl restrictions are experimental

Urgau · 2025-05-31T11:22:55Z

compiler/rustc_resolve/src/errors.rs

Could you also add test for all these resolver errors please.

ytmimi · 2025-05-31T14:55:59Z

src/tools/rustfmt/tests/source/restriction.rs

Can you add a #![feature(impl_restriction)] to this test? I know it's not technically necessary for rustfmt, but it would be nice to more explicitly mention the feature in the test case.

jhpratt added T-compiler F-impl_restriction A-ast labels May 30, 2025

rustbot assigned Urgau May 30, 2025

rustbot added the S-waiting-on-review label May 30, 2025

Introduce restrictions to the AST

206f423

rust-cloud-vms bot force-pushed the restrictions-pr1 branch from 5060bf9 to b718886 Compare May 30, 2025 05:45

This was referenced May 30, 2025

Tracking Issue for Restrictions #105077

Open

Implement restrictions, prepare for stabilization rust-lang/rust-project-goals#257

Open

This comment has been minimized.

Sign in to view

jhpratt added 5 commits May 30, 2025 06:39

Parse impl restriction on trait definitions

2c412f1

rustfmt impl restriction

11cba59

Place impl restriction behind feature gate

14626e4

Lower impl restriction to rustc_middle

7f84bd4

Make diagnostics more translatable

12ce9a9

rust-cloud-vms bot force-pushed the restrictions-pr1 branch from b718886 to 12ce9a9 Compare May 30, 2025 06:39

Urgau reviewed May 31, 2025

View reviewed changes

Urgau added S-waiting-on-author and removed S-waiting-on-review labels May 31, 2025

ytmimi reviewed May 31, 2025

View reviewed changes

	(unstable, impl_restriction, "CURRENT_RUSTC_VERSION", Some(105077)),
	(incomplete, impl_restriction, "CURRENT_RUSTC_VERSION", Some(105077)),

	impl_restriction: _,
	// FIXME(impl_restrictions): lower to HIR
	impl_restriction: _,


		#![cfg_attr(with_gate, feature(impl_restriction))]

		pub impl(crate) trait Bar {} //[without_gate]~ ERROR

	pub impl(in crate::foo) trait Baz {} //[without_gate]~ ERROR
	pub impl(in crate::foo) trait Baz {} //[without_gate]~ ERROR impl restrictions are experimental

Introduce impl restrictions to AST, lower to rustc_middle #141754

Are you sure you want to change the base?

Introduce impl restrictions to AST, lower to rustc_middle #141754

Conversation

jhpratt commented May 30, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 30, 2025

Uh oh!

This comment has been minimized.

jhpratt commented May 30, 2025

Uh oh!

bors commented May 30, 2025

Uh oh!

bors commented May 30, 2025

Uh oh!

Urgau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

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!

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!

Urgau May 31, 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!

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!

Introduce `impl` restrictions to AST, lower to `rustc_middle` #141754

Introduce `impl` restrictions to AST, lower to `rustc_middle` #141754

jhpratt commented May 30, 2025 •

edited by rustbot

Loading

Urgau May 31, 2025 •

edited

Loading