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

Tracking issue for fundamental feature #29635

Open
aturon opened this issue Nov 5, 2015 · 26 comments
Open

Tracking issue for fundamental feature #29635

aturon opened this issue Nov 5, 2015 · 26 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Nov 5, 2015

This feature flag, part of RFC 1023, is not intended to be stabilized as-is. But this issue tracks discussion about whether some external feature it needed. Perhaps there is a cleaner way to address the balance between negative reasoning and API evolution. See the RFC for details.

@aturon aturon added T-lang Relevant to the language team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Nov 5, 2015
@steveklabnik

This comment has been minimized.

@khuey
Copy link
Contributor

khuey commented Jun 22, 2017

See rust-lang/futures-rs#479 for an example of something this would solve.

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jul 22, 2017
@steveklabnik
Copy link
Member

Triage: not aware of any change.

@eddyb
Copy link
Member

eddyb commented Mar 20, 2020

So here's a fun bug due to lacking validation:

a.rs:

#![feature(fundamental)]

#[fundamental]
pub struct Foo;

b.rs:

extern crate a;

impl Clone for a::Foo {
    fn clone(&self) -> Self { a::Foo }
}

I think the rule we should enforce is that #[fundamental] types have at least one type parameter, as only types can be local (e.g. constants are only ever "builtin", parameters or projections).

@Diggsey
Copy link
Contributor

Diggsey commented Jul 7, 2020

I think it would make more sense to say that outside the defining crate, the coherence rules for #[fundamental] types are identical to a tuple of its generic parameters. You can't implement Clone for Foo for the same reason that you can't implement Clone for ().

@Diggsey
Copy link
Contributor

Diggsey commented Jul 7, 2020

How do people feel about just stabilizing #[fundamental] on types as-is? I'm sure there could be a "more precise" way to exert control over the coherence rules, but honestly I've only needed to do that two or three times, and in each case #[fundamental] was sufficient.

Even if a more precise way is found, it's not the end of the world to have #[fundamental] as well, and it would be easy to remove with an edition change.

Has anyone actually encountered a situation where more control was needed?

@joshtriplett joshtriplett added S-tracking-design-concerns Status: There are blocking design concerns. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. and removed S-tracking-design-concerns Status: There are blocking design concerns. labels Dec 8, 2021
@joshtriplett
Copy link
Member

This needs some evaluation of what use cases will still exist after the current work on negative reasoning becomes available and stable.

It's possible there's still a use case for it after that, in which case we need to look at that more closely, and figure out if it can be supported in a reasonable way.

Or it's possible that what's left should be perma-unstable, given the coordination difficulties of doing this outside the standard library.

@chorman0773
Copy link
Contributor

chorman0773 commented Dec 9, 2021

I have a library in https://github.com/LightningCreations/lccc (the actual implementation/host side, rather than the stdlib/target side, so I don't have privileged unstable access), xlang_abi, which contains types that are necessary for communicating between modules soundly, and includes reimplementations of some standard library types (including Box). I would like to be able to support fundamental types on, at the very least, xlang_abi::boxed::Box (aformetioned type), as well as the Dyn* types from xlang_abi::traits (which are pointers/reference types that provide a stable abi for trait object types, mostly through emulation).
The relevant types are in https://github.com/LightningCreations/lccc/blob/main/xlang/xlang_abi/src/boxed.rs and https://github.com/LightningCreations/lccc/blob/main/xlang/xlang_abi/src/traits.rs

@mitsuhiko
Copy link
Contributor

I have a use case for #[fundamental] that I don't think there is much of an alternative around. I'm basically providing a wrapper type Slot<T>. Users of the API would like to implement impl MyTrait for Slot<TheirType>. The only way with the orphan rules is to the best of my knowledge this #[fundamental] attribute.

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Feb 2, 2022

FWIW, here is how miniserde handles that: https://docs.rs/miniserde/0.1.19/miniserde/macro.make_place.html (example of an impl like the one mentioned by @mitsuhiko: https://docs.rs/miniserde/0.1.19/miniserde/de/index.html#deserializing-a-primitive)

@mitsuhiko
Copy link
Contributor

The challenge with the approach that miniserde does is that these types are not compatible. miniserde gets around this by using Option<T> as compatible public API and unsafely transmuting between &mut Option<T> and &mut PlaceType<T>. I'm in quite a similar situation and having one shared slot type is obviously preferrable as it could be used in public APIs consistently.

@idanarye
Copy link

How about instead of marking the entire type as #[fundamental] we'd only mark the specific generic parameter that allows impl blocks in other crates?

So:

a.rs:

#![feature(fundamental)]

pub struct Foo<#[fundamental] S, T> {
    // PhantomData ceremony
}

b.rs:

struct Bar;

// This is legal, because S is Bar and we own Bar
impl Clone for a::Foo<Bar, ()> {
    fn clone(&self) -> Self {
        a::Foo {
            // PhantomData ceremony
        }
    }
}

// This is illegal, because Bar is in T. S is () and we don't own ()
impl Clone for a::Foo<(), Bar> {
    fn clone(&self) -> Self {
        a::Foo {
            // PhantomData ceremony
        }
    }
}

Alternative syntax:

#[fundamental(S)]
pub struct Foo<S, T> {
    // PhantomData ceremony
}

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Apr 19, 2022

I think that the subset of #[fundamental]-for(-generic)-types (≠ #[fundamental]-for-traits) which could be stabilized would have to not only be explicit around not only around the generic parameters like you've suggested, @idanarye, but also around the specific traits that downstream users may implement.

Doing so would also improve the teachability of the feature as a whole: the question _what does #[fundamental] stand for is quite complex to answer:

  • it's either for traits, or for generic types. For a non-generic type it doesn't make sense for instance.
  • In the case of generic types, it yields a universal promise, future-wise, regarding a lack of blanket impls over that generic parameter, for any trait in existence. It's not only subtle, but a huge burden maintenance-wise. For instance, breaking that promise would not be easy to spot from a library author point of view (I guess the rationale is that the stdlib has enough pairs of eyes over it so that this footgun doesn't matter).

Stabilizing a subset of it with an explicit notation regarding both the generic parameter, and the trait involved, would thus not only improve the teachability / understandability of the feature and of its semantics, it would also improve the diagnostics when misused, as well as reduce the possitibility for footguns.

Instead of #[fundamental] on a type, have an annotation for empty blanket impls over that type

Say, #[downstream_may_impl], or #[this_crate_shall_not_impl]

  • Or just #[may_impl] (or even keep the #[fundamental] name) — that detail is left for later bikeshedding.
#[bikeshed::this_crate_shall_not_impl]
impl<#[downstream] S, T> Serialize for Foo<S, T>;

this would thus not overlap with:

impl<T> Serialize for Foo<String, T> {}

since String can't be a #[downstream] type, but it would overlap with

impl<S : Clone, T> Serialize for Foo<S, T> {}

Since <S : Clone> and <#[downstream] S> may overlap.

  • This is what I mean when talking of "catching mistakes". By having written that explicit promise of a lack of impl within the current crate, the coherence system would be able to complain should the blanket impl over Clone be added, which it could simply not have done in the case of a #[fundamental(S)] struct Foo<S, T> annotation (since undistinguishable from a blanket impl of Serialize always having been present).

This yields intuitive expectations w.r.t. the classic coherence rules, and is explicit over both the #[downstream] parameter, fundamental/coherence-wise, as well as the specific traits involved.

With it, using pseudo-code, one could even finally illustrate the semantics of Box being fundamental, for instance:

#[bikeshed::this_crate_shall_not_impl]
impl<#[downstream] T : ?Sized> * for Box<T>;
  • (I'm skipping the A : Allocator parameter, since I don't even know whether Box is fundamental over it as well or not)

where we'd be able to see there is yet another magical thing with a #[fundamental] generic type: a * quantification over all possible traits (but for those already implemented / "shadowed"…).


This proposal would be enough to cover @mitsuhiko's and mini_serde's use cases, for instance.

@chorman0773
Copy link
Contributor

chorman0773 commented Apr 19, 2022 via email

@idanarye
Copy link

(I'm skipping the A : Allocator parameter, since I don't even know whether Box is fundamental over it as well or not)

What do you mean "as well"? With your suggestion a type can be fundamental over different generic parameters as long as it's for different traits, but it should still not be fundamental over different generic paramters for the same trait. If Box is fundamental for both T and A over all the traits, than one crate can implement a trait for it for a T that it owns and another crate can implement a trait for it for an A that it owns, and we'd get into the same diamond pattern the orphan rules were created to prevent.

It's up for for the individual type to decide which generic parameters to be fundamental over, and in Box's case it's an easy choice - it's T. HashMap has a much harder choice - should it be fundamental over K or over V? (probably V, but K has a strong enough case here to at least deserve a discussion)

Unless... did you mean you avoided doing something like that?

#[bikeshed::this_crate_shall_not_impl]
impl<#[downstream] T : ?Sized, A: Allocator> * for Box<T, A>;

If so - why not? Even if you allow this, it'd still be up for the downstream crate to include the allocator in their own impls, and if they do something wrong with it (can't imagine what exactly, but let's say the will) it'd be explicit (since they'd have to explicitly add a generic parameter) and if they do something wrong rustc can probably catch it. So why prevent this possibility?

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Apr 21, 2022

Heh, precisely because I didn't want to think about 2-ary-fundamental generic types I skipped the alloc parameter 😅 , but @idanarye it's nonetheless an interesting question; I actually find that my syntax can help explain what possible semantics #[fundamental] for 2-ary-fundamental generic types could be.

Given:

//! `::upstream`
pub trait Trait {}

#[bikeshed::this_crate_shall_not_impl]
impl<#[downstream] T : ?Sized, #[downstream] A: Allocator> Trait for Box<T, A>;

and:

//! `::a`
struct Local;

impl<A> ::upstream::Trait for ::upstream::Box<crate::Local, A> {}

then it would error stating that this blanket-over-the-second-param impl overlaps with the upstream's:

impl<#[downstream] T : ?Sized, #[downstream] A: Allocator> Trait for Box<T, A>;

precisely because a downstream crate could write

//! `::b`
struct Local;

impl ::upstream::Trait for ::upstream::Box<::a::Local, crate::Local> {}

That is, a #[bikeshed::this_crate_shall_not_impl] declaration has the advantage of explicitly forbidding blanket impls overlapping with it, not only within-the-same-crate (except for the impl * for case), but also for downstream crates (even in the impl * for case).


@chorman0773 the impl * for was not an actual or serious proposal, it was just pseudo-code. For that case @idanarye's syntax or the one you've suggested could work 👍 (I just wanted to chime in to suggest that other "ultra explicit and restricted fundamental" syntax to target specific traits, since it would already be quite useful, and I find it quite useful as well even just when reasoning about fundamental)

@idanarye
Copy link

That's the thing - this declaration should be rejected by the compiler:

impl<#[downstream] T : ?Sized, #[downstream] A: Allocator> Trait for Box<T, A>;

The rule should be that only one generic parameter is allowed to be #[downstream] for each such implementation. The creator of the fundamental type has to choose: either this:

impl<#[downstream] T : ?Sized, A: Allocator> Trait for Box<T, A>;

Or this:

impl<T : ?Sized, #[downstream] A: Allocator> Trait for Box<T, A>;

Of course, with this syntax that gives downstream permissions for implementing specific traits, one could do:

impl<#[downstream] T : ?Sized, A: Allocator> Trait1 for Box<T, A>;

impl<T : ?Sized, #[downstream] A: Allocator> Trait2 for Box<T, A>;

And then we could have:

//! `::a`
struct Local;

impl<A> ::upstream::Trait1 for ::upstream::Box<crate::Local, A> {}
//! `::b`
struct Local;

impl ::upstream::Trait2 for ::upstream::Box<::a::Local, crate::Local> {}

@scottmcm
Copy link
Member

I think improving the current behaviour of this might be a blocker for #32838, since it's not at all obvious that we want to let people implement things for Box<T, CustomAllocator>, but it seems like that's what the #[fundamental] currently does.

@idanarye
Copy link

@scottmcm Only if the entire type is made fundamental. If the fundamentalness is limited to one generic parameters, as per my suggestion, then one could implement traits for Box<T, CustomAllocator> only in the crate that defines the T (or the trait). So you can't just implement things for Box<i32, CustomAllocator> because you don't own i32's definition.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 17, 2022
Add `Box<[T; N]>: TryFrom<Vec<T>>`

We have `[T; N]: TryFrom<Vec<T>>` (rust-lang#76310) and `Box<[T; N]>: TryFrom<Box<[T]>>`, but not this combination.

`vec.into_boxed_slice().try_into()` isn't quite a replacement for this, as that'll reallocate unnecessarily in the error case.

**Insta-stable, so needs an FCP**

(I tried to make this work with `, A`, but that's disallowed because of `#[fundamental]` rust-lang#29635 (comment))
@alexkirsz
Copy link
Contributor

We have a use case for #[fundamental] types in turbo, where we're building an incremental computation engine.

A Vc<T> represents the result of a computation. In order to read the underlying value T, you must .await the Vc<T> because:

  • the computation might not have finished otherwise; and
  • we use .await calls to track dependencies.

You can also pass the Vc<T> itself around to create composite computations: fn op(Vc<T>, Vc<U>) -> Vc<V>.

We want to allow users to implement inherent methods on Vc<T> provided T is local. From my understanding of the reference, this should already be possible, but it is not implemented.

Would it make sense to extend fundamental types to cover inherent impls as well?

See also this Zulip thread.

@scottmcm
Copy link
Member

There's no disambiguation mechanism for conflicting things on inherent impls, so that would mean it's a breaking change to add anything, which seems too strong.

I thing the expectation will remain that such methods be added via extension traits, not inherent methods, even on fundamental types.

@idanarye
Copy link

Isn't it already a breaking change to add methods?

Consider this:

mod foo {
    pub struct Foo;
}

mod bar {
    pub trait Bar {
        fn bar(&self);
    }

    impl Bar for super::Foo {
        fn bar(&self) {
            println!("Bar.bar");
        }
    }
}

use foo::Foo;
use bar::Bar;

fn main() {
    Foo.bar();
}

If we add this to mod foo:

impl Foo {
    pub fn bar(&self) {
        println!("Inherent bar");
    }
}

It'll change the behavior of Foo.bar().

@chorman0773
Copy link
Contributor

chorman0773 commented Jan 26, 2023 via email

@idanarye
Copy link

Sop wouldn't doing the same for #[fundamental] and inherent methods also be acceptable?

@scottmcm
Copy link
Member

@idanarye No. It's allowed for traits because you have to option of using UFCS to write it in a way that can't conflict no matter what other methods people add to other traits.

@alexkirsz
Copy link
Contributor

@scottmcm wrote:

There's no disambiguation mechanism for conflicting things on inherent impls, so that would mean it's a breaking change to add anything, which seems too strong.

This is an acceptable tradeoff for us. We won't have inherent impl<T> Vc<T> in the crate where Vc is defined. All inherent impls will occur in user crates, for some known U type (impl Vc<U>).

thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
Add `Box<[T; N]>: TryFrom<Vec<T>>`

We have `[T; N]: TryFrom<Vec<T>>` (#76310) and `Box<[T; N]>: TryFrom<Box<[T]>>`, but not this combination.

`vec.into_boxed_slice().try_into()` isn't quite a replacement for this, as that'll reallocate unnecessarily in the error case.

**Insta-stable, so needs an FCP**

(I tried to make this work with `, A`, but that's disallowed because of `#[fundamental]` rust-lang/rust#29635 (comment))
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
Add `Box<[T; N]>: TryFrom<Vec<T>>`

We have `[T; N]: TryFrom<Vec<T>>` (#76310) and `Box<[T; N]>: TryFrom<Box<[T]>>`, but not this combination.

`vec.into_boxed_slice().try_into()` isn't quite a replacement for this, as that'll reallocate unnecessarily in the error case.

**Insta-stable, so needs an FCP**

(I tried to make this work with `, A`, but that's disallowed because of `#[fundamental]` rust-lang/rust#29635 (comment))
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Add `Box<[T; N]>: TryFrom<Vec<T>>`

We have `[T; N]: TryFrom<Vec<T>>` (#76310) and `Box<[T; N]>: TryFrom<Box<[T]>>`, but not this combination.

`vec.into_boxed_slice().try_into()` isn't quite a replacement for this, as that'll reallocate unnecessarily in the error case.

**Insta-stable, so needs an FCP**

(I tried to make this work with `, A`, but that's disallowed because of `#[fundamental]` rust-lang/rust#29635 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests