Skip to content

Introduce impl restrictions to AST, lower to rustc_middle #141754

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jhpratt
Copy link
Member

@jhpratt jhpratt commented May 30, 2025

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

@jhpratt jhpratt added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. F-impl_restriction `#![feature(impl_restriction)]` A-ast Area: AST labels May 30, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 30, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@rust-log-analyzer

This comment has been minimized.

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

jhpratt commented May 30, 2025

@bors try

@bors
Copy link
Collaborator

bors commented May 30, 2025

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

bors added a commit that referenced this pull request May 30, 2025
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
Copy link
Collaborator

bors commented May 30, 2025

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

Copy link
Member

@Urgau Urgau left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Only have some small things.

Comment on lines +3328 to +3330
Unrestricted,
Restricted { path: P<Path>, id: NodeId, shorthand: bool },
Implied,
Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines +3533 to 3535
pub impl_restriction: Restriction,
pub safety: Safety,
pub is_auto: IsAuto,
Copy link
Member

Choose a reason for hiding this comment

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

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,

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Comment on lines +370 to 372
self.print_restriction("impl", impl_restriction);
self.print_safety(*safety);
self.print_is_auto(*is_auto);
Copy link
Member

Choose a reason for hiding this comment

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

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);

@@ -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)),
Copy link
Member

Choose a reason for hiding this comment

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

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)),

@@ -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: _,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the benefit of this?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -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
Copy link
Member

@Urgau Urgau May 31, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

#[test]
fn test_impl_restriction() {
Copy link
Member

Choose a reason for hiding this comment

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

Could you test additional tests with unsafe and auto.


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

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

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@Urgau Urgau 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 May 31, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area: AST F-impl_restriction `#![feature(impl_restriction)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

6 participants