-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
5060bf9
to
b718886
Compare
This comment has been minimized.
This comment has been minimized.
b718886
to
12ce9a9
Compare
@bors try |
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
☀️ Try build successful - checks-actions |
There was a problem hiding this 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.
Unrestricted, | ||
Restricted { path: P<Path>, id: NodeId, shorthand: bool }, | ||
Implied, |
There was a problem hiding this comment.
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 at all, while impl
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?
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
pub impl_restriction: Restriction, | ||
pub safety: Safety, | ||
pub is_auto: IsAuto, |
There was a problem hiding this comment.
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.
pub impl_restriction: Restriction, | |
pub safety: Safety, | |
pub is_auto: IsAuto, | |
pub safety: Safety, | |
pub is_auto: IsAuto, | |
pub impl_restriction: Restriction, |
There was a problem hiding this comment.
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.
self.print_restriction("impl", impl_restriction); | ||
self.print_safety(*safety); | ||
self.print_is_auto(*is_auto); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be re-ordered.
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)), |
There was a problem hiding this comment.
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.
(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: _, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl_restriction: _, | |
// FIXME(impl_restrictions): lower to HIR | |
impl_restriction: _, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_restriction
s here.
@@ -793,6 +793,16 @@ fn test_vis() { | |||
// Attributes are not allowed on visibilities. | |||
} | |||
|
|||
#[test] | |||
fn test_impl_restriction() { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub impl(in crate::foo) trait Baz {} //[without_gate]~ ERROR | |
pub impl(in crate::foo) trait Baz {} //[without_gate]~ ERROR impl restrictions are experimental |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 forrustfmt
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 torustc_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