Skip to content

traits with self-containing supertraits are not object safe #38603

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Jan 20, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Dec 25, 2016

This should be the last time I fix this function.

Fixes #38404.

@rust-highfive
Copy link
Contributor

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 25, 2016

This fixes the ICE, but we still permit stuff such as

trait Q<T:?Sized> {}
trait Foo where u32: Q<Self> {}

fn main() {
    let f: Box<Foo> = ...;
}

That shouldn't cause an ICE because non-supertrait bounds are never used, but I still think I should future-compatibility-warning this.

@arielb1 arielb1 added I-needs-decision Issue: In need of a decision. and removed I-needs-decision Issue: In need of a decision. labels Dec 25, 2016
@eddyb eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Dec 25, 2016
@eddyb
Copy link
Member

eddyb commented Dec 25, 2016

cc @rust-lang/compiler @brson We should probably gate this on Crater.

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 25, 2016

The current patch is pretty non-aggressive, prohibiting only cases that tend to ICE. I think we want to crater a more aggressive version of it (cc #38604).

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 31, 2016

now ready for crater

@nikomatsakis
Copy link
Contributor

cc @brson -- can you run this through craterbomb?

@brson
Copy link
Contributor

brson commented Jan 3, 2017

I'll run it through crater. Cargobomb isn't quite ready for this yet.

@brson
Copy link
Contributor

brson commented Jan 4, 2017

Still cratering.

@brson
Copy link
Contributor

brson commented Jan 6, 2017

Crater says https://gist.github.com/anonymous/3409c01561daa0670445c1ac9f967f6b

Some false positives but some legit errors.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 9, 2017

Root regressions from @brson's run (I haven't analyzed them yet, hence the unchecked check boxes). We should try to and submit fixes or at least contact authors of affected crates and so forth.

@arielb1
Copy link
Contributor Author

arielb1 commented Jan 10, 2017

I am not sure what's behind the rmt_serialize errors, but resolution finishes before my new code runs so it shouldn't be me.

All other errors look spurious.

@nikomatsakis
Copy link
Contributor

@arielb1

I am not sure what's behind the rmt_serialize errors, but resolution finishes before my new code runs so it shouldn't be me.

That is odd. Do you have a local build? Can you give this a spin locally?

Ariel Ben-Yehuda and others added 2 commits January 11, 2017 00:02
@arielb1
Copy link
Contributor Author

arielb1 commented Jan 10, 2017

Looks like not our bug.

Compiling using the Cargo.lock in the repository works and uses the following versions:

   Compiling byteorder v0.3.13
   Compiling libc v0.2.8
   Compiling rustc-serialize v0.3.18
   Compiling rmp v0.7.3
   Compiling log v0.3.5
   Compiling rmp-serialize v0.7.0
   Compiling neovim-lib v0.1.1 (file:///home/ariel/Rust/neovim-lib)

While after a cargo update, or on crater, the following versions are used, triggering the error with both nightly and modified rustc:

   Compiling byteorder v1.0.0
   Compiling byteorder v0.4.2
   Compiling num-traits v0.1.36
   Compiling log v0.3.6
   Compiling rustc-serialize v0.3.22
   Compiling rmp v0.7.5
   Compiling rmp v0.8.1
   Compiling rmp-serialize v0.7.0

@nikomatsakis
Copy link
Contributor

Looks like not our bug.

So you get the same results both with and without this PR?

Since this is being used as a library, the Cargo.lock that is found in the repo isn't meaningful anyway. Cargo.lock is only respected for the final target, iiuc.

@arielb1
Copy link
Contributor Author

arielb1 commented Jan 14, 2017

@nikomatsakis

Exactly. When you use the Cargo.lock from source control, it works with both the old and new rustcs, and when you cargo update, it does not work.

@nikomatsakis
Copy link
Contributor

@arielb1 I'm sort of confused by what you meant when you wrote "not our bug"

@arielb1
Copy link
Contributor Author

arielb1 commented Jan 18, 2017

@nikomatsakis

That's it's broken, but not by our changes.

@nikomatsakis
Copy link
Contributor

OK, I misread this comment at first:

When you use the Cargo.lock from source control, it works with both the old and new rustcs, and when you cargo update, it does not work.

Interesting.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 19, 2017

📌 Commit c85fb19 has been approved by nikomatsakis

@bors
Copy link
Collaborator

bors commented Jan 19, 2017

⌛ Testing commit c85fb19 with merge 7da72d5...

@bors
Copy link
Collaborator

bors commented Jan 20, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

alexcrichton commented Jan 20, 2017 via email

@bors
Copy link
Collaborator

bors commented Jan 20, 2017

⌛ Testing commit c85fb19 with merge 8fa0690...

@bors
Copy link
Collaborator

bors commented Jan 20, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

alexcrichton commented Jan 20, 2017 via email

@bors
Copy link
Collaborator

bors commented Jan 20, 2017

⌛ Testing commit c85fb19 with merge 6be85b4...

@bors
Copy link
Collaborator

bors commented Jan 20, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

alexcrichton commented Jan 20, 2017 via email

@bors
Copy link
Collaborator

bors commented Jan 20, 2017

⌛ Testing commit c85fb19 with merge d38430b...

@alexcrichton
Copy link
Member

@bors: retry

  • prioritizing rollup

@bors
Copy link
Collaborator

bors commented Jan 20, 2017

⌛ Testing commit c85fb19 with merge 5a6ac84...

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Collaborator

bors commented Jan 20, 2017

⌛ Testing commit c85fb19 with merge b53d374...

bors added a commit that referenced this pull request Jan 20, 2017
traits with self-containing supertraits are not object safe

This should be the last time I fix this function.

Fixes #38404.
@bors
Copy link
Collaborator

bors commented Jan 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing b53d374 to master...

@bors bors merged commit c85fb19 into rust-lang:master Jan 20, 2017
bors added a commit that referenced this pull request Jan 20, 2017
bors added a commit that referenced this pull request Jan 21, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-crater Status: Waiting on a crater run to be completed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 line ICE on stable and nightly (Unexpected type in full type resolver)
7 participants