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

Set transmute from fn item to type fn lint a hard error #19925

Closed
nikomatsakis opened this Issue Dec 16, 2014 · 42 comments

Comments

Projects
None yet
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Dec 16, 2014

Original title: Fn item types should be zero-sized

Per RFC 401, every fn in Rust should have its own unique, zero-sized type. This fits with the overall design of closures, which also have their own unique types (but those types are not zero-sized, as they carry the environment). None of these types have any explicit syntax at present. This has been a longstanding plan, but until recently it was not fully implemented. This is changing with PR #31710, which means that some existing code will need to be adjusted to account for the new behavior. This issue attempts to summarize how things work and describe what needs to be changed.

Zero-sized fn types

What all of this means is that, if you have a function declaration foo:

// for the purposes of this discussion, all of these different kinds of `fn` declarations are equivalent:
fn foo(x: i32) { ... }
extern "C" fn foo(x: i32);
impl i32 { fn foo(x: self) { ... } }

the type of foo is not fn(i32), as one might expect. Rather, it is a unique, zero-sized marker type that I will just write as typeof(foo). However, typeof(foo) can be coerced to a function pointer fn(i32), so you rarely notice this:

let x: fn(i32) = foo; // OK, coerces

The reason that this matter is that the type fn(i32) is not specify to any particular function: it's a function pointer. So calling x() results in a virtual call, whereas foo() is statically dispatched, because the type of foo tells us precisely what function is being called.

Impact on users

As noted above, coercions mean that most code will continue to work just fine before and after this issue is fully fixed. However, you can tell the difference in a few scenarios. Perhaps the most prominent is using transmute to convert a fn item into a fn pointer. This is often done as part of an FFI:

extern "C" fn foo(userdata: Box<i32>) {
   ...
}

let f: extern "C" fn(*mut i32) = transmute(foo);
callback(f);

Here, transmute is being used to convert the types of the fn arguments. This pattern is now incorrect because, because the type of foo is a function item (typeof(foo)), which is zero-sized, and the target type (fn()) is a function pointer, which is not zero-sized. For now, this pattern still works due to some special-cased code in the compiler, but that code is expected to be removed. For future compatibility, this pattern should be rewritten. There are a few possible ways to do this:

  • change the original fn declaration to match the expected signature, and do the cast in the fn body (perhaps the best);
  • cast the fn item fo a fn pointer before calling transmute, as shown here:
    • let f: extern "C" fn(*mut i32) = transmute(foo as extern "C" fn(_))
    • let f: extern "C" fn(*mut i32) = transmute(foo as usize) /* works too */

The same applies to transmutes to *mut fn(), which were observed frequently in practice. Note though that use of this type is generally incorrect. The intention is typically to describe a function pointer, but just fn() alone suffices for that. *mut fn() is a pointer to a fn pointer. (Since these values are typically just passed to C code, however, this rarely makes a difference in practice.)

Current status and implementation trail

  • PR #19891 laid the groundwork with fresh, unique types per fn item, but left the back-end work undone
  • PR #31710 completes the work.

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Dec 16, 2014

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Dec 16, 2014

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Dec 19, 2014

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Dec 19, 2014

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Dec 21, 2014

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Dec 21, 2014

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Dec 22, 2014

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Dec 22, 2014

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 27, 2015

Nominating. Not sure how important this is.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jan 29, 2015

P-high, not 1.0 milestone.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

On a related note, the subtyping relation is not quite right. This code should not type check:

fn foo<A>(_: A, _: A) { }

fn bar<B>() { }

fn main() {
    let x = bar::<u32>;
    let y = bar::<i32>;
    foo(x, y);
}
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

To elaborate, the type of x and y should be enough to precisely identify the code that's about to run, but here the types of two distinct versions of bar are considered compatible. Not good! This works out ok because we're still encoding the fn pointer anyway.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

Put another way, the type of a fn item ought to have the values for all of its substs (sort of like bar<u32> vs bar<i32>). But the compiler is currently representing them as something like fn() {bar} -- that is, the fn signature plus a def-id.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

Nominating. Not sure how seriously to take this (but probably not that hard to fix).

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Apr 2, 2015

Classifying even the type-system issue as a "bug that needs fixing" -- thus we can afford to leave this as P-high, not a 1.0-blocker.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Apr 2, 2015

Tagging as P-backcompat-lang since compiler incorrectly accepts things it shouldn't accept.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Jan 5, 2017

Closing it as in "this has been fully implemented", by merging #34198.

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Jan 6, 2017

I have to admit that I I continue to wonder if we should just define the behavior of transmute in this case to be that it reifies into a fn ptr. It's a very useful kind of conversion.

The latter point may be true, but transmute doing anything besides actual bit-for-bit transmuting feels very wrong to me.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 7, 2017

The latter point may be true, but transmute doing anything besides actual bit-for-bit transmuting feels very wrong to me.

Yeah...I know.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 10, 2017

cc @pnkfelix and @withoutboats -- your thoughts here?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 10, 2017

@nikomatsakis can't you always as cast? Is making transmute just about convenience? I wouldn't like to add special cases to something as unsafe as transmute just for convenience.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 10, 2017

@withoutboats you can, indeed. In any case, the proposal is to remove the special case from transmute (and hence you will get a hard error).

What I am saying is merely that I would like a more concise syntax here, since when you actually have to write (e.g.) x as fn(_, _) -> _ that is pretty long and annoying. My particular problem arose in a macro, where I also didn't have the type readily on hand.


@eddyb -- one thing, though it has more to do w/ the PR than this, I think we should make sure to give a very clear error message, along with a suggestion of what to write instead.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 10, 2017

@rfcbot reviewed

you can, indeed. ... What I am saying is merely that I would like a more concise syntax here

Sounds reasonable, just wanted to make sure I understand that this special case is just about concision & not expressibility.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Jan 10, 2017

@nikomatsakis In that case, we have to keep the special-casing code where we're not emitting the lint.
At least it'd still be an error and trans would stay clean.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 11, 2017

@eddyb

In that case, we have to keep the special-casing code where we're not emitting the lint.

Yes, such is life. I left a comment in the PR.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 11, 2017

Basically nice errors == special-case code. =)

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jan 12, 2017

@rfcbot reviewed

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 17, 2017

It seems that @rfcbot went on vacation. I will put this into Final Comment Period manually:

🔔 Hear ye, hear ye! This issue is now entering a 3-week final comment period, which expires February 7, 2017. 🔔

The intention is to convert this forward compatibility warning into a hard error.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 23, 2017

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

psst @eddyb, I wasn't able to add the final-comment-period label, please do so.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 2, 2017

The final comment period is now complete.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Feb 17, 2017

Although the FCP is complete, the change has not been finalized, and I want to float one possible tweak to the rules that might go a long way to eliminating the annoyance that these rules can cause. Each function has a unique type, but we could make the unique types for extern "C" functions have he size of a pointer, and only make extern "Rust" functions be zero-sized. After all, a common time to send pointers is in FFI.

@wycats

This comment has been minimized.

Copy link
Contributor

wycats commented Feb 17, 2017

I raised this with @nikomatsakis privately because this broke a significant amount of code I wrote that I just came back to after a number of months.

In my view extern "C" fn is more or less equivalent to repr(C), and means something like: "a function intended to be called using the C ABI." In general, C code is calling functions using the C ABI, and C code also requires an actual pointer in order to work.

While there may be some cases of extern "C" functions that are only ever called from Rust, this seems exceedingly rare compared to extern "C" functions meant to be called from C. Since pointers are part of the C representation in such an exceedingly large % of cases, the extra type-cast we're imposing here seems inappropriate.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Feb 17, 2017

@wycats You should never need the transmute in the first place, except when dealing with variadics (and we could even add coercions for those!).

@wycats

This comment has been minimized.

Copy link
Contributor

wycats commented Feb 18, 2017

@eddyb you don't need the transmute because you can do a manual cast?

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Feb 18, 2017

transmute is not something you should ever touch, unless there is no other way.

I suppose we should've been stricter and not allow many type combinations, where a better option exists.

bors added a commit that referenced this issue Feb 28, 2017

Auto merge of #34198 - eddyb:you're-a-bad-transmute-and-you-should-fe…
…el-bad, r=nikomatsakis

Make transmuting from fn item types to pointer-sized types a hard error.

Closes #19925 by removing the future compatibility lint and the associated workarounds.
This is a `[breaking-change]` if you `transmute` from a function item without casting first.
For more information on how to fix your code, see #19925.

bors added a commit that referenced this issue Feb 28, 2017

Auto merge of #34198 - eddyb:you're-a-bad-transmute-and-you-should-fe…
…el-bad, r=nikomatsakis

Make transmuting from fn item types to pointer-sized types a hard error.

Closes #19925 by removing the future compatibility lint and the associated workarounds.
This is a `[breaking-change]` if you `transmute` from a function item without casting first.
For more information on how to fix your code, see #19925.

bors added a commit that referenced this issue Mar 1, 2017

Auto merge of #34198 - eddyb:you're-a-bad-transmute-and-you-should-fe…
…el-bad, r=nikomatsakis

Make transmuting from fn item types to pointer-sized types a hard error.

Closes #19925 by removing the future compatibility lint and the associated workarounds.
This is a `[breaking-change]` if you `transmute` from a function item without casting first.
For more information on how to fix your code, see #19925.

bors added a commit that referenced this issue Mar 1, 2017

Auto merge of #34198 - eddyb:you're-a-bad-transmute-and-you-should-fe…
…el-bad, r=nikomatsakis

Make transmuting from fn item types to pointer-sized types a hard error.

Closes #19925 by removing the future compatibility lint and the associated workarounds.
This is a `[breaking-change]` if you `transmute` from a function item without casting first.
For more information on how to fix your code, see #19925.

@bors bors closed this in #34198 Mar 1, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Mar 2, 2017

My take on this proposal that I floated earlier:

I still think it's worth considering making extern "C" fn types not be zero-sized, but it seems like it ought to go through an RFC, and there is no reason for that decision to affect the FCP of this particular issue.

@wagenet

This comment has been minimized.

Copy link
Contributor

wagenet commented Apr 11, 2017

The main thing that makes me sad about this is that it seems like a regression. Code that used to work now breaks without a major version change. At least there is a workaround.

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.