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

Calling methods on generic parameters of const fns #2632

Open
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
@oli-obk
Copy link
Contributor

oli-obk commented Feb 5, 2019

TLDR: Allow

const fn add<T: Add>(a: T, b: T) -> T::Output {
    a + b
}

and

pub struct Foo<T: Trait>(T);
impl<T: ?const Trait> Foo<T> {
    fn new(t: T) -> Self {
        // not calling methods on `t`, so we opt out of requiring
        // `<T as Trait>` to have const methods via `?const`
        Self(t)
    }
}

cc @Centril @varkor @RalfJung @eddyb

This RFC has gone through an extensive pre-RFC phase in rust-rfcs/const-eval#8

Rendered

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Feb 5, 2019

One alternative syntax design that is not mentioned here is the question of const impl versus impl const.

There are strong arguments that const impl is the more consistent syntax, as it is consistent both with the existing practice of prefixing modifiers to impl (default impl, unsafe impl, etc.) and prefixing const to keywords to indicate const variants (e.g. const fn [and the hypothetical const trait]).

Conversely, as far as I'm aware, impl const is not consistent with any existing syntax in the language or this RFC.


(I don't like to start the discussion with syntax bikeshedding, but as mentioned in the original post, this RFC has already ungone significant design discussion and I'm satisfied it's close to the optimal conservative design.)

@rpjohnst

This comment has been minimized.

Copy link

rpjohnst commented Feb 5, 2019

How do we want -> impl Trait and arg: dyn Trait to interact with const fn? If you must specify -> impl const Trait, does it also make sense to require arg: dyn const Trait? Both cases feel more existential-y and thus less tied to the function's type parameters, so I kind of lean that way for consistency.

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Feb 5, 2019

I imagine it makes most sense to use the same (syntactic) rules for impl Trait and dyn Trait as for parameter bounds: that is, in const fn, an impl Trait actually means impl const Trait/const impl Trait (and similarly for dyn) and impl ?const Trait/?const impl Trait allows opting-out of constness.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Feb 6, 2019

I don’t think this has been mentioned before, but putting const before impl would be more consistent with other modifiers like unsafe and pub if we decided to do that.

Drop the correct type
Co-Authored-By: oli-obk <github35764891676564198441@oli-obk.de>
@ExpHP

This comment has been minimized.

Copy link

ExpHP commented Feb 6, 2019

I'm not sure why const Drop must be recursive on fields. In my mental model, the body of drop doesn't have calls to the fields' drop impls inserted, but rather, it's the role of drop glue to piece them all together:

{
    let local = get_thing();

    // Implicitly inserted
    // (each one is individually omitted if that type
    //  does not explicitly implement Drop)
    Drop::drop(&mut local);
    Drop::drop(&mut local.field_1);
    Drop::drop(&mut local.field_2);
}

Requiring const Drop on fields increases churn when library A cannot write const Drop yet due to a non-const Drop in library B. (so to use A in a const context, changes now need to be made to both library B and A in that order, instead of just B).


(one could argue that allowing impl const Drop in library A when a field is non-const Drop could lead to misleading documentation; but another thread of discussion on this PR seems to be arriving at the conclusion that we additionally require an auto-trait like ConstDrop, which partially mitigates this concern)

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Feb 10, 2019

I don’t think this has been mentioned before, but putting const before impl would be more consistent with other modifiers like unsafe and pub if we decided to do that.

It has been mentioned before. This was even the original syntax. Reasoning for the change can be found at rust-rfcs/const-eval#8 (comment)

cc @scottmcm for consistency (not in semantics, but user expectations) I have slowly come back around to const impl Trait for Type being the best syntax.

@ExpHP

This comment has been minimized.

Copy link

ExpHP commented Feb 10, 2019

Ultimately the location of const is really nothing but the color of the bikeshed. impl const may be different from the other things, but it is not that hard to remember this "exception" when you have things like T: ?const Trait.

Conceptually, the keyword is attached to the trait. I.e. it's not an impl const of a Trait, but rather an impl of a const Trait.

@Lokathor Lokathor referenced this pull request Feb 11, 2019

Open

Meta tracking issue for `const fn` #57563

0 of 14 tasks complete

@Centril Centril self-assigned this Feb 14, 2019

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Feb 14, 2019

Conversely, as far as I'm aware, impl const is not consistent with any existing syntax in the language or this RFC.

I think that's looking at it from the wrong perspective.

It's not that it's (impl const) Trait, but that it's impl (const Trait), the same as you can impl (dyn Trait). And that's consistent with something like struct Foo<T: const Default>(T); or similar.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Feb 14, 2019

But as far as I can tell, the Constness is not a property of the Trait, but of the impl, which is not the same for dyn, right?

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 14, 2019

It's not that it's (impl const) Trait, but that it's impl (const Trait), the same as you can impl (dyn Trait). And that's consistent with something like struct Foo<T: const Default>(T); or similar.

I agree with this in particular when viewed in light of effect systems. Moreover, impl const Trait also has composability benefits, e.g. arg: impl Add + ?const Clone + const Debug vs. arg: const impl Add + Cone + Debug are probably not the same semantically.

But as far as I can tell, the Constness is not a property of the Trait, but of the impl, which is not the same for dyn, right?

Traits can be seen as logical requirements and implementations are proofs that those requirements are satisfied. In that light, e.g. T: const Foo can be seen as a constraint that T satisfies const Foo and impl const Foo for MyT is a witness for that.

@Centril Centril added the A-effects label Feb 16, 2019

Show resolved Hide resolved text/0000-const-generic-const-fn-bounds.md Outdated
Show resolved Hide resolved text/0000-const-generic-const-fn-bounds.md Outdated
Show resolved Hide resolved text/0000-const-generic-const-fn-bounds.md Outdated
Show resolved Hide resolved text/0000-const-generic-const-fn-bounds.md Outdated
Show resolved Hide resolved text/0000-const-generic-const-fn-bounds.md Outdated
Show resolved Hide resolved text/0000-const-generic-const-fn-bounds.md Outdated
Show resolved Hide resolved text/0000-const-generic-const-fn-bounds.md Outdated
Apply suggestions from code review
Co-Authored-By: oli-obk <github35764891676564198441@oli-obk.de>
Show resolved Hide resolved text/0000-const-generic-const-fn-bounds.md Outdated
Show resolved Hide resolved text/0000-const-generic-const-fn-bounds.md Outdated
Show resolved Hide resolved text/0000-const-generic-const-fn-bounds.md Outdated

Centril and others added some commits Feb 16, 2019

Comma nits
Co-Authored-By: oli-obk <github35764891676564198441@oli-obk.de>
* can only pass types with explicit (const or not) `Drop` impls,
still can't drop any values inside the function
* mention `Drop` in the parameter bounds
* can only pass types with explicit `const Drop` impls (so no `u32`)

This comment has been minimized.

@pnkfelix

pnkfelix Feb 19, 2019

Member

this bullet seems internally inconsistent (potentially due to a typo?): you say "mention Drop in the parameter bounds" for the bullet, but then say in the sub-bullet that such a bound will require explicit const Drop impls. I would think that you either want the bullet to say "method ConstDrop in the parameter bounds", or you want the sub-bullet to say that the Drop bound will only allow passing types with explicit Drop (not const Drop) impls.

This comment has been minimized.

@oli-obk

oli-obk Feb 20, 2019

Author Contributor

This is part of the intro as to why we need ConstDrop. The ConstDrop trait has not been mentioned before here, and I'm introducing it below to offer a fourth way (and actually useful) way to have const fn interact with Drop

Having a T: Foo bound on a const fn will allow you to only pass types that implement const Foo if you are within a const context. Drop isn't special here, so you can only call a const fn with a T: Drop bound with a type that has a const Drop impl.

A notable use case of `impl const` is defining `Drop` impls.
Since const evaluation has no side effects, there is no simple example that
showcases `const Drop` in any useful way. Instead we create a `Drop` impl that
has user visible side effects:

This comment has been minimized.

@ExpHP

ExpHP Feb 19, 2019

I'm confused. Surely this impl should be forbidden due to interior mutability? (i.e. Cell::set would never be const)

This comment has been minimized.

@oli-obk

oli-obk Feb 20, 2019

Author Contributor

There's no reason to forbid Cell::set in const environments (that I know of). The following piece of code has deterministic behavior, so we can theoretically allow it in const contexts.

let x = Cell::new(42);
x.set(5);
assert!(x.get() == 5);

Do you have a problematic example in mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment