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

solve ICE of issue #85350 by avoiding the case with ty::Error #85477

Closed
wants to merge 1 commit into from

Conversation

ABouttefeux
Copy link
Contributor

fix #85350


avoids looking for bounds in the case of supertrait on ty::Error

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2021
@jackh726
Copy link
Member

jackh726 commented May 19, 2021

r? @jackh726

@jackh726 jackh726 assigned jackh726 and unassigned varkor May 19, 2021
@rust-log-analyzer

This comment has been minimized.

@ABouttefeux
Copy link
Contributor Author

I just want to check something, maybe there is a better way to fix it. Anyway I will need to move the test for the CI to pass.

@ABouttefeux ABouttefeux marked this pull request as draft May 19, 2021 16:39
@jackh726
Copy link
Member

Hmm. So this might be one way to solve this. What I was sort of expecting was something around when we resolve the function elision (because we follow function elision rules in FnMut(&Context)) we just add that anonymous region to the impl lifetimes. But that doesn't make sense now that I think about it, because impl lifetimes are early bound and function lifetimes are late bound

@jackh726
Copy link
Member

Ah but I guess its treating that FnMut as a trait object. So we could add the lifetimes to that.

@jackh726
Copy link
Member

Interestingly, adding a dyn to the original repro doesn't ICE anymore. So there's some mismatch somewhere

@ABouttefeux
Copy link
Contributor Author

ABouttefeux commented May 19, 2021

Ok thanks 😃

@ABouttefeux
Copy link
Contributor Author

ABouttefeux commented May 20, 2021

After a bit of digging I think E0229 is erroneously emitted

error[E0229]: associated type bindings are not allowed here
 --> issue-85350.rs:3:6
  |
3 | impl FnMut(&Context) for 'tcx {
  |      ^^^^^^^^^^^^^^^ associated type not allowed here

Where as

impl FnMut(&dyn Context) for 'tcx {
    fn print () -> Self::Output{ }
}

does not emit this error code ( and does not ICE)


rustc --explain E0229:

An associated type binding was done outside of the type parameter declaration
and where clause.

Erroneous code example:

pub trait Foo {
    type A;
    fn boo(&self) -> <Self as Foo>::A;
}

struct Bar;

impl Foo for isize {
    type A = usize;
    fn boo(&self) -> usize { 42 }
}

fn baz<I>(x: &<I as Foo<A=Bar>>::A) {}
// error: associated type bindings are not allowed here

To solve this error, please move the type bindings in the type parameter
declaration:

fn baz<I: Foo<A=Bar>>(x: &<I as Foo>::A) {} // ok!

Or in the where clause:

fn baz<I>(x: &<I as Foo>::A) where I: Foo<A=Bar> {}

@ABouttefeux
Copy link
Contributor Author

Interestingly

impl FnMut(&'tcx Context) for 'tcx {
    fn print () -> Self::Output{ }
} 

does not ICE and emits E0229 so maybe it is something else

@jackh726
Copy link
Member

FnMut(&'tcx Context) there should desugar to FnMut<(&'tcx Context, ), Output=()>; so, I think this makes sense:
the impl should be:

impl<'a> FnMut<(&'a Context,)> for 'tcx {
    type Output = ();
    fn print () -> Self::Output{ }
}

This really is a "it's really not clear what we should be doing because the syntax is quite wrong" situation. So it's hard to figure out what the user intended. So, a few options:

  1. Try to bail out before lowering at all
  2. Treat the FnMut(...) "normally" (i.e. as an elision context) somehow; this is difficult because the elided lifetimes are late bound, so I'm not sure where we would add that in terms of bound vars
  3. Make elision here not allowed (i.e. fall back to 'static)
  4. Treat the FnMut(...) as a trait object and add the elided lifetime to the trait object's trait ref

@ABouttefeux
Copy link
Contributor Author

I am a bit stuck. I am do not find where the lowering precisely happens for FnMut implementation precisely or where the bound region are added for elided lifetime.

I do not really see how to do 3 or 4 yet.
and for 1 I am not sure at which point I should bail out.

@jackh726
Copy link
Member

I am a bit stuck. I am do not find where the lowering precisely happens for FnMut implementation precisely or where the bound region are added for elided lifetime.

I do not really see how to do 3 or 4 yet.
and for 1 I am not sure at which point I should bail out.

@ABouttefeux let me think about this and try to write some notes this weekend

@ABouttefeux
Copy link
Contributor Author

@jackh726 any updates ? Take the time you need 😄 . Lately I did not had a lot of time anyway.

@jackh726
Copy link
Member

jackh726 commented Jun 1, 2021

I haven't been able to get to this :/

@jackh726
Copy link
Member

@ABouttefeux this is honestly kind of an edge case; I'm okay with just landing this as-is,

@jackh726
Copy link
Member

That being said...the test location isn't correct. And I'm not quite sure the best place for it. Maybe just under src/test/ui/issues as issue-85350-anon-lifetimes-in-malformed-impl.rs or something like that.

@bors delegate+

just @bors r=jackh726 when test is moved if you're okay with this fix

@bors
Copy link
Contributor

bors commented Jun 24, 2021

✌️ @ABouttefeux can now approve this pull request

@bors
Copy link
Contributor

bors commented Jun 24, 2021

📌 Commit 42f7764 has been approved by `jackh726``

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2021
@jackh726
Copy link
Member

@bors r- looks like bors picked that up...

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 24, 2021
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 24, 2021
@Alexendoo

This comment has been minimized.

@jackh726 jackh726 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 18, 2021
@bors
Copy link
Contributor

bors commented Feb 14, 2022

☔ The latest upstream changes (presumably #93938) made this pull request unmergeable. Please resolve the merge conflicts.

@jackh726
Copy link
Member

jackh726 commented Mar 1, 2022

I'm going to go ahead and close this as inactive.

@jackh726 jackh726 closed this Mar 1, 2022
@jackh726 jackh726 added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants