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

ICE bootstrapping rustc with NLL enabled #48071

Closed
nikomatsakis opened this issue Feb 8, 2018 · 18 comments · Fixed by #52488
Closed

ICE bootstrapping rustc with NLL enabled #48071

nikomatsakis opened this issue Feb 8, 2018 · 18 comments · Fixed by #52488
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. NLL-complete Working towards the "valid code works" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

When I try to bootstrap rustc with NLL enabled, I get an odd ICE compiling libsyntax_ext:

error: internal compiler error: broken MIR in NodeId(16117) (_368 = move _369): bad assignment (for<'cx, 'r, 's> fn(&'cx mut syntax::ext::base::ExtCtxt<'r>, syntax_pos::Span, &'s [syntax::tokenstream::To\
kenTree]) -> std::boxed::Box<syntax::ext::base::MacResult + 'cx> = for<'r, 's, 't0> fn(&'r mut syntax::ext::base::ExtCtxt<'s>, syntax_pos::Span, &'t0 [syntax::tokenstream::TokenTree]) -> std::boxed::Box<\
syntax::ext::base::MacResult>): RegionsOverlyPolymorphic(BrNamed(crate11:DefIndex(1:1012), 'cx), '_#729r)
   --> src/libsyntax_ext/lib.rs:65:44
    |
65  |                           expander: Box::new($f as MacroExpanderFn),
    |                                              ^^^^^^^^^^^^^^^^^^^^^
...
92  | /     register! {
93  | |         line: expand_line,
94  | |         __rust_unstable_column: expand_column_gated,
95  | |         column: expand_column,
...   |
112 | |         compile_error: compile_error::expand_compile_error,
113 | |     }
    | |_____- in this macro invocation
@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels Feb 8, 2018
@nikomatsakis nikomatsakis added this to the NLL: Valid code works milestone Feb 8, 2018
@Aaron1011
Copy link
Member

I'd like to work on this.

@nikomatsakis
Copy link
Contributor Author

@Aaron1011 any progress? want me to take a look at all?

@Aaron1011
Copy link
Member

@nikomatsakis: I haven't yet been able to reproduce this, due to #48129 stopping compilation. I'll try to work around that issue locally, and see if I can reproduce + investigate the original issue.

@Aaron1011
Copy link
Member

@nikomatsakis: I ran across another issue while trying to reach the ICE: #48224

@Aaron1011
Copy link
Member

@nikomatsakis: Until the intermediate issues are resolved, I don't think I'll be able to reproduce this.

@nikomatsakis
Copy link
Contributor Author

@Aaron1011 this branch #48372 clears the way

@nikomatsakis
Copy link
Contributor Author

Hmm, but on that branch I am now encountering the AddAssign problems I guess. So we're still blocked on #48129

@nikomatsakis
Copy link
Contributor Author

Actually, the issue-48071 branch on my repo reproduces the problem. You have to build like so:

RUSTFLAGS_STAGE_NOT_0='-Znll -Zborrowck=mir -Ztwo-phase-borrows' ./x.py build --stage 2 

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Feb 21, 2018

Error that I get is (lightly edited):

error: internal compiler error: broken MIR in NodeId(16117) (_368 = move _369): bad assignment (

for<'cx, 'r, 's> fn(&'cx mut syntax::ext::base::ExtCtxt<'r>, syntax_pos::Span, &'s [syntax::tokenstream::TokenTree]) -> std::boxed::Box<syntax::ext::base::MacResult + 'cx>
for<'r, 's, 't0> fn(&'r mut syntax::ext::base::ExtCtxt<'s>, syntax_pos::Span, &'t0 [syntax::tokenstream::TokenTree]) -> std::boxed::Box<syntax::ext::base::MacResult>): 

RegionsOverlyPolymorphic(BrNamed(crate11:DefIndex(1:1012), 'cx), '_#729r)

Removing explicit paths:

for<'cx, 'r, 's> fn(&'cx mut ExtCtxt<'r>, Span, &'s [TokenTree]) -> Box<MacResult + 'cx>
for<'r, 's, 't0> fn(&'r mut ExtCtxt<'s>, Span, &'t0 [TokenTree]) -> Box<MacResult>)

The problem seems to be the return type.

@nikomatsakis
Copy link
Contributor Author

Minimized:

#![allow(warnings)]
#![feature(dyn_trait)]
#![feature(nll)]

trait Foo {
    fn foo(&self) { }
}

impl Foo for () {
}

type MakeFooFn = for<'a> fn(&'a u8) -> Box<dyn Foo + 'a>;

fn make_foo(x: &u8) -> Box<dyn Foo + 'static> {
    Box::new(())
}

fn main() {
    let x: MakeFooFn = make_foo as MakeFooFn;
}

@nikomatsakis
Copy link
Contributor Author

I think the problem is that there is a missing coercion of sorts, or else that this code in the NLL type checker is too strict:

CastKind::ReifyFnPointer => {
let fn_sig = op.ty(mir, tcx).fn_sig(tcx);
// The type that we see in the fcx is like
// `foo::<'a, 'b>`, where `foo` is the path to a
// function definition. When we extract the
// signature, it comes from the `fn_sig` query,
// and hence may contain unnormalized results.
let fn_sig = self.normalize(&fn_sig, location);
let ty_fn_ptr_from = tcx.mk_fn_ptr(fn_sig);
if let Err(terr) = self.eq_types(ty_fn_ptr_from, ty, location.at_self()) {
span_mirbug!(
self,
rvalue,
"equating {:?} with {:?} yields {:?}",
ty_fn_ptr_from,
ty,
terr
);
}
}

In particular, it constructs the "reified" form of the fn pointer in ty_fn_ptr_from and then requires that the target type be equal to that. But in this case, the target type is a subtype of that.

@nikomatsakis
Copy link
Contributor Author

(Hmm, I have a vague memory of solving a similar problem somewhere else, where we were rolling multiple coercions into one. It's sort of debatable what's right here, I suppose -- probably we should change the NLL type checker to use sub_types.)

@nikomatsakis
Copy link
Contributor Author

OK well that proposed change didn't actually fix anything =)

@nikomatsakis
Copy link
Contributor Author

Oh, interesting. The problem, I believe, is #33684 -- which @sgrif and I have been working on a PR to fix, independently.

@nikomatsakis nikomatsakis added the NLL-complete Working towards the "valid code works" goal label Mar 14, 2018
@nikomatsakis nikomatsakis added NLL-deferred and removed NLL-complete Working towards the "valid code works" goal labels Apr 3, 2018
@nikomatsakis
Copy link
Contributor Author

Tagging as NLL-deferred -- this is blocked on @sgrif's work at the moment and there exists a workaround for within rustc.

@nikomatsakis
Copy link
Contributor Author

Hmm. I'm going to mark this as high priority because it blocks bootstrap. I think it's a bit tricky to fix. While bike riding recently though I had some ideas here -- I realized that we could probably write a custom bit of subtyping code used only (initially) by the MIR type checker. The hope would be that this is faster than canonicalizing etc. This code could use a universe-based check. This would sidestep the limitations of our existing subtyping algorithm which are biting us here. It would also help eliminate a blocker for chalk integration later. I may take a stab at this.

@nikomatsakis nikomatsakis self-assigned this Jun 29, 2018
@nikomatsakis nikomatsakis added NLL-complete Working towards the "valid code works" goal and removed NLL-deferred labels Jul 6, 2018
@nikomatsakis
Copy link
Contributor Author

OK, my branch is now working and passing tests. I want to clean it up some and perhaps try to improve its performance though.

@pnkfelix
Copy link
Member

visited for triage. this should be fixed via PR #52488 (which is WIP but hopefully will be ready soon).

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jul 25, 2018
bors added a commit that referenced this issue Jul 25, 2018
… r=<try>

introduce universes to NLL type check

This branch aims to fix #48071 and also advance chalk integration a bit at the same time. It re-implements the subtyping/type-equating check so that NLL doesn't "piggy back" on the subtyping code of the old type checker.

This new code uses the "universe-based" approach to handling higher-ranked lifetimes, which sidesteps some of the limitations of the current "leak-based" scheme. This avoids the ICE in #48071.

At the same time, I aim for this to potentially be a kind of optimization. This NLL code is (currently) not cached, but it also generates constraints without doing as much instantiation, substitution, and folding. Right now, though, it still piggy backs on the `relate_tys` trait, which is a bit unfortunate -- it means we are doing more hashing and things than we have to. I want to measure the see the perf. Refactoring that trait is something I'd prefer to leave for follow-up work.

r? @pnkfelix -- but I want to measure perf etc first
bors added a commit that referenced this issue Jul 25, 2018
… r=<try>

introduce universes to NLL type check

This branch aims to fix #48071 and also advance chalk integration a bit at the same time. It re-implements the subtyping/type-equating check so that NLL doesn't "piggy back" on the subtyping code of the old type checker.

This new code uses the "universe-based" approach to handling higher-ranked lifetimes, which sidesteps some of the limitations of the current "leak-based" scheme. This avoids the ICE in #48071.

At the same time, I aim for this to potentially be a kind of optimization. This NLL code is (currently) not cached, but it also generates constraints without doing as much instantiation, substitution, and folding. Right now, though, it still piggy backs on the `relate_tys` trait, which is a bit unfortunate -- it means we are doing more hashing and things than we have to. I want to measure the see the perf. Refactoring that trait is something I'd prefer to leave for follow-up work.

r? @pnkfelix -- but I want to measure perf etc first
bors added a commit that referenced this issue Jul 25, 2018
… r=<try>

introduce universes to NLL type check

This branch aims to fix #48071 and also advance chalk integration a bit at the same time. It re-implements the subtyping/type-equating check so that NLL doesn't "piggy back" on the subtyping code of the old type checker.

This new code uses the "universe-based" approach to handling higher-ranked lifetimes, which sidesteps some of the limitations of the current "leak-based" scheme. This avoids the ICE in #48071.

At the same time, I aim for this to potentially be a kind of optimization. This NLL code is (currently) not cached, but it also generates constraints without doing as much instantiation, substitution, and folding. Right now, though, it still piggy backs on the `relate_tys` trait, which is a bit unfortunate -- it means we are doing more hashing and things than we have to. I want to measure the see the perf. Refactoring that trait is something I'd prefer to leave for follow-up work.

r? @pnkfelix -- but I want to measure perf etc first
bors added a commit that referenced this issue Jul 26, 2018
… r=pnkfelix

introduce universes to NLL type check

This branch aims to fix #48071 and also advance chalk integration a bit at the same time. It re-implements the subtyping/type-equating check so that NLL doesn't "piggy back" on the subtyping code of the old type checker.

This new code uses the "universe-based" approach to handling higher-ranked lifetimes, which sidesteps some of the limitations of the current "leak-based" scheme. This avoids the ICE in #48071.

At the same time, I aim for this to potentially be a kind of optimization. This NLL code is (currently) not cached, but it also generates constraints without doing as much instantiation, substitution, and folding. Right now, though, it still piggy backs on the `relate_tys` trait, which is a bit unfortunate -- it means we are doing more hashing and things than we have to. I want to measure the see the perf. Refactoring that trait is something I'd prefer to leave for follow-up work.

r? @pnkfelix -- but I want to measure perf etc first
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. NLL-complete Working towards the "valid code works" goal 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.

4 participants