Skip to content
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

With a generic struct, Self(x) works where x is of the wrong type argument #61882

Closed
Centril opened this issue Jun 16, 2019 · 5 comments · Fixed by #61896
Closed

With a generic struct, Self(x) works where x is of the wrong type argument #61882

Centril opened this issue Jun 16, 2019 · 5 comments · Fixed by #61896
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Jun 16, 2019

In particular, we can write:

struct A<T>(T);

impl A<bool> {
    const B: A<u8> = Self(0); // OK but shouldn't be?

    const C: A<u8> = Self { 0: 0 }; // Correctly rejected.
}

This can lead to the following interesting situation:

struct A<T>(T);

impl A<bool> {
    const B: Self = Self(0); // Rejected, so `Self(..)` is not of type `Self`?
}

cc @petrochenkov, @varkor, @eddyb, and @alexreg

This does seem like a bug to me... In particular, type_alias_enum_variants does not allow Self::Some(transform(x)) where Self == Option<T> but where Option<U> is expected.

cc @rust-lang/lang

@Centril Centril added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Jun 16, 2019
@petrochenkov
Copy link
Contributor

Yes, that's a bug, Self in the example should be A<bool> and A::<bool>(0) should report an error.

We missed a test for this in #53751 somehow (for Self types this property is tested in struct-path-self-type-mismatch.rs).

@petrochenkov
Copy link
Contributor

Self constructors were stabilized not too long ago, so this should preferably be fixed soon and backported to beta.

@eddyb
Copy link
Member

eddyb commented Jun 16, 2019

Ouch, this explains why we were able to remove the Ty result from rewrite_self_ctor as redundant in #61085 / #61189 - there was a subst call missing on that type.
But that's not the end of it, this is incorrectly allowed right now, as well:

struct A<T>(T);

impl A<bool> {
    const B: A<u8> = Self::<u8>(0);
}

So the order here is wrong:

let res = match self.rewrite_self_ctor(res, span) {
Ok(res) => res,
Err(ErrorReported) => return (tcx.types.err, res),
};
let path_segs = match res {
Res::Local(_) => vec![],
Res::Def(kind, def_id) =>
AstConv::def_ids_for_value_path_segments(self, segments, self_ty, kind, def_id),
_ => bug!("instantiate_value_path on {:?}", res),
};

In that Res::SelfCtor really needs to be treated like Res::Local.

In general, I think instead of rewrite_self_ctor we need a method that only returns the right information (such as the modified Res and substituted Ty) given the impl DefId inside Res::SelfCtor.
And every call should be audited.
EDIT: there's only one call now, huh, so it can be inlined.
I guess I could prepare a master and beta PR?

@eddyb
Copy link
Member

eddyb commented Jun 16, 2019

We shouldn't forget tests like A<&'static T> being created with Self(non_static_ref).

@eddyb
Copy link
Member

eddyb commented Jun 16, 2019

Looks like the test added in #59025 for #57924 has an explicit generic arg: Self::<E>(e), but somehow @varkor and I didn't spot that during the review, whoops.

@Centril Centril removed I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jun 16, 2019
Centril added a commit to Centril/rust that referenced this issue Jun 18, 2019
…nkov

rustc_typeck: correctly compute `Substs` for `Res::SelfCtor`.

Fixes rust-lang#61882.

r? @petrochenkov cc @varkor
Centril added a commit to Centril/rust that referenced this issue Jun 18, 2019
…nkov

rustc_typeck: correctly compute `Substs` for `Res::SelfCtor`.

Fixes rust-lang#61882.

r? @petrochenkov cc @varkor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants