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

beta regression: assembunny-plus 0.0.3 #40951

Closed
alexcrichton opened this Issue Mar 31, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@alexcrichton
Copy link
Member

alexcrichton commented Mar 31, 2017

The assembunny-plus crate has regressed on beta:

   Compiling assembunny_plus v0.0.3 (file:///home/alex/code/assembunny-plus)
error: borrowed value does not live long enough
   --> src/parser.rs:309:98
    |
309 |     let mut output: Vec<Token> = vec![Token::new(TokenType::KEYWORD, index_of(&KEYWORD_INDEX, &&*str_toks[0].to_lowercase()).unwrap() as i32)];
    |                                                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^                   - temporary value only lives until here
    |                                                                                                  |
    |                                                                                                  does not live long enough
    |
    = note: borrowed value must be valid for the static lifetime...
    = note: consider using a `let` binding to increase its lifetime

error: aborting due to previous error

error: Could not compile `assembunny_plus`.

To learn more, run the command again with --verbose.

cc @broad-well

broad-well/assembunny-plus@4ed0f0d

rustc 1.17.0-beta.2 (b7c276653 2017-03-20)
binary: rustc
commit-hash: b7c27665307704a9b158fe242e88e83914029415
commit-date: 2017-03-20
host: x86_64-unknown-linux-gnu
release: 1.17.0-beta.2
LLVM version: 3.9
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 4, 2017

OK, I did some digging around. I believe this regression is a bug. Here is a minimal test case:

const FOO: [&'static str; 1] = ["foo"];

fn find<T: PartialEq>(t: &[T], element: &T) { }

fn main() {
    let x = format!("hi");
    find(&FOO, &&*x);
}

Specifically what is happening is that we used to infer the T in find to &'x str for some 'x, but we now infer it to &'static str. I suspect this is related to the soundness fix around unsized coercions. You can make the code compile in various ways, e.g. find::<&str> or &FOO[..], all of which kind of sidestep that aspect of type inference.

cc @eddyb

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 4, 2017

(Nominating for compiler team triage -- I can't attend mtg this week, but suggest P-high.)

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 4, 2017

There should be enough subtyping to make that work, I'm not sure where it goes wrong. It's broken on nightly too, right?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 4, 2017

This is actually #18653. I will see about fixing that.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 6, 2017

I'm going to mark this as P-high and assign myself. I am investigating.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 6, 2017

triage: P-high

@rust-highfive rust-highfive added P-high and removed I-nominated labels Apr 6, 2017

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Apr 6, 2017

assigning niko to hold him to his earlier comment. 😄

@alexcrichton alexcrichton added this to the 1.17 milestone Apr 10, 2017

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Apr 11, 2017

Backports are best in by this friday, next thursday is the absolute deadline. cc @nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 11, 2017

@brson working on it :)

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Apr 11, 2017

generalize type variables too
When we are generalizing a super/sub-type, we have to replace type
variables with a fresh variable (and not just region variables).  So if
we know that `Box<?T> <: ?U`, for example, we instantiate `?U` with
`Box<?V>` and then relate `Box<?T>` to `Box<?V>` (and hence require that
`?T <: ?V`).

This change has some complex interactions, however:

First, the occurs check must be updated to detect constraints like `?T
<: ?U` and `?U <: Box<?T>`. If we're not careful, we'll create a
never-ending sequence of new variables. To address this, we add a second
unification set into `type_variables` that tracks type variables related
through **either** equality **or** subtyping, and use that during the
occurs-check.

Second, the "fudge regions if ok" code was expecting no new type
variables to be created. It must be updated to create new type variables
outside of the probe. This is relatively straight-forward under the new
scheme, since type variables are now independent from one another, and
any relations are moderated by pending subtype obliations and so forth.
This part would be tricky to backport though.

cc rust-lang#18653
cc rust-lang#40951

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Apr 11, 2017

generalize type variables too
When we are generalizing a super/sub-type, we have to replace type
variables with a fresh variable (and not just region variables).  So if
we know that `Box<?T> <: ?U`, for example, we instantiate `?U` with
`Box<?V>` and then relate `Box<?T>` to `Box<?V>` (and hence require that
`?T <: ?V`).

This change has some complex interactions, however:

First, the occurs check must be updated to detect constraints like `?T
<: ?U` and `?U <: Box<?T>`. If we're not careful, we'll create a
never-ending sequence of new variables. To address this, we add a second
unification set into `type_variables` that tracks type variables related
through **either** equality **or** subtyping, and use that during the
occurs-check.

Second, the "fudge regions if ok" code was expecting no new type
variables to be created. It must be updated to create new type variables
outside of the probe. This is relatively straight-forward under the new
scheme, since type variables are now independent from one another, and
any relations are moderated by pending subtype obliations and so forth.
This part would be tricky to backport though.

cc rust-lang#18653
cc rust-lang#40951

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Apr 12, 2017

generalize type variables too
When we are generalizing a super/sub-type, we have to replace type
variables with a fresh variable (and not just region variables).  So if
we know that `Box<?T> <: ?U`, for example, we instantiate `?U` with
`Box<?V>` and then relate `Box<?T>` to `Box<?V>` (and hence require that
`?T <: ?V`).

This change has some complex interactions, however:

First, the occurs check must be updated to detect constraints like `?T
<: ?U` and `?U <: Box<?T>`. If we're not careful, we'll create a
never-ending sequence of new variables. To address this, we add a second
unification set into `type_variables` that tracks type variables related
through **either** equality **or** subtyping, and use that during the
occurs-check.

Second, the "fudge regions if ok" code was expecting no new type
variables to be created. It must be updated to create new type variables
outside of the probe. This is relatively straight-forward under the new
scheme, since type variables are now independent from one another, and
any relations are moderated by pending subtype obliations and so forth.
This part would be tricky to backport though.

cc rust-lang#18653
cc rust-lang#40951

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Apr 12, 2017

generalize type variables too
When we are generalizing a super/sub-type, we have to replace type
variables with a fresh variable (and not just region variables).  So if
we know that `Box<?T> <: ?U`, for example, we instantiate `?U` with
`Box<?V>` and then relate `Box<?T>` to `Box<?V>` (and hence require that
`?T <: ?V`).

This change has some complex interactions, however:

First, the occurs check must be updated to detect constraints like `?T
<: ?U` and `?U <: Box<?T>`. If we're not careful, we'll create a
never-ending sequence of new variables. To address this, we add a second
unification set into `type_variables` that tracks type variables related
through **either** equality **or** subtyping, and use that during the
occurs-check.

Second, the "fudge regions if ok" code was expecting no new type
variables to be created. It must be updated to create new type variables
outside of the probe. This is relatively straight-forward under the new
scheme, since type variables are now independent from one another, and
any relations are moderated by pending subtype obliations and so forth.
This part would be tricky to backport though.

cc rust-lang#18653
cc rust-lang#40951
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 12, 2017

OK, this is fixed be #40570, but I think I also have a more tailored fix that we can backport to beta.

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Apr 13, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 13, 2017

Status:

  • Fixed on nightly through #40570
  • Pending PR for beta (#41269) but it is unclear if it makes sense to backport, since the changes are non-trivial, and the impact is not huge. (One crate that I am aware of.)
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 13, 2017

After discussion in the @rust-lang/compiler meeting, we decided not to backport the fix for this to beta, as it is non-trivial. The problem is fixed on nightly, and I'll offer a patch to the assembunny crate.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Apr 20, 2017

Looks fixed on nightly in assembunny-plus as well, and due to decision to not backport closing.

Thanks for the fixes @nikomatsakis!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.