rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

[RFC] externally implementable functions

Open m-ou-se opened this issue 1 year ago • 42 comments

An alternative to this is https://github.com/rust-lang/rfcs/pull/3635

Rendered

Tracking:

  • https://github.com/rust-lang/rust/issues/125418

m-ou-se avatar May 10 '24 09:05 m-ou-se

How do you deal with the following case?

crate A defines an extern function f.

crate B imports A and implements f. crate C imports A and implements f.

crate D imports B and C but cannot because both of them implement f and they conflict.

I agree conflicting implementations are a compiler error, but if libraries do it then you make some libraries mutually exclusive.

Edit: as I noted below, you might make it so B and C can both be imported as long as D also implements f

jdonszelmann avatar May 10 '24 09:05 jdonszelmann

I agree conflicting implementations are a compiler error, but if libraries do it then you make some libraries mutually exclusive.

Yes, those would be mutually exclusive. Exactly how today panic-halt, panic-semihosting, and panic-reset are all mutually exclusive.

m-ou-se avatar May 10 '24 10:05 m-ou-se

Can libraries define a new default implementation of an extern function? Something like

// crate A:
extern impl fn logger() -> Logger {
    Logger::to_stdout().with_colors()
}

// crate B imports A:
extern impl fn a::logger() -> Logger {
    Logger::to_stderr().with_colors()
}

// crate C imports B:
impl fn b::logger() -> Logger {
    Logger::to_file("log.txt")
}

jdonszelmann avatar May 10 '24 10:05 jdonszelmann

Can libraries define a new default implementation of an extern function?

No, I don't think we should do that. It'd be hard to define the priority of the multiple defaults.

m-ou-se avatar May 10 '24 10:05 m-ou-se

How do you deal with the following case?

crate A defines an extern function f.

crate B imports A and implements f. crate C imports A and implements f.

crate D imports B and C but cannot because both of them implement f and they conflict.

I agree conflicting implementations are a compiler error, but if libraries do it then you make some libraries mutually exclusive.

Edit: as I noted below, you might make it so B and C can both be imported as long as D also implements f

On the one hand, I would hope that crates that implement externally implementable functions would be minimal, i.e. only have this implementation and nothing else, so that including both in the crate graph would always be a clear and understandable error (and not a mistake because you'd want functionality from both).

On the other hand, the example of logging shows that there are clear cases where you might want to combine different loggers into some super-logger that is specific for your use case. Logging implementation crates could then either ship a companion crate that only implements the function, or could have a non-default feature to enable the implementation.

juntyr avatar May 10 '24 10:05 juntyr

Simply feature gating these implementations solves a lot of the problems I think.

jdonszelmann avatar May 10 '24 10:05 jdonszelmann

If none are found, the result is either an error, or, if the extern impl fn has a default body, an implementation is generated that calls that default body.

I think the RFC needs to clarify how this works with different crate types. Presumably this check is not required when building a library crate (or else the feature would be useless). What about when building a dylib or cdylib?

Also, I think it would be good if the root (binary) crate could resolve the issue with multiple implementations by providing its own implementation.

Diggsey avatar May 10 '24 11:05 Diggsey

I think it would be good if the root (binary) crate could resolve the issue with multiple implementations by providing its own implementation.

That's a possible extension. But that's not what we currently have for the panic handler or global allocator, right?

m-ou-se avatar May 10 '24 11:05 m-ou-se

Very nice RFC.

It's unlikely the root binary crate should provide these or micro-manage them too much.

It's most likely library crate features control these, which enables many options. If you do want micro-managment by the root binary crate then these could live in micro-crates too.

I'd think pub const and pub static would be the most likely future extensions here. pub fn groups could probably wait for more existential types discussions ala https://github.com/rust-lang/rfcs/pull/2492

burdges avatar May 10 '24 12:05 burdges

I think this is a great idea, and I think we should try it as an experiment.

@rfcbot merge

@rfcbot concern github-suggestions

joshtriplett avatar May 10 '24 13:05 joshtriplett

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

  • [x] @joshtriplett
  • [ ] @nikomatsakis
  • [ ] @pnkfelix
  • [ ] @scottmcm
  • [ ] @tmandry

Concerns:

  • ~~github-suggestions~~ resolved by https://github.com/rust-lang/rfcs/pull/3632#issuecomment-2107120772
  • ~~statically-unused-extern-impl-fn~~ resolved by https://github.com/rust-lang/rfcs/pull/3632#issuecomment-2125159873

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. See this document for info about what commands tagged team members can give me.

rfcbot avatar May 10 '24 13:05 rfcbot

@burdges

I'd think pub const and pub static would be the most likely future extensions here.

no no no please don't extend this RFC to const. For static and fn one could implement the feature via the linker. But for const it can be used in compile-time constructs like [T; N] and Array<T, N> meaning the compiler has to consider multiple crates at the same time to fill in the extern const value for compile-time computation. It makes the compilation model extremely complex without much conceivable benefits compared to extern static.

kennytm avatar May 10 '24 15:05 kennytm

I think this may just be a future possibility, but I would assume that some uses would need the implementation to uphold a special contract - so it might make sense to have unsafe-to-impl externally implemented functions? (Being a future possibility, this could also be added later, meaning that for now the implementation will always have no restrictions)

deltragon avatar May 10 '24 19:05 deltragon

Adding restrictions is generally seen as a breaking change, so we wouldn't want to stabilize specific extern impl fns without those restrictions, so having the ability to make restrictions would effectively need to be part of the base RFC concept.

Lokathor avatar May 10 '24 19:05 Lokathor

The existing panic_handler has no such restrictions. Same for the unstable alloc_error_handler.

For global_alloc it's a bit more tricky as it's a bunch of functions and they're unsafe to call and on a trait that's unsafe to implement...

RalfJung avatar May 10 '24 19:05 RalfJung

In the alternatives section, you might want to mention the existing ability to use #[no_mangle] in the defining crate, and extern {} blocks in the calling crate.

The downsides of that approach I can think of are:

  • Global namespace of function names
  • Forced to be unsafe
  • Doesn't work with dylibs (without platform-specific hacks)
  • Cargo doesn't know about it
    • But would this feature get any special Cargo integration? It would be nice to be able to say "this crate is a dependency but only at link time", improving build parallelism.

comex avatar May 10 '24 21:05 comex

@rfcbot resolved github-suggestions

joshtriplett avatar May 13 '24 09:05 joshtriplett

@rfcbot concern statically-unused-extern-impl-fn

joshtriplett avatar May 13 '24 09:05 joshtriplett

I posted an alternative that does this for static instead of fn: https://github.com/rust-lang/rfcs/pull/3635

They're pretty much equivalent in power. Doing it for fn is a bit more ergonomic in quite a few cases, but doing it for static is maybe a bit more consistent. I don't have much of a preference, but we should consider both.

m-ou-se avatar May 13 '24 13:05 m-ou-se

Process question for @rust-lang/lang: should this be an experimental feature gate instead of an RFC?

m-ou-se avatar May 15 '24 15:05 m-ou-se

@m-ou-se Definitely not "instead of", but possibly "in addition to". This feature absolutely needs an RFC in order to be a future stable feature, but it could also be an experimental feature gate in order to implement it without waiting for the RFC to be merged.

That said, I really hope we accept and merge this soon, and I'm hoping we get to it in the next lang meeting. I would like to prioritize it.

joshtriplett avatar May 16 '24 11:05 joshtriplett

At high level, I would expect something like this to be implemented using traits.

Basically we have

  • An interface defined in one place - possibly including multiple functions like with allocators, possibly having default implementaitons.
  • An implementation of that interface defined in a different place.

That screams "use traits for this", because all kinds of interfaces and implementations are always done through traits in Rust. But the RFC defines something that is similar to trait impls, has its own set of surface rules similar to trait impls, but is not trait impls.

petrochenkov avatar May 17 '24 12:05 petrochenkov

That screams "use traits for this"

Yeah, I agree that would be a good fit, but then we basically end up with https://github.com/rust-lang/rfcs/pull/2492, which was not accepted at the time because it involved too much complexity.

I'd be happy if we could pick that route and make that all work. This RFC is just an attempt to do something much more basic to start with, since a much more complicated change like https://github.com/rust-lang/rfcs/pull/2492 seems unlikely to work out any time soon.

m-ou-se avatar May 17 '24 14:05 m-ou-se

As written, linkers can do this resolution, no? Isn't that enough reason this should still exist, even if some non-linker-friendly trait based scheme emerges in 5 years or whatever?

burdges avatar May 17 '24 20:05 burdges

Just like global registration I don't think you want to implement this feature through the linker, at the very least initially. Personally, I'd be worried about the errors that are generated and which are hard to bring to the same standard as other rust compiler errors. I also like this comment about it.

jdonszelmann avatar May 17 '24 22:05 jdonszelmann

I'd be worried about the errors that are generated and which are hard to bring to the same standard as other rust compiler errors.

My suggestion is to have the check for a downstream crate implementing it be in rustc, but the doing the actual tying up with the linker, like is currently done for #[panic_handler] and #[global_allocator] (for the latter the default is handled using a compiler generated shim, but when you actually use #[global_allocator] no shim is used at all.). In the end some form of tying up by the linker is necessary anyway unless you want to have a global constructor which at runtime sets a static to point to the external implementation of the function.

bjorn3 avatar May 18 '24 09:05 bjorn3

@rfcbot resolve statically-unused-extern-impl-fn

Resolved by the use of cfg.

joshtriplett avatar May 22 '24 16:05 joshtriplett

@m-ou-se

That screams "use traits for this"

Yeah, I agree that would be a good fit, but then we basically end up with #2492, which was not accepted at the time because it involved too much complexity.

I'd be happy if we could pick that route and make that all work. This RFC is just an attempt to do something much more basic to start with, since a much more complicated change like #2492 seems unlikely to work out any time soon.

I don't see why traits/impls used for this feature cannot be restricted to prohibit, for example, associated types or generics, if they complicate the minimal design.

petrochenkov avatar May 22 '24 16:05 petrochenkov

I'd be happy to see an implementation using traits that is initially restricted in what can appear in the trait, if that makes it simple enough to ship.

I do hope eventually we can ship a version that allows, for instance, associated constants, and that we can use those associated constants in generic bounds on functions in the standard library. (For instance, conditionally providing impl From<u64> for usize.) However, I know that'd be more complex, and I think we should ship something without that support first if we can do that more easily.

joshtriplett avatar May 22 '24 16:05 joshtriplett

I don't see why traits/impls used for this feature cannot be restricted to prohibit, for example, associated types or generics, if they complicate the minimal design.

So you're proposing basically https://github.com/rust-lang/rfcs/pull/2492 with some restrictions to make the implementation much simpler?

If you have time to write down a more concrete proposal (not necessariliy an RFC, but some clear examples or something), that would be valuable.

m-ou-se avatar May 22 '24 16:05 m-ou-se