Skip to content

Inherent static methods can be called with a non-well-formed Self-type #28848

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

Closed
arielb1 opened this issue Oct 5, 2015 · 7 comments
Closed
Labels
A-type-system Area: Type system E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.

Comments

@arielb1
Copy link
Contributor

arielb1 commented Oct 5, 2015

UPDATE: This has been fixed, but a test is still needed. See the comment below for instructions.


STR

struct Foo<'a, 'b: 'a>(&'a &'b ());

impl<'a, 'b> Foo<'a, 'b> {
    fn xmute(a: &'b ()) -> &'a () {
        unreachable!()
    }
}

pub fn foo<'a, 'b>(u: &'b ()) -> &'a () {
    Foo::<'a, 'b>::xmute(u)
}

fn main() {}

Here a static method on Foo is called with unrelated lifetimes 'a and 'b, potentially violating the 'b: 'a bound. As Foo::xmute is not allowed to assume Foo is well-formed, this does not cause a soundness issue, but is still a potential back-compat hazard.

cc @nikomatsakis

@arielb1
Copy link
Contributor Author

arielb1 commented Oct 7, 2015

The root cause is that the substs of an inherent impl don't contain an entry for Self, being only the impl substs - so Self is not checked when we check the substs.

@nikomatsakis
Copy link
Contributor

As we discussed briefly on IRC, I feel like logically the substs for an inherent impl ought to contain an entry for Self. Basically I think we ought to strive to check things "as if" there were a trait involved, even if the actual mechanism differs. Another way to look at this is that there could be an implicit "where" clause on inherent impls that Self WF (which is obviously not syntax, but which IS a notion that rustc possesses). Yet a third way is that the "path" to the inherent impl references the Self type (and must thus be WF), even if it is not part of the substitutions. I'm not quite sure which of those three perspectives I like best.

@steveklabnik steveklabnik added the A-type-system Area: Type system label Nov 11, 2015
@Mark-Simulacrum
Copy link
Member

This no longer compiles with the following error, which I think means that this is fixed. Can someone confirm?

error[E0478]: lifetime bound not satisfied
  --> /tmp/t:10:5
   |
10 |     Foo::<'a, 'b>::xmute(u)
   |     ^^^^^^^^^^^^^^^^^^^^
   |
note: lifetime parameter instantiated with the lifetime 'b as defined on the body at 9:40
  --> /tmp/t:9:41
   |
9  |   pub fn foo<'a, 'b>(u: &'b ()) -> &'a () {
   |  _________________________________________^ starting here...
10 | |     Foo::<'a, 'b>::xmute(u)
11 | | }
   | |_^ ...ending here
note: but lifetime parameter must outlive the lifetime 'a as defined on the body at 9:40
  --> /tmp/t:9:41
   |
9  |   pub fn foo<'a, 'b>(u: &'b ()) -> &'a () {
   |  _________________________________________^ starting here...
10 | |     Foo::<'a, 'b>::xmute(u)
11 | | }
   | |_^ ...ending here

error: aborting due to previous error

@nikomatsakis
Copy link
Contributor

Yes, I think this has been fixed -- and the message confirms it. Probably we should be safe and add a test though, so marking as needs-test.

@nikomatsakis nikomatsakis added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. labels Apr 18, 2017
@nikomatsakis
Copy link
Contributor

Adding a regression test is an easy way to contribute to test! Simply add a file with a name like src/test/compile-fail/issue-28848.rs that includes the example from the main comment. There are some instructions for adding compiler tests to be found here; you will need to add //~ ERROR annotations indicating the expected errors (in this case, something like //~ ERROR lifetime bound not satisfied on line 10 should work). Alternatively, you could add this as a ui test, in which case the directions can be found here.

@sirideain
Copy link
Contributor

I would be interested in giving this a try.

@Mark-Simulacrum
Copy link
Member

Go ahead! Let us know if there's anything we can do to help you out.

bors added a commit that referenced this issue May 3, 2017
Add test for Inherent static methods

Fixes #28848
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-type-system Area: Type system E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.
Projects
None yet
Development

No branches or pull requests

5 participants