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

Make UnsafeCell::into_inner safe #47204

Merged
merged 1 commit into from Jan 28, 2018

Conversation

Projects
None yet
@varkor
Contributor

varkor commented Jan 5, 2018

This fixes #35067. It will require a Crater run as discussed in that
issue.

Make UnsafeCell::into_inner safe
This fixes #35067. It will require a Crater run as discussed in that
issue.
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jan 5, 2018

Collaborator

r? @Mark-Simulacrum

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

Collaborator

rust-highfive commented Jan 5, 2018

r? @Mark-Simulacrum

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

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum Jan 5, 2018

Member

r? @BurntSushi

@bors try

cc @aidanhs for crater

Member

Mark-Simulacrum commented Jan 5, 2018

r? @BurntSushi

@bors try

cc @aidanhs for crater

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 5, 2018

Contributor

⌛️ Trying commit 4829d50 with merge 68950fc...

Contributor

bors commented Jan 5, 2018

⌛️ Trying commit 4829d50 with merge 68950fc...

bors added a commit that referenced this pull request Jan 5, 2018

Auto merge of #47204 - varkor:unsafecell-into_inner-safe, r=<try>
Make UnsafeCell::into_inner safe

This fixes #35067. It will require a Crater run as discussed in that
issue.
@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Jan 5, 2018

Member

This shouldn't need a crater anymore since this works now:

let x: unsafe fn(_) -> _ = UnsafeCell::<u32>::into_inner;

I added that conversion way back in #37389.

Member

cramertj commented Jan 5, 2018

This shouldn't need a crater anymore since this works now:

let x: unsafe fn(_) -> _ = UnsafeCell::<u32>::into_inner;

I added that conversion way back in #37389.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 5, 2018

Contributor

💔 Test failed - status-travis

Contributor

bors commented Jan 5, 2018

💔 Test failed - status-travis

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 5, 2018

Contributor

It still technically needs a crater run. There are other ways to expose the type. e.g. you might have a trait implemented only for unsafe fn() and so forth.

Contributor

nikomatsakis commented Jan 5, 2018

It still technically needs a crater run. There are other ways to expose the type. e.g. you might have a trait implemented only for unsafe fn() and so forth.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Jan 5, 2018

Member

@nikomatsakis Even then you'd still have to trigger the coercion to unsafe fn() first, wouldn't you? For example:

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

impl Foo for unsafe fn() {}

unsafe fn baz() {}

fn main() {
    // These do not work:
    baz.bar();
    Foo::bar(&baz);

    // These do:
    let x: unsafe fn() = baz;
    x.bar();
    Foo::bar(&x);
}
Member

cramertj commented Jan 5, 2018

@nikomatsakis Even then you'd still have to trigger the coercion to unsafe fn() first, wouldn't you? For example:

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

impl Foo for unsafe fn() {}

unsafe fn baz() {}

fn main() {
    // These do not work:
    baz.bar();
    Foo::bar(&baz);

    // These do:
    let x: unsafe fn() = baz;
    x.bar();
    Foo::bar(&x);
}
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 5, 2018

Contributor

@cramertj ah, hmm, good point. You may be right. =)

Contributor

nikomatsakis commented Jan 5, 2018

@cramertj ah, hmm, good point. You may be right. =)

@carols10cents

This comment has been minimized.

Show comment
Hide comment
@carols10cents

carols10cents Jan 9, 2018

Member

errrrr try failed because of... cargo? I'm going to see if retrying works...

@bors try

Member

carols10cents commented Jan 9, 2018

errrrr try failed because of... cargo? I'm going to see if retrying works...

@bors try

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 10, 2018

Member

@bors clean retry try

Member

kennytm commented Jan 10, 2018

@bors clean retry try

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 10, 2018

Contributor

⌛️ Trying commit 4829d50 with merge 15e0ec4...

Contributor

bors commented Jan 10, 2018

⌛️ Trying commit 4829d50 with merge 15e0ec4...

bors added a commit that referenced this pull request Jan 10, 2018

Auto merge of #47204 - varkor:unsafecell-into_inner-safe, r=<try>
Make UnsafeCell::into_inner safe

This fixes #35067. It will require a Crater run as discussed in that
issue.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 10, 2018

Contributor

💔 Test failed - status-travis

Contributor

bors commented Jan 10, 2018

💔 Test failed - status-travis

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 10, 2018

Member

Nope doesn't work. Cargo needs to add #[allow(unused_unsafe)] for their LazyCell.

Member

kennytm commented Jan 10, 2018

Nope doesn't work. Cargo needs to add #[allow(unused_unsafe)] for their LazyCell.

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum Jan 10, 2018

Member

This is a breaking change because of the deny(warnings) resulting the compilation failure, though it is minor. We have done this in the past, though (most recently with the ASCII extension traits). @rust-lang/libs: Do we want to do this considering the breaking change aspect?

Member

Mark-Simulacrum commented Jan 10, 2018

This is a breaking change because of the deny(warnings) resulting the compilation failure, though it is minor. We have done this in the past, though (most recently with the ASCII extension traits). @rust-lang/libs: Do we want to do this considering the breaking change aspect?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 10, 2018

Member

@rfcbot fcp merge

Curious to see what the libs team thinks!

Member

alexcrichton commented Jan 10, 2018

@rfcbot fcp merge

Curious to see what the libs team thinks!

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Jan 10, 2018

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Jan 10, 2018

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Jan 23, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Jan 23, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton
Member

alexcrichton commented Jan 23, 2018

@bors: r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 23, 2018

Contributor

📌 Commit 4829d50 has been approved by alexcrichton

Contributor

bors commented Jan 23, 2018

📌 Commit 4829d50 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 24, 2018

Contributor

⌛️ Testing commit 4829d50 with merge ea2b106...

Contributor

bors commented Jan 24, 2018

⌛️ Testing commit 4829d50 with merge ea2b106...

bors added a commit that referenced this pull request Jan 24, 2018

Auto merge of #47204 - varkor:unsafecell-into_inner-safe, r=alexcrichton
Make UnsafeCell::into_inner safe

This fixes #35067. It will require a Crater run as discussed in that
issue.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 24, 2018

Contributor

💔 Test failed - status-travis

Contributor

bors commented Jan 24, 2018

💔 Test failed - status-travis

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 24, 2018

Member

Same error as #47204 (comment), please fix cargo first.

(Triage: Blocked by rust-lang/cargo#4972)

Member

kennytm commented Jan 24, 2018

Same error as #47204 (comment), please fix cargo first.

(Triage: Blocked by rust-lang/cargo#4972)

varkor added a commit to varkor/cargo that referenced this pull request Jan 24, 2018

Allow unused_unsafe in LazyCell in preparation for lib change
rust-lang/rust#47204 makes `UnsafeCell::into_inner` safe, which means `LazyCell::into_inner` will no longer need an `unsafe` block. `LazyCell` is a blocker for the change in Rust: this fix should allow the change to take place.

bors added a commit to rust-lang/cargo that referenced this pull request Jan 24, 2018

Auto merge of #4972 - varkor:lazy-cell-unused-unsafe, r=alexcrichton
Allow unused_unsafe in LazyCell in preparation for lib change

rust-lang/rust#47204 makes `UnsafeCell::into_inner` safe, which means `LazyCell::into_inner` will no longer need an `unsafe` block. `LazyCell` is a blocker for the change in Rust: this fix should allow the change to take place.
@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 28, 2018

Member

@bors retry

The cargo on master contained rust-lang/cargo#4972 already.

Member

kennytm commented Jan 28, 2018

@bors retry

The cargo on master contained rust-lang/cargo#4972 already.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 28, 2018

Contributor

⌛️ Testing commit 4829d50 with merge 21882aa...

Contributor

bors commented Jan 28, 2018

⌛️ Testing commit 4829d50 with merge 21882aa...

bors added a commit that referenced this pull request Jan 28, 2018

Auto merge of #47204 - varkor:unsafecell-into_inner-safe, r=alexcrichton
Make UnsafeCell::into_inner safe

This fixes #35067. It will require a Crater run as discussed in that
issue.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 28, 2018

Contributor

💔 Test failed - status-appveyor

Contributor

bors commented Jan 28, 2018

💔 Test failed - status-appveyor

@alexcrichton alexcrichton merged commit 4829d50 into rust-lang:master Jan 28, 2018

1 of 2 checks passed

homu Test failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 28, 2018

Member

Er this passed, just a 3 hour timeout, merged manually

Member

alexcrichton commented Jan 28, 2018

Er this passed, just a 3 hour timeout, merged manually

@varkor varkor deleted the varkor:unsafecell-into_inner-safe branch Jan 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment