-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
auto-impl: parser support #149335
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: main
Are you sure you want to change the base?
auto-impl: parser support #149335
Conversation
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in compiler/rustc_passes/src/check_attr.rs This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a HIR ty lowering was modified cc @fmease Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt Some changes occurred in compiler/rustc_sanitizers cc @rcvalle |
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
This patch introduce AST elements for `auto impl` inside the `trait` and `impl` block. This patch does not handle the name resolution, yet. It will be handled in the next patch series. Signed-off-by: Xiangfei Ding <dingxiangfei2009@protonmail.ch>
644d1ce to
fac5797
Compare
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 we add a simple formatting test for the AutoImpl changes added to rustfmt.
|
I think the priority here is to first get parser code proof-read. |
| AssocKind::Const { .. } => Const, | ||
| AssocKind::Type { .. } => Type, | ||
| AssocKind::Fn { .. } => Fn, | ||
| AssocKind::AutoImpl => todo!(), |
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'm not really comfortable with a todo!() here. Please add a new variant to SourceItemOrderingTraitAssocItemKind
|
It's probably more a language-related comment, than compiler-related, but the placement of auto impls doesn't seem right. Auto impls are not associated items, they could very well live as free items, and placed into the traits just for the proximity. Even from the compiler point of view in HIR and below we'd now need to separate real associated items from the things that just live there in source code, and may break some other assumptions across the compiler about only one level of associated item nesting existing, making building the prototype harder. |
|
The semicolon in |
|
Could you add some tests executing all the supported syntax, including |
| | ItemKind::DelegationMac(_) | ||
| | ItemKind::MacroDef(..) => None, | ||
| ItemKind::Static(_) => None, | ||
| ItemKind::AutoImpl(_) => None, |
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.
There are generics in AutoImpl, but this returns None.
| self.print_assoc_item(impl_item); | ||
| } | ||
| let empty = item.attrs.is_empty() && items.is_empty(); | ||
| self.bclose(item.span, empty, cb); |
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.
print_assoc_auto_impl was supposed to be reused here?
| /// Allows `super let` statements. | ||
| (unstable, super_let, "1.88.0", Some(139076)), | ||
| /// Allows supertraits to be implemented with automatic defaults | ||
| (unstable, supertrait_auto_impl, "CURRENT_RUSTC_VERSION", Some(99999)), |
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.
There's no tracking issue for the experiment?
Probably need a lang team approval and a tracking issue to start.
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.
Thanks @petrochenkov for raising this. You're right; this should have a tracking issue documenting the lang experiment and its champion. Talked with @dingxiangfei2009, after he pinged me, and he's filing the tracking issue for this, which we'll nominate and discuss in a meeting.
| self.r | ||
| .tcx() | ||
| .dcx() | ||
| .emit_err(errors::AutoImplOutsideTraitOrImplTrait { span: item.span }); |
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.
This belongs to ast_validation.rs, or AST lowering, or something like that.
| } | ||
| AssocItemKind::AutoImpl(_ai) => { | ||
| let tcx = self.r.tcx(); | ||
| if !tcx.features().supertrait_auto_impl() { |
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.
Since this is a new syntax, feature gating needs to be done in parser using gated_spans.
|
(I'll continue the review tomorrow.) |
| constness, | ||
| }) | ||
| } | ||
| ItemKind::AutoImpl(box AutoImpl { .. }) => todo!("we should implement lowering to HIR"), |
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 report a regular error instead of an ICE here and for AssocItemKind::AutoImpl?
Upd: and in other crates too.
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.
Or report a fatal error in lowering and turn todo!s into unreachable!s in the later compilation stages.
| AssocItemKind::AutoImpl(ai) => { | ||
| let tcx = self.tcx; | ||
| if !tcx.features().supertrait_auto_impl() { | ||
| feature_err( |
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.
Feature gating once in the parser should be enough.
| // Re. GATs and GACs (generic_const_items), we're not bound by backward compatibility. | ||
| ImplItemKind::Const(..) | ImplItemKind::Type(..) => ParamDefaultPolicy::Forbidden, | ||
| ImplItemKind::Fn(..) => ParamDefaultPolicy::FutureCompatForbidden, | ||
| ImplItemKind::AutoImpl(..) => ParamDefaultPolicy::Forbidden, |
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.
| ImplItemKind::AutoImpl(..) => ParamDefaultPolicy::Forbidden, | |
| ImplItemKind::AutoImpl(..) | ImplItemKind::ExternImpl(..) => ParamDefaultPolicy::Forbidden, |
| hir::ImplItemKind::AutoImpl(poly_trait_ref, impl_items) => { | ||
| let (cb, ib) = self.head(""); | ||
| self.word_nbsp("auto"); | ||
| self.word_nbsp("impl"); |
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.
Maybe factor out the common parts like in AST printing?
| /// An `auto impl` implementation | ||
| AutoImpl(&'hir PolyTraitRef<'hir>, &'hir [ImplItem<'hir>]), | ||
| /// An `extern impl` directive | ||
| ExternImpl(&'hir PolyTraitRef<'hir>), |
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.
Where is ExternImpl actually generated?
I don't see being produced by AST lowering.
| let (check_ty, is_assoc_ty) = match item.kind { | ||
| ty::AssocKind::Const { .. } | ty::AssocKind::Fn { .. } => (true, false), | ||
| ty::AssocKind::Type { .. } => (item.defaultness(self.tcx).has_value(), true), | ||
| ty::AssocKind::AutoImpl => return, |
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'm pretty sure the auto impls should be privacy-checked here too.
| if ctxt == AssocCtxt::Trait { | ||
| if let AssocItemKind::AutoImpl(_) = item.kind { | ||
| // We do not define any local names. | ||
| // We will now recursively descend into `auto impl` |
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.
Why not? This doesn't seem right.
| self.bump(); // `union` | ||
| self.parse_item_union()? | ||
| } else if self.check_keyword(exp!(Unsafe)) | ||
| && self.is_keyword_ahead(1, &[kw::Auto, kw::Impl]) |
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.
This catches non-auto unsafe impl.
|
After reading the PR I'd again suggest to implement these impls as free items first, because you'll have a whole host of issues just from trying to move them from free to associated items. |
|
Reminder, once the PR becomes ready for a review, use |
| } | ||
| ty::AssocKind::Const { .. } => None, | ||
| // This does not change judgement on GAT | ||
| ty::AssocKind::AutoImpl => continue, |
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 want the type system to have to care about a new assoc kind. Also please get types approval before having this merged (I've assigned myself). Why is this PR doing more than just parser stuff like the title and PR description indicates?
|
☔ The latest upstream changes (presumably #146348) made this pull request unmergeable. Please resolve the merge conflicts. |
Tracking:
auto impl#149556This patch introduce AST elements for
auto implinside thetraitandimplblock.This patch does not handle the name resolution, yet. It will be handled in the next patch series.
RFC: rust-lang/rfcs#3851
cc
As a tracking issue is pending, I will link rust-lang/rust-project-goals#393 for more context.