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

rustc_typeck: Allow reification from fn item to unsafe ptr #37389

Merged
merged 2 commits into from
Oct 29, 2016

Conversation

cramertj
Copy link
Member

See rust-lang/rfcs#1762.

I've never contributed to the compiler internals before-- apologies if I'm not going about this the right way.

@rust-highfive
Copy link
Collaborator

r? @nrc

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

@nrc
Copy link
Member

nrc commented Oct 25, 2016

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned nrc Oct 25, 2016
@@ -196,6 +196,8 @@ impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> {
// Function items are coercible to any closure
// type; function pointers are not (that would
// require double indirection).
// Additionally, we permit coercin of function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: coercin -> coercion

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

-> CoerceResult<'tcx> {
if let ty::TyFnPtr(fn_ty_b) = b.sty {
match (fn_ty_a.unsafety, fn_ty_b.unsafety) {
(hir::Unsafety::Normal, hir::Unsafety::Unsafe) => {
Copy link
Member

@eddyb eddyb Oct 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks coercing from an unsafe fn item to an unsafe fn pointer. I'm dumb!

@eddyb
Copy link
Member

eddyb commented Oct 25, 2016

@bors r+ Thanks!

@bors
Copy link
Contributor

bors commented Oct 25, 2016

📌 Commit 4bb6d4e has been approved by eddyb

@eddyb
Copy link
Member

eddyb commented Oct 25, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Oct 25, 2016

📌 Commit ab6119a has been approved by eddyb

@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 26, 2016
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 26, 2016
…-ptr, r=eddyb

rustc_typeck: Allow reification from fn item to unsafe ptr

See rust-lang/rfcs#1762.

I've never contributed to the compiler internals before-- apologies if I'm not going about this the right way.
@bors
Copy link
Contributor

bors commented Oct 29, 2016

⌛ Testing commit ab6119a with merge 2b262cf...

bors added a commit that referenced this pull request Oct 29, 2016
rustc_typeck: Allow reification from fn item to unsafe ptr

See rust-lang/rfcs#1762.

I've never contributed to the compiler internals before-- apologies if I'm not going about this the right way.
@bors bors merged commit ab6119a into rust-lang:master Oct 29, 2016
@cramertj cramertj deleted the cramertj/fn-item-to-unsafe-ptr branch October 31, 2016 05:16
@brson
Copy link
Contributor

brson commented Nov 1, 2016

This feature enhancement landed insta-stable. I don't see any discussion of why that decision was made. Can somebody explain?

@cramertj
Copy link
Member Author

cramertj commented Nov 1, 2016

@brson I opened up rust-lang/rfcs#1762 for discussion, but my impression from the comments there was that this was simply a missing implementation. The appropriate conversions already existed (fn item -> fn pointer and fn pointer -> unsafe fn pointer), but the current logic didn't allow them to be applied transitively. This commit just adds the missing transitive conversion.

However, if this requires more discussion, I'd be happy to revert and open a more formal RFC.

@petrochenkov
Copy link
Contributor

@brson
The relevant RFC is https://github.com/rust-lang/rfcs/blob/master/text/0401-coercions.md and the tracking issue is #18469 (comment).
This PR implements part of "add all transitive coercions" sub-item (which is also tracked by #18602).

@nikomatsakis
Copy link
Contributor

I agree that this feels more like a bugfix than a language change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants