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

RFC: Function pointers reform #996

Closed
wants to merge 12 commits into from

Conversation

CloudiDust
Copy link
Contributor

Repurpose Rust's current function pointer types to denote "incomplete function items", and introduce function reference types and new function pointer types, so that function references/pointers work more like value references/pointers.

This is based on #883 and related discussions, where the design is already largely agreed upon. The author of #883, @Ericson2314 is too busy to update that RFC, so this RFC is created.

Rendered


# Detailed design

Make the current function pointer types unsized, and introduce function reference types of the form `&fn(arg_list) -> ret_type` and new (const) function pointer types of the form `&const fn(arg_list) -> ret_type`.
Copy link
Contributor

Choose a reason for hiding this comment

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

A quite misleading typo &const fn -> *const fn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@petrochenkov, thanks, fixed.

@CloudiDust
Copy link
Contributor Author

(EDIT: Posted a line note as a normal comment, removed. Please see the line note replying to @eddyb.)

And also reinterpret them as "incomplete function items".

This change is made in order to deal with the fact that all Rust types
must have dynamically determinable sizes.
@nikomatsakis
Copy link
Contributor

I'll be honest, I trend negative on this idea. It's not that it doesn't make sense, it's just that it's late-breaking changes to the type system that don't feel necessary. The current fn type is quite useful as is, unless you are handling dynamically loaded or generated code. The latter feels like it can quite easily be handled by a wrapper type that implements Fn (the objection was raised in #883 that this would require a number of wrappers to cover different ABIs, variadic arguments, etc, which is reasonable, although I think this could be surmounted in other ways as well).

I am concerned that the change is somewhat invasive to the type system in that it introduces the idea of types that have no size or alignment. (This occurred to me while reading the RFC, but I see that @eddyb raised this same point on #883 and I just hadn't noticed it yet.) Basically, the type fn is not unsized in the same sense of [T] or Trait -- in those cases, there is a reasonable size and alignment that is known but we have chosen to "forget". In the case of fn, there is no size at all. (RFC #709, which covered things like null-terminated strings, had a similar requirement.)

It may be that we just need to incorporate this notion ("Sizeable", as @eddyb called it, though I'd prefer another name). It is certainly important because I would like to eventually permit passing DST by value, so that we could do things like Box::new(*rc), where rc has a type like Rc<[int]>, to allocate a boxed array of the right (dynamically known) size. This would be fine if rc had a type like Rc<Trait> as well, but invalid for types like Rc<fn()> (under this proposal) or Rc<...null-terminated-string...> (per #709). But I'm not sure if this will cause future complications and crossing the line into "truly unsized" types without time to experiment makes me nervous! (The fact that Box::new doesn't accept unsized types yet though means that there is still room to tweak defaults etc as needed in the future.)

There is one other angle from which this RFC may make sense, though. I appreciate that it makes the "nullability and safety" of fn pointers part of the pointer type. @brson was recently discussing with me how to properly type a C function pointer that one receives from C code, you ought to write Option<unsafe extern "C" fn()>, which is quite a mouthful -- and most people just write extern "C" fn(), which is wrong. Under this scheme, it seems like we could potentially just write *const extern "C" fn(), which feels much nicer (here I am sort of ignoring the matter of unsafe fn pointers; it seems conceivable to me that we could just remove this from the type system and disallow coercing from an unsafe fn item to a fn pointer, although it could also be an oversimplification. If we kept unsafe fn types, the "correct" type would probably be *const unsafe extern "C" fn(), but there's no much difference in practice.)

Some minor changes to the RFC as written I would like to see:

  • The RFC talks about &fn() and *const fn() as if they are atomic types, and mentions &mut fn() not being a thing. This isn't really how the type system works though -- &T, *const T, &mut T etc are types for all T. This means that in fact &mut fn() is a type, if not a particularly useful one. Similarly, there is no need to explicitly mention &fn and *const fn as being types, it's enough to say that fn becomes an unsized type.
  • I didn't read the RFC logic super carefully, I just kind of assumed that it aligned with what seemed to be the consensus on Make function pointer types look like borrowed pointer types for forwards compatability #883, but I'll try to re-read it shortly and make sure I agree with it (not a change to the RFC, just a fact).
  • I'd like the alternatives from Make function pointer types look like borrowed pointer types for forwards compatability #883 copied in.
  • I'd like an examination of the practical impact of this change. I think that it in terms of existing code, it means that:
    • fn() => &'static fn()
    • extern "C" fn() => &'static extern "C" fn() or *const extern "C" fn()
      • depending on the source of the fn, and modulo unsafety

UPDATE: After a more careful reading of this RFC, I see that some of the things I said I wanted already existed. My bad. Perhaps they can just be made more prominent. :)

The following rules will apply:

1. `fn{f}`s are still function item types, or "function handles/proxies/values", whose representations and semantics remain unchanged.
2. `fn`s are no longer function pointer types, but types representing "incomplete function items", which are unsized statically and zero-sized dynamically.
Copy link
Contributor

Choose a reason for hiding this comment

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

zero-sized dynamically doesn't seem right; they ought to have no dynamic size I think

@nikomatsakis
Copy link
Contributor

OK, I did a more careful reading, and I see that the RFC already addressed my big concern about "unsized types". Sorry about that. Problem is that I don't like this answer of saying that boxes have zero-size dynamically. It seems to imply that Box::new would do wacky things (presuming it supported unsized types). Box::new as I envisioned it would work roughly like this:

impl<T:?Sized> Box<T> {
    fn new(value: T) -> Box<T> {
        let size = sizeof_value::<T>(&value);
        let data = malloc(size);
        memcpy(data, &value, size);
        forget(value);
        transmute(data)
    }
}

but if that were applied with T=fn, that means that size would be 0, and hence the result would basically be an undefined pointer, so any attempt to call a Box<fn()> would crash. No good.

That said, this design is not even at the RFC level yet, much less implemented. Nor do I think we have stabilized any intrinsics or other things that truly require all unsized types to have sizes yet (perhaps alignments, but that seems like it's not as big a problem). So we could probably say that size is zero for now, and later add some sort of traits to distinguish this case where the size of actually just not knowable or relevant. So maybe my objection isn't that strong: but it still seems like a possible "forwards compatibility" risk.

@glaebhoerl
Copy link
Contributor

@nikomatsakis A subtle change seems to have happened along the way from the previous RFC to this one: #883, acknowledging the time pressure, was originally specified as only a superficial syntactic change (from fn() to &'static fn()), to keep open the possibility of reforming the semantics at some later point post-1.0. This RFC, by contrast, sets out the reformed design in full. The complaint now is that this is too big a lift this late in the cycle, and that it has some novel aspects which risk backing us into corners we may later regret to be in. But if that's the case, then couldn't we simply revert to the earlier plan - to update only the surface syntax for now, and revisit the rest of it later when we have more room to breathe?

@nikomatsakis nikomatsakis self-assigned this Mar 23, 2015
@nikomatsakis
Copy link
Contributor

@glaebhoerl I personally consider having types that are written &'static fn() but do not match against &'static T to be worse than the status quo. We've seen the confusion that causes, and the use case for this change seems to be relatively slim (as I wrote) -- not to mention addressable in a more targeted way in the future (e.g. #252 or the wrapper approach).

@glaebhoerl
Copy link
Contributor

I personally consider having types that are written &'static fn() but do not match against &'static T to be worse than the status quo

That's not in dispute. The trade would be having something worse than the current status quo for a short period (~1.0) in exchange for something better than it forever after. (Again, just as a stopgap if doing the whole thing at once would be too much.)

@nikomatsakis
Copy link
Contributor

@glaebhoerl

The trade would be having something worse than the current status quo for a short period (~1.0) in exchange for something better than it forever after.

My concern about this proposal was that we make fn a type and it must be "truly unsized" to fit in. If we change the syntax to &'static fn(), then the only "better future" is one in which fn is a type, precisely what I was concerned about. Basically my concern is not the difficulty to implement -- though there is that -- but rather the fact that this commits us to a particular path in the type system that may have unforeseen interactions.

@glaebhoerl
Copy link
Contributor

Ah, it wasn't clear that your concerns spring directly from the fundamental idea, rather than from the details of how the sizing / unsizing / dynamically sizing aspects are handled (which I admit I don't really follow). Thanks for clarifying.

@CloudiDust
Copy link
Contributor Author

@nikomatsakis Thanks for the reply!

I'll update this RFC to address the problems you brought up. (I still think I am correct about some of them, so please again correct me if I am misunderstanding something.)

You mentioned in #883 that the proposed &fn{f} -> &fn coercions would have to first determine the actual values of the &fn pointers because &fn{f}s/&Fs would point to undefined locations. And here you pointed out that Box<fn>s have to be special cased.

So I am thinking, maybe we can somewhat "unify" the two special cases and say:

  • For fn{f} and fn types, though they are considered statically or dynamically zero-sized, the values of pointers to them are defined - the starting addresses of the functions they represent.

Maybe we need a FnHandle (open to bikeshedding) marker trait to express such a property?

@glaebhoerl
Copy link
Contributor

@nikomatsakis (That said, there is potentially another way to make fn a type - I'm not sure what the precise differences and tradeoffs end up being.)

@CloudiDust
Copy link
Contributor Author

@nikomatsakis, on second thought, as fn{f}s are currently not zero-sized and &fn{f}s have defined values, we cannot mark fn{f}s as FnHandles. Thus, only fns can be FnHandles, which makes the trait kind of pointless.

Treating fns as zero sized dynamically but don't yet expose the fact (if possible) is better. (That means not extending the likes of size_of_val to unsized types for now.)

@petrochenkov
Copy link
Contributor

Can fn() be Sized with size 0, but not Copy and not creatable, a la enum E {}?
This is how "truly unsized" types originating from FFI are treated today.
Value_opaque from librustc_llvm is a good example:

#[allow(missing_copy_implementations)]
pub enum Value_opaque {}
pub type ValueRef = *mut Value_opaque;

@eddyb
Copy link
Member

eddyb commented Mar 24, 2015

I consider that our treatment of uninhabited types is subpar and a footgun in the presence of unsafe code, fwiw (easily leads to UB).
Disallowing Copy seems like it would improve the situation, I guess, but I'm not convinced it's enough.

And please, if we add some trait-based mechanism, let's think of the future and make it general enough to describe entire classes of types, not the issue at hand, alone.

@CloudiDust
Copy link
Contributor Author

@eddyb, agreed. FnHandle is too specific to be useful. It is better if we can "hide" the dynamic sizes of fns for now.

@CloudiDust
Copy link
Contributor Author

This RFC has been revised.

And some other adjustments.
@nikomatsakis
Copy link
Contributor

So somehow I missed @huonw's comment from before, or else it didn't sink in. I found it informative not only because of analyzing how commonly (or, more to the point, rarely) fn pointers are used but because it highlights more aspects of the stdlib (and language) that might benefit here, and in particular points out that e.g. #[weak_linkage] would benefit from being able to express nullable fn pointers. That nudges me slightly in favor of this design.

However, this doesn't change my basic feeling that I don't want to introduce a new concept (types with no sizes) into the type system this soon before beta. It may be that support for such types is inevitable but that's not totally clear to me yet. I think this RFC has covered really interesting ground in the design space, and we may well want to adopt a design like this, but there is no reason we can't come back to this after 1.0 is released by making fn an alias for something else like &'static fnptr(...). But we may find we can model the concepts using library types in a satisfactory way (e.g., weak-linkage can be done with Option types).

(After all, the same statistics that show that changing fn wouldn't break a code also suggest that improving fn wouldn't benefit a lot of code either, since simple fn pointers just aren't that important or widely used.)

UPDATE: Edited slightly for clarity.

@petrochenkov
Copy link
Contributor

@nikomatsakis

but there is no reason we can't come back to this after 1.0 is released by making fn an alias for something else like &'static fnptr(...).

Or we can turn this design 180 degrees!

  • Now: rename function pointer types from fn() to fnptr(). This RFC becomes almost trivial.
  • Day N, somewhere in the future: Introduce truly unsized types.
  • Day N + 1: Introduce fn(), deprecate fnptr() and make it alias to &'static fn()

@CloudiDust
Copy link
Contributor Author

@petrochenkov, good idea! I'll revise the RFC. (Or, if all wanted now is simply renaming fns to fnptrs, maybe I should close this and file a new dedicated RFC?)

@petrochenkov
Copy link
Contributor

I would refrain from complete rewriting of the RFC until at least one more person considers this idea acceptable :)

@eddyb
Copy link
Member

eddyb commented Mar 25, 2015

👎 on adding a fnptr keyword - this late such suggestions should not be taken lightly.

@CloudiDust
Copy link
Contributor Author

@eddyb, I know beta is approaching fast, but fwiw, I suppose fnptr is a much smaller breaking change than what the RFC proposes? Or is adding keywords actually a much more involving procedure than I expected?

@CloudiDust
Copy link
Contributor Author

@eddyb, on second thought, the "user visible" breaking change may not be smaller, but there should be less risk in the implementation and future proofing department.

@CloudiDust
Copy link
Contributor Author

I noticed that we had unsized reserved as a keyword.

&'static unsized foo(...) and *const unsized foo(...) can be added in a backwards compatible way, without doing any changes now. Though it is a bit longer.

@petrochenkov, would this be an accaptable alternative to renaming fn types now?

@nikomatsakis @eddyb, does this align with the intended semantics of the unsized keyword?

Also I think we should at least do one terminology change, let's call the current function pointers "function handles", because they are like pointers but not considered as "true pointers" by the language.

@eddyb
Copy link
Member

eddyb commented Mar 27, 2015

@CloudiDust There should be no unsized keyword, there is no use for it. It was considered necessary before Sized? (now ?Sized) was introduced.

@CloudiDust
Copy link
Contributor Author

@eddyb, I see, so, would &unsized fn and *const unsized fn be a good use of unsized? Or is it an abuse?

@CloudiDust
Copy link
Contributor Author

Yet another possible syntax for function references/pointers may be:

&'static [fn] (arg_list) -> ret_type
*const [fn] (arg_list) -> ret_type

This looks better and shorter, but requires looking ahead for disambiguation. (Other possibilities like &'static ~fn/ *const @fn would result in sigil soup and ~fn/@fn used to mean other things in old Rust.)

EDIT: Clarification and typo correction.

@nikomatsakis
Copy link
Contributor

So, as I've written earlier, I think RFC is on a good track, but we're not going to take it in the immediate future, because there are higher priorities at the moment and these changes can be done backwards compatibly. (And, as I mentioned, I'm particularly reluctant to introduce "truly unsized" types just now until we have some other aspects of DST better nailed down, though we should definitely keep them in mind.) Therefore I'm closing this RFC as postponed under #1037. Thanks @CloudiDust and others for putting in the effort into the (many iterations of...) this design. I feel fairly confident we will come back to the design advanced by this RFC at some point.

@nrc nrc added the postponed RFCs that have been postponed and may be revisited at a later time. label Jun 4, 2015
@eternaleye eternaleye mentioned this pull request Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
postponed RFCs that have been postponed and may be revisited at a later time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants