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

Externally implementable traits #3645

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented May 22, 2024

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented May 22, 2024

Bikeshed: instead of calling it extern trait, what about extern mod? (Unlike trait, mod doesn't imply an implementing type)

@Lokathor
Copy link
Contributor

I do not know what the good answer is, but extern trait is not good if you can't use it as a trait bound. we don't need to add confusion to the language, just use a separate name.

@Jules-Bertholet
Copy link
Contributor

If we do want to stick with trait (for example, because it's what users are familiar with in terms of "specifying/implementing an interface"), I think we should stay consistent with the type-directed nature of Rust traits and return to a design similar to #2492. (This reminds me of the traits vs first-class modules debate)

@max-niederman
Copy link
Contributor

Bikeshed: instead of calling it extern trait, what about extern mod? (Unlike trait, mod doesn't imply an implementing type)

I like this a lot more than using "trait." IMO it's much closer to a module since, as the RFC says (emphasis added)

An extern trait may contain functions but not other associated items such as types or constants. Additionally, these functions may not refer to self or Self in their signature. Effectively, these functions follow the same rules as free functions outside an impl block.

I also think it's quite readable if you use impl mod <extern_module_name> like

// core::panic:

extern mod PanicHandler {
    fn panic_handler(_: &PanicInfo) -> !;
}

// user:

impl mod core::panic::PanicHandler {
    fn panic_handler(panic_info: &PanicInfo) -> ! {
        eprintln!("panic: {panic_info:?}");
        loop {}
    }
}

@Jules-Bertholet
Copy link
Contributor

Also, should crates be allowed to partially impl an extern trait/mod, and let other crates impl the rest? And should there be a way to "weakly" impl such an item (permitting downstream crates to partially or fully override)?

@Amanieu
Copy link
Member Author

Amanieu commented May 22, 2024

The problem with #2492 is that it's not something that can be resolved at link-time: it introduces circular dependencies in the type system which can't be resolved without deferring all codegen to the root crate.

Also, should crates be allowed to partially impl an extern trait/mod, and let other crates impl the rest? And should there be a way to "weakly" impl such an item (permitting downstream crates to partially or fully override)?

No, the entire point of grouping functions in a single trait is that they must all be provided together. If you want to only partially implement an extern trait then you should provide your own extern trait for the part that your crate doesn't provide and then forward your extern impl to that.

Bikeshed: instead of calling it extern trait, what about extern mod? (Unlike trait, mod doesn't imply an implementing type)

I think a case can be made for either trait or mod, since it's really something that shares some characteristics of both. Note that there are downsides to extern mod as well, such as the syntax for extern unsafe mod or impl mod looking strange compare to how mod is usually used.

@traviscross
Copy link
Contributor

traviscross commented May 22, 2024

Setting aside the nits that one could pick on this proposal, the main and interesting question here seems to be whether it should be possible to express that the person who implements an extern item must uphold certain obligations (that the compiler cannot check) in order to prevent undefined behavior.

We recently covered a similar case to this in:

There, we resolved that the person who declares the signatures within an extern block is responsible for those being correct, and that this is a separate obligation from the ones that a caller (or other user) must uphold when calling (or otherwise using) an item not marked safe within an extern.

There is conceptual overlap between that and this RFC (and the alternative proposals). Here, there's a difference between the caller having to uphold certain unchecked obligations when invoking one of these functions and the implementer having to uphold certain unchecked obligations. Given the intended and anticipated use cases for this, I can certainly see how this distinction could matter, and given that we just addressed a similar case in RFC 3484, I could see a lot of reason to be consistent here conceptually.

Due to how central this is to the motivation for this proposal, @Amanieu, I might suggest adding more discussion of this to the RFC.

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented May 23, 2024

The problem with #2492 is that it's not something that can be resolved at link-time

Considering this further, I think the analogy to extern {} blocks is the right way to think about this feature. In today's extern {} (tomorrow's unsafe extern {}), you declare a set of interfaces, such that each interface has exactly one implementation. However, the compiler can't check that said implementation exists and is unique and valid, so the declarer of the extern block must unsafely promise those things.

In this hypothetical feature, one also declares a set of interfaces with exactly one outside implementation; however, now it's the compiler's job to check that this implementation exists and has the correct signature. (There may be additional preconditions on top of that, and the interface definer should be able to specify this by requiring interface implementers to use unsafe.)

Other than the burden of unsafe proof, these two features are extremely similar; therefore their syntaxes should arguably look similar also. Perhaps something like this:

//! crate foo
extern impl {
    // Unsafe to call, safe to implement
    unsafe fn frob();
    static FOO: u32;
}

unsafe extern impl {
    // Safe to call, unsafe to implement
    fn brotzle();
}
//! crate bar
extern crate foo;

impl unsafe fn foo::frob() {
    println!("frobbing");
}

impl static foo::FOO: u32 = 42;

unsafe impl fn foo::brotzle() {
    println!("brot");
}

@tmandry
Copy link
Member

tmandry commented May 23, 2024

@Jules-Bertholet I agree; I just posted a suggestion for how to unify the proposal in the other RFC with extern blocks: #3632 (comment). Looks like we are thinking along the same lines.

@tmandry
Copy link
Member

tmandry commented May 23, 2024

For this RFC, I would suggest resolving the issues that have been raised by using a concrete type for the impl. That way it's "just" a trait implementation for a regular concrete type.

// core::panic:

pub trait PanicHandler {
    fn panic_handler(_: &PanicInfo) -> !;
}

pub struct GlobalPanicHandler;
extern impl PanicHandler for GlobalPanicHandler;

// user:

impl core::panic::PanicHandler for core::Panic::GlobalPanicHandler {
    fn panic_handler(panic_info: &PanicInfo) -> ! {
        eprintln!("panic: {panic_info:?}");
        loop {}
    }
}

This shouldn't inherit the problems of the other RFC, but should be compatible with it. Perhaps it can even be made strongly forward-compatible with it in the sense that it would be forward-compatible for the above code in core to switch to using extern type in the future, if the GlobalPanicHandler type contained a PhantomExternSized or similar.

And while we're at it, we could just allow extern impl for inherent impls without needing a trait at all.

@ehuss ehuss added T-lang Relevant to the language team, which will review and decide on the RFC. T-types Relevant to the types team, which will review and decide on the RFC. labels May 23, 2024
@joshtriplett
Copy link
Member

Looking at this, I'm going to reiterate the concern I raised in the meeting: this seems confusingly different from other uses of trait in the language, and I think it would cause more confusion than clarity.

I would love to see a real trait-based solution, but not something that's just using the word "trait" with relatively little in common with traits.

@joshtriplett
Copy link
Member

The problem with #2492 is that it's not something that can be resolved at link-time: it introduces circular dependencies in the type system which can't be resolved without deferring all codegen to the root crate.

That doesn't seem like a fatal problem. We already defer generation of generics to the point where they're instantiated. If we had to defer codegen of things that depend on an external type, would that be such a substantial problem?

(Note: I am not proposing that we block other efforts while waiting for such a solution. I'm trying to envision the simplest possible implementation of full external types with trait bounds.)

@Jules-Bertholet
Copy link
Contributor

If we had to defer codegen of things that depend on an external type, would that be such a substantial problem?

Even if such deferral is feasible, it would be nice (for compile times) if we don't have to do it unless it's inherently necessary. If the thing being externally implemented is a plain function, deferring codegen should ideally not be forced by the design of the feature.

@tmccombs
Copy link

tmccombs commented Jun 2, 2024

Another possibility could be something like:

// core::panic:

pub trait PanicHandler {
    fn panic_handler(&self, _: &PanicInfo) -> !;
}


pub extern static panic_handler: &'static  dyn PanicHandler;

// user:

struct GlobalPanicHandler;
impl core::panic::PanicHandler for GlobalPanicHandler {
    fn panic_handler(&self, panic_info: &PanicInfo) -> ! {
        eprintln!("panic: {panic_info:?}");
        loop {}
    }
}
extern static core::panic::panic_handler = &GlobalPanicHandler;

With some optimization to avoid dynamic dispatch.

@max-niederman
Copy link
Contributor

Another possibility could be something like:

// core::panic:

pub trait PanicHandler {
    fn panic_handler(&self, _: &PanicInfo) -> !;
}


pub extern static panic_handler: &'static  dyn PanicHandler;

// user:

struct GlobalPanicHandler;
impl core::panic::PanicHandler for GlobalPanicHandler {
    fn panic_handler(&self, panic_info: &PanicInfo) -> ! {
        eprintln!("panic: {panic_info:?}");
        loop {}
    }
}
extern static core::panic::panic_handler = &GlobalPanicHandler;

With some optimization to avoid dynamic dispatch.

This is proposed in RFC-3635.

Comment on lines +205 to +207
An alternative is to only provide externally definable *statics* instead.

That would be equivalent in power: one can store a function pointer in a static, and one can return a reference to a static from a function ([RFC 3635](https://github.com/rust-lang/rfcs/pull/3635)).
Copy link
Contributor

Choose a reason for hiding this comment

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

They are equivalent in power, but not necessarily in ergonomics — not how they invite themselves to be used. I think there's a serious problem lurking here.

One of the benefits of the way #[global_allocator] works is that a library can trivially provide a type which implements GlobalAlloc (let's call it foo::Alloc), but leave it to a dependent crate (let's call it bar) to actually declare the static item that uses that type. That dependent can therefore choose to, for example, write its own type bar::Alloc which wraps foo::Alloc to modify its behavior.

On the other hand, the global allocation interface in Example 2 in this RFC does not have this virtue: the easiest path (in the absence of macro support) is for foo to write unsafe extern impl GlobalAllocator { /* several function definitions */ }, and then bar cannot extend foo's allocator implementation. foo would have to go out of its way to provide an allocator that can be wrapped.

foo can already make itself unusable to bar by writing a #[global_allocator] attribute, but the advantage of the declaration being a single static instead of a whole impl block is that it's easy for foo to not do that, and instead just tell its users "declare this static to activate my implementation"; it's less code than the less flexible option. I worry that if this RFC were implemented as-is,

  • crates defining externally implementable traits would write simple-looking traits (rather than ones with extra indirection through another type like Example 3), and
  • crates implementing them would do so in the least-effort way: just write the extern impl with code inside it, providing no opportunity for dependents to build on that implementation.

It's possible for the trait defining crate to mitigate this — such as by providing the macro suggested in Example 2, or writing a specifically single-function trait like Example 3. But that's a mitigation by writing more code. I think this feature needs to be designed so that writing straightforward minimal code has good results — and externally definable statics (that worked like #[global_allocator] currently does) would do that.

(None of this issue is unique to allocators; it applies to any interface made up of multiple functions, such as log::Log. I'm focusing on the allocator example because it has multiple methods, it's already in the RFC, and we already have #[global_allocator] to compare to. I've wanted or required this type of flexibility for both allocators (in test instrumentation) and loggers (to send logs to multiple destinations).)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC. T-types Relevant to the types team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants