-
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?
Changes from all commits
206f423
2c412f1
11cba59
14626e4
7f84bd4
12ce9a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -32,7 +32,7 @@ use rustc_data_structures::tagged_ptr::Tag; | |||||||||||||
use rustc_macros::{Decodable, Encodable, HashStable_Generic}; | ||||||||||||||
pub use rustc_span::AttrId; | ||||||||||||||
use rustc_span::source_map::{Spanned, respan}; | ||||||||||||||
use rustc_span::{ErrorGuaranteed, Ident, Span, Symbol, kw, sym}; | ||||||||||||||
use rustc_span::{DUMMY_SP, ErrorGuaranteed, Ident, Span, Symbol, kw, sym}; | ||||||||||||||
use thin_vec::{ThinVec, thin_vec}; | ||||||||||||||
|
||||||||||||||
pub use crate::format::*; | ||||||||||||||
|
@@ -3316,6 +3316,42 @@ impl VisibilityKind { | |||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
#[derive(Clone, Encodable, Decodable, Debug)] | ||||||||||||||
pub struct Restriction { | ||||||||||||||
pub kind: RestrictionKind, | ||||||||||||||
pub span: Span, | ||||||||||||||
pub tokens: Option<LazyAttrTokenStream>, | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
#[derive(Clone, Encodable, Decodable, Debug)] | ||||||||||||||
pub enum RestrictionKind { | ||||||||||||||
Unrestricted, | ||||||||||||||
Restricted { path: P<Path>, id: NodeId, shorthand: bool }, | ||||||||||||||
Implied, | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
impl Restriction { | ||||||||||||||
pub fn unrestricted() -> Self { | ||||||||||||||
Restriction { kind: RestrictionKind::Unrestricted, span: DUMMY_SP, tokens: None } | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
pub fn restricted(path: P<Path>, id: NodeId, shorthand: bool) -> Self { | ||||||||||||||
Restriction { | ||||||||||||||
kind: RestrictionKind::Restricted { path, id, shorthand }, | ||||||||||||||
span: DUMMY_SP, | ||||||||||||||
tokens: None, | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
pub fn implied() -> Self { | ||||||||||||||
Restriction { kind: RestrictionKind::Implied, span: DUMMY_SP, tokens: None } | ||||||||||||||
} | ||||||||||||||
Comment on lines
+3334
to
+3348
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the previous PR, https://github.com/rust-lang/rust/pull/106074/files#diff-68dedde73c3f169dfeeed73fc362bede2bb414477a495b8b2fb626fc5e91dd19R172 seems to be the only place where a Could you change the methods so take There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do. Passing |
||||||||||||||
|
||||||||||||||
pub fn with_span(self, span: Span) -> Self { | ||||||||||||||
Restriction { span, ..self } | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// Field definition in a struct, variant or union. | ||||||||||||||
/// | ||||||||||||||
/// E.g., `bar: usize` as in `struct Foo { bar: usize }`. | ||||||||||||||
|
@@ -3494,6 +3530,7 @@ impl Default for FnHeader { | |||||||||||||
|
||||||||||||||
#[derive(Clone, Encodable, Decodable, Debug)] | ||||||||||||||
pub struct Trait { | ||||||||||||||
pub impl_restriction: Restriction, | ||||||||||||||
pub safety: Safety, | ||||||||||||||
pub is_auto: IsAuto, | ||||||||||||||
Comment on lines
+3533
to
3535
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the synctax changes from the RFC,
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Argh…you're right, though I do think |
||||||||||||||
pub ident: Ident, | ||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 |
||||||||
is_auto, | ||||||||
safety, | ||||||||
ident, | ||||||||
generics, | ||||||||
bounds, | ||||||||
items, | ||||||||
}) => { | ||||||||
let ident = self.lower_ident(*ident); | ||||||||
let (generics, (safety, items, bounds)) = self.lower_generics( | ||||||||
generics, | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -357,6 +357,7 @@ impl<'a> State<'a> { | |||||||||||||
self.bclose(item.span, empty, cb); | ||||||||||||||
} | ||||||||||||||
ast::ItemKind::Trait(box ast::Trait { | ||||||||||||||
impl_restriction, | ||||||||||||||
safety, | ||||||||||||||
is_auto, | ||||||||||||||
ident, | ||||||||||||||
|
@@ -366,6 +367,7 @@ impl<'a> State<'a> { | |||||||||||||
}) => { | ||||||||||||||
let (cb, ib) = self.head(""); | ||||||||||||||
self.print_visibility(&item.vis); | ||||||||||||||
self.print_restriction("impl", impl_restriction); | ||||||||||||||
self.print_safety(*safety); | ||||||||||||||
self.print_is_auto(*is_auto); | ||||||||||||||
Comment on lines
+370
to
372
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be re-ordered.
Suggested change
|
||||||||||||||
self.word_nbsp("trait"); | ||||||||||||||
|
@@ -473,6 +475,22 @@ impl<'a> State<'a> { | |||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
// FIXME(jhpratt) make `kw` into a const generic once permitted | ||||||||||||||
pub(crate) fn print_restriction(&mut self, kw: &'static str, restriction: &ast::Restriction) { | ||||||||||||||
match restriction.kind { | ||||||||||||||
ast::RestrictionKind::Unrestricted => self.word_nbsp(kw), | ||||||||||||||
ast::RestrictionKind::Restricted { ref path, shorthand, id: _ } => { | ||||||||||||||
let path = Self::to_string(|s| s.print_path(path, false, 0)); | ||||||||||||||
if shorthand { | ||||||||||||||
self.word_nbsp(format!("{kw}({path})")) | ||||||||||||||
} else { | ||||||||||||||
self.word_nbsp(format!("{kw}(in {path})")) | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
ast::RestrictionKind::Implied => {} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
fn print_defaultness(&mut self, defaultness: ast::Defaultness) { | ||||||||||||||
if let ast::Defaultness::Default(_) = defaultness { | ||||||||||||||
self.word_nbsp("default"); | ||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. The feature should be marked
Suggested change
|
||||||
/// Allows `impl Trait` to be used inside associated types (RFC 2515). | ||||||
(unstable, impl_trait_in_assoc_type, "1.70.0", Some(63063)), | ||||||
/// Allows `impl Trait` in bindings (`let`). | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,6 +178,7 @@ pub struct ResolverOutputs { | |
#[derive(Debug)] | ||
pub struct ResolverGlobalCtxt { | ||
pub visibilities_for_hashing: Vec<(LocalDefId, Visibility)>, | ||
pub impl_restrictions: FxIndexMap<LocalDefId, Restriction>, | ||
/// Item with a given `LocalDefId` was defined during macro expansion with ID `ExpnId`. | ||
pub expn_that_defined: FxHashMap<LocalDefId, ExpnId>, | ||
pub effective_visibilities: EffectiveVisibilities, | ||
|
@@ -322,6 +323,35 @@ impl Visibility { | |
} | ||
} | ||
|
||
#[derive(Clone, Debug, PartialEq, Eq, Copy, Hash, Encodable, Decodable, HashStable)] | ||
pub enum Restriction { | ||
/// The restriction does not affect the item. | ||
Unrestricted, | ||
/// The restriction only applies outside of this path. | ||
Restricted(DefId, Span), | ||
} | ||
|
||
impl Restriction { | ||
/// Returns `true` if the behavior is allowed/unrestricted in the given module. A value of | ||
/// `false` indicates that the behavior is prohibited. | ||
pub fn is_allowed_in(self, module: DefId, tcx: TyCtxt<'_>) -> bool { | ||
let restricted_to = match self { | ||
Restriction::Unrestricted => return true, | ||
Restriction::Restricted(module, _) => module, | ||
}; | ||
|
||
tcx.is_descendant_of(module, restricted_to.into()) | ||
} | ||
|
||
/// Obtain the [`Span`] of the restriction. If unrestricted, an empty span is returned. | ||
pub fn span(&self) -> Span { | ||
match self { | ||
Restriction::Unrestricted => DUMMY_SP, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we instead return an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only used in one location ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is it really needed? If it's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The initial PR had a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think what #106074 (comment) suggested was to have a (not the) dummy span, probably the same empty span the parser uses and have |
||
Restriction::Restricted(_, span) => *span, | ||
} | ||
} | ||
} | ||
|
||
#[derive(Clone, Debug, PartialEq, Eq, Copy, Hash, TyEncodable, TyDecodable, HashStable)] | ||
#[derive(TypeFoldable, TypeVisitable)] | ||
pub struct ClosureSizeProfileData<'tcx> { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -377,6 +377,13 @@ parse_inclusive_range_no_end = inclusive range with no end | |
parse_incorrect_parens_trait_bounds = incorrect parentheses around trait bounds | ||
parse_incorrect_parens_trait_bounds_sugg = fix the parentheses | ||
|
||
parse_incorrect_restriction = incorrect {parse_restriction_noun} restriction | ||
.help = some possible {parse_restriction_noun} restrictions are: | ||
`{$keyword}(crate)`: {parse_restriction_adjective} only in the current crate | ||
`{$keyword}(super)`: {parse_restriction_adjective} only in the current module's parent | ||
`{$keyword}(in path::to::module)`: {parse_restriction_adjective} only in the specified path | ||
Comment on lines
+381
to
+384
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to also mention There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. I'll need to special-case |
||
.suggestion = make this {parse_restriction_adjective} only to module `{$path}` with `in` | ||
|
||
parse_incorrect_semicolon = | ||
expected item, found `;` | ||
.suggestion = remove this semicolon | ||
|
@@ -783,6 +790,18 @@ parse_reserved_string = invalid string literal | |
.note = unprefixed guarded string literals are reserved for future use since Rust 2024 | ||
.suggestion_whitespace = consider inserting whitespace here | ||
|
||
# internal use only | ||
parse_restriction_adjective = { $keyword -> | ||
[impl] implementable | ||
*[DEFAULT_IS_BUG] BUG | ||
} | ||
|
||
# internal use only | ||
parse_restriction_noun = { $keyword -> | ||
[impl] impl | ||
*[DEFAULT_IS_BUG] BUG | ||
} | ||
|
||
parse_return_types_use_thin_arrow = return types are denoted using `->` | ||
.suggestion = use `->` instead | ||
|
||
|
@@ -853,6 +872,7 @@ parse_trailing_vert_not_allowed = a trailing `|` is not allowed in an or-pattern | |
parse_trait_alias_cannot_be_auto = trait aliases cannot be `auto` | ||
parse_trait_alias_cannot_be_unsafe = trait aliases cannot be `unsafe` | ||
|
||
parse_trait_alias_cannot_have_impl_restriction = trait alias cannot have `impl` restriction | ||
parse_transpose_dyn_or_impl = `for<...>` expected after `{$kw}`, not before | ||
.suggestion = move `{$kw}` before the `for<...>` | ||
|
||
|
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
andImplied
?As far as I can see from the parser,
Implied
seems to mean noat all, whileimpl
Unrestricted
seems to meanimpl
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 onlyRestricted
andUnrestricted
(yourImplied
).What's the reasoning for that? Shouldn't we just have
Restricted
andUnrestricted
? Like so?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 ↩
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
andImplied
,Unrestricted
is an explicit keyword-only modifier, whileImplied
is "figure this out some other way". The reasonImplied
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
andmut
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.
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.