-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Implement RFC 2302: tuple_struct_self_ctor #53751
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Thank you for doing this! More tests would be nice :)
src/doc/unstable-book/src/language-features/tuple-struct-self-ctor.md
Outdated
Show resolved
Hide resolved
let Self(v) = self; | ||
println!("{}", v); | ||
} | ||
} |
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.
Include tests for:
Self
as a function passed somewhereSelf
as a unit struct value wrt. pattern matching and construction.
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.
Test cases added.
println!("{}", v); | ||
} | ||
} | ||
|
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.
Include a test where all of the above works through type aliases.
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.
Type alias is not working. At present, even this case can not work:
struct S(i32);
type T = S;
fn main() {
let x = T(1);
}
I think type alias is unrelated to this feature. We should solve this in a separate PR.
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 test would rather be (and is part of the RFC):
struct S(i32);
type T = S;
impl T {
fn stuff() {
let Self(x) = match Self(0) { Self(x) => Self(x) };
let opt: Option<Self> = Some(0).map(Self);
}
}
For named field structs, this already works today:
pub struct S { f: i32 }
pub type T = S;
impl T {
pub fn stuff() {
let Self { f: _x } = match (Self { f: 0 }) { Self { f } => Self { f } };
}
}
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, test case is added.
src/librustc_resolve/lib.rs
Outdated
} else { | ||
Def::StructCtor(variant_id, CtorKind::Const) | ||
}; | ||
self.tuple_structs.insert(def_id, variant_def); |
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.
Constructors are already to this table in build_reduced_graph.rs
, so this seems to be duplicate work.
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.
Removed.
src/librustc_resolve/lib.rs
Outdated
@@ -1434,6 +1434,9 @@ pub struct Resolver<'a, 'b: 'a> { | |||
/// it's not used during normal resolution, only for better error reporting. | |||
struct_constructors: DefIdMap<(Def, ty::Visibility)>, | |||
|
|||
/// Map from tuple struct's DefId to VariantData's Def | |||
tuple_structs: DefIdMap<Def>, |
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.
With https://github.com/rust-lang/rust/pull/53751/files?w=1#r214416891 in mind, this field looks like an exact copy of struct_constructors
field above.
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. It is removed.
} | ||
} | ||
} | ||
|
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.
Still needs tests that it works through type aliases such as with:
struct S(i32);
type T = S;
impl T {
fn stuff() {
let Self(x) = match Self(0) { Self(x) => Self(x) };
let opt: Option<Self> = Some(0).map(Self);
}
}
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. Test case is added, see ST6
.
src/libsyntax/feature_gate.rs
Outdated
@@ -1755,6 +1758,15 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> { | |||
ast::ExprKind::Async(..) => { | |||
gate_feature_post!(&self, async_await, e.span, "async blocks are unstable"); | |||
} | |||
ast::ExprKind::Call(ref callee, _) => { | |||
if let ast::ExprKind::Path(_, ref p) = callee.node { |
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.
Unit struct constructors are not feature gated.
A better place to place the feature gate is probably lowering.rs
.
Every time we construct hir::Path {
with a single Self
segment and Def::StructCtor
definition we can report a feature 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.
Thanks. Fixed.
src/librustc_resolve/lib.rs
Outdated
fn with_tuple_struct_self_ctor_rib<F>(&mut self, self_ty: &Ty, f: F) | ||
where F: FnOnce(&mut Resolver) | ||
{ | ||
let variant_def = if self.session.features_untracked().tuple_struct_self_ctor { |
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 gates very rarely enable features, they usually report an error when some feature is already used, so this condition is not necessary.
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. Fixed.
src/librustc_resolve/lib.rs
Outdated
{ | ||
let variant_def = if self.session.features_untracked().tuple_struct_self_ctor { | ||
let base_def = self.def_map.get(&self_ty.id).map(|r| r.base_def()); | ||
if let Some(Def::Struct(ref def_id)) = base_def { |
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 won't work for type aliases in impl
.
struct S(u8);
type Alias = S;
impl Alias { ... Self(10) ... }
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. Test case is added.
src/librustc_resolve/lib.rs
Outdated
@@ -2521,8 +2570,9 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> { | |||
ValueNS, | |||
impl_item.span, | |||
|n, s| MethodNotMemberOfTrait(n, s)); | |||
|
|||
visit::walk_impl_item(this, impl_item); | |||
this.with_tuple_struct_self_ctor_rib(self_type, |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.
Ideally, Self
value "rib" should be added in all cases when Self
type rib is added, and not only for methods in impls.
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.
Fixed.
I'm pretty sure this implementation also loses generic arguments from impl. impl S<u8, u16> {
...
Self(x) // Will probably start inferring `T` and `U` for `S<T, U>` instead of using `u8` and `u16`
...
} |
Implementation strategy that supports type aliases and works correctly with generic parameters should likely mirror what is currently done with I'd recommend to search for |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
207b3df
to
e225596
Compare
@petrochenkov A new variant |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc_typeck/check/mod.rs
Outdated
new_def = Def::StructCtor(variant.did, variant.ctor_kind); | ||
variant.did | ||
} else { | ||
span_bug!(span, "SelfCtor is not refer to adt type") |
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.
typo 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.
Fixed.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc/hir/lowering.rs
Outdated
!self.sess.features_untracked().tuple_struct_self_ctor { | ||
emit_feature_err(&self.sess.parse_sess, "tuple_struct_self_ctor", | ||
p.span, GateIssue::Language, | ||
"tuple struct Self constructors are unstable"); |
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.
Nit: this is not about tuple structs, unit structs have Self
constructors too, so the wording could be tweaked to "Self
struct constructors are unstable" without mentioning tuples.
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 name tuple_struct_self_ctor
contains "tuple" 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.
The original feature name came from the RFC. Now it is renamed to self_struct_ctor
. And related test files are renamed.
🌲 The tree is currently closed for pull requests below priority 1, this pull request will be tested once the tree is reopened |
…rkor Implement RFC 2302: tuple_struct_self_ctor Tracking issue: #51994
💥 Test timed out |
@bors retry |
…rkor Implement RFC 2302: tuple_struct_self_ctor Tracking issue: #51994
☀️ Test successful - status-appveyor, status-travis |
📣 Toolstate changed by #53751! Tested on commit 6ff0b2e. 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). |
Tested on commit rust-lang/rust@6ff0b2e. Direct link to PR: <rust-lang/rust#53751> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).
And clippy caused RLS to fail as usual. |
Avoid panic when matching function call Fix rust-lang#55718 This bug is introduced by rust-lang#53751. The original code checked `Def::AssociatedConst(..) | Def::Method(..)` before `pat_ty.no_bound_vars().expect("expected fn type")`. But somehow I exchanged the sequence carelessly. Sorry about that. r? @petrochenkov
Tracking issue: #51994