Tracking issue for RFC #2056: Allow trivial constraints to appear in where clauses

[aturon]

This is a tracking issue for the RFC "Allow trivial constraints to appear in where clauses " (rust-lang/rfcs#2056).

Steps:

• Implement the RFC – Implement RFC 2056 trivial constraints in where clauses #48557
  • As noted in Tracking issue for RFC #2056: Allow trivial constraints to appear in where clauses  #48214 (comment), the new trait solver might solve a few issues with this feature, so this is currently blocked on the new trait solver (-Znext-solver)
• Adjust documentation (see instructions on forge)
• Stabilization PR (see instructions on forge)

Unresolved questions:

• Should the lint error by default instead of warn?

[matthewjasper]

I'm working on this.

[nikomatsakis]

@matthewjasper ok. I've not thought hard about what it will take to support this. I had thought about doing it after making some more progress on general trait refactoring, but if it can be easily supported today seems fine.

[nikomatsakis]

Oh, I meant to add, please ping me on IRC/gitter with any questions! I can try to carve out some time to think about it as well.

[matthewjasper]

So with #51042 merged this feature is implemented on nightly, but the implementation isn't particularly clean and there are some edge cases that don't work as one would expect:

#![feature(trivial_bounds)]

struct A;
trait X<T> {
    fn test(&self, t: T) {}
}

impl X<i32> for A {}

fn foo(a: A) where A: X<i64> {
    a.test(1i64); // expected i32, found i64
}

Since Chalk might be able to fix some of these issues, I think that this feature is now waiting for Chalk to be integrated.

edit: As of 2018-12-28 -Zchalk does not currently solve this issue.

[RalfJung]

As another concrete example where trivially false constraints arise due to macros, see taiki-e/pin-project#102 (comment). That crate now carries a weird hack to work around this limitation.

What is the current status of this feature?

[matthewjasper]

Not much has changed since my last comment.

[Systemcluster]

The current implementation generates a warning when using the pre-existing impl trait syntax, which, reading the RFC, I assume is a false-positive.

#![allow(dead_code, unused_variables,)]
#![feature(trait_alias, trivial_bounds)]

trait T = Fn(&i32, &i32) -> bool;
fn impl_trait_fn() -> impl T {
	|a: &i32, b: &i32| true
}

This code generates the warning warning: Trait bound impl T: T does not depend on any type or lifetime parameters. (playground link)

[Luro02]

I think this change could cause problems with some proc-macros that rely on

struct _AssertCopy where String: ::core::marker::Copy;

to not compile, like @ExpHP wrote in rust-lang/rfcs#2056

To play devil's advocate, I do have to wonder if some crates may have popped up since 1.7 that rely on these errors as a form of static assertion about what traits are implemented (as the author was likely unaware that trivially true bounds were intended to produce compiler errors). Out of concern for this, I might lean towards "warn by default", since macros can easily disable the warnings in their output.

but I can not comprehend why this should be warn by default as this would break existing proc-macros that rely on this behavior?

I can say for a fact that there are definitely crates that rely on this behavior, because I recently published my first proc-macro (shorthand), which does rely on this behavior and I saw those assertions (struct __AssertCopy where String: ::core::marker::Copy;) in some crates and adopted it into my own crate.

I think most of those assertions are only used to make better error messages.

For example my proc-macro emits

struct _AssertCopy where String: ::core::marker::Copy;

which is spanned around the field of a struct and this creates this error message:

error[E0277]: the trait bound `std::string::String: std::marker::Copy` is not satisfied
 --> $DIR/not_copy.rs:6:12
  |
6 |     value: String,
  |            ^^^^^^ the trait `std::marker::Copy` is not implemented for `std::string::String`
  |
  = help: see issue #48214
  = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable

I would say this lint should be deny by default to keep backwards compatibility.

[eddyb]

@Luro02 What else do you generate? Does your proc macro use unsafe and would it do the wrong thing without that "doesn't implement Copy" error?

If so why not put the bound on some impl or fn that would misbehave?

That would actually protect it, unlike an unrelated and unused type definition.

(Alternatively, just use _AssertCopy somewhere in your unsafe code)

[Luro02]

@eddyb Nope, in my case it is not using any unsafe code and it would not do the wrong thing, but who knows some macro might use this as a guard. For example one could have an attribute like this:

#[only(copy)]
struct Example {
    field: String, // error because not copy
}

this would silently be accepted.

Alternatively, just use _AssertCopy somewhere in your unsafe code

does this mean, that trivial bounds do not compile in unsafe code? 🤔

The unrelated/unused type definiton is there to get a specific error message (like String does not implement Copy instead of can not move String because it does not implement Copy)

If so why not put the bound on some impl or fn that would misbehave?

I think this would still compile/ignore the bound
fn value(&self) where String: Copy and it is a bit more difficult, than simply emitting a line in the function body.

[dtolnay]

I think the suggestion to "just use the bound somewhere in code" means emitting something like:

struct _AssertCopy where String: ::core::marker::Copy;
let _: _AssertCopy;

There is no way this would compile with an unsatisfied bound.

[eddyb]

does this mean, that trivial bounds do not compile in unsafe code? thinking

No, @dtolnay explained what I meant. I was expecting you had unsafe code, but it sounds like you're just using it to get better diagnostics, which makes it less tricky of a situation.

I think this would still compile/ignore the bound

If it doesn't error when compiling the function, it will error when trying to use it.
It's not going to let you call the function without erroring.

[jjpe]

Just ran into this issue while implementing a derive macro Delta that derives a trait DeltaOps that:

1. has an associated type DeltaOps::Delta
2. has nontrivial bounds on Self
3. has nontrivial bounds on DeltaOps::Delta
4. and is implemented for (among others) mutually recursive enums.

That caused an overflow in the compiler, with compile errors like this one:

error[E0275]: overflow evaluating the requirement `rule::Expr: struct_delta_trait::DeltaOps`
  --> src/parser.rs:26:10
   |
26 | #[derive(struct_delta_derive::Delta)]
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: required because of the requirements on the impl of `struct_delta_trait::DeltaOps` for `std::vec::Vec<rule::Expr>`
   = note: required because of the requirements on the impl of `struct_delta_trait::DeltaOps` for `rule::Rule`
   = note: required because of the requirements on the impl of `struct_delta_trait::DeltaOps` for `std::borrow::Cow<'static, rule::Rule>`
   = note: required because of the requirements on the impl of `struct_delta_trait::DeltaOps` for `stack::StackFrame`
   = note: required because of the requirements on the impl of `struct_delta_trait::DeltaOps` for `std::vec::Vec<stack::StackFrame>`
   = note: required because of the requirements on the impl of `struct_delta_trait::DeltaOps` for `stack::Stack`
   = help: see issue #48214
   = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

As you can see, the compiler gave me a suggestion to use #![feature(trivial_bounds)] in my crate.
My question is, will that actually work i.e. will it solve the overflow problem with bounds checking on recursive types without breaking stable functionality?

[Ekleog]

No, I meant that the macro generates a where clause with trait bounds with a wrapper type.

Oh ok I think I understand now!

AFAIK, what serde actually does is the same as what the derive macros in the standard library do: they refer to type parameters in generics rather than field types. Therefore, there is no need for a filter/visitor. (see #26925 for more)

Hmm I guess the choice is then currently between having the issues listed in #26925 and being unable to derive on recursive structs, until someone designs a way to filter only on types that depend on actual generic arguments or this issue is fixed. Or to use the wrapper type trick to delay execution, but that probably can't be generalized to any crate.

I'm curious, do you know why the default with proc-macro + syn seems to be "unable to derive on recursive structs"? (I'm referring to the fact that this simple code, where generics comes straight from DeriveInput, seems to set default where bounds based on field types)

[taiki-e]

Hmm I guess the choice is then currently between having the issues listed in #26925 and being unable to derive on recursive structs, until someone designs a way to filter only on types that depend on actual generic arguments or this issue is fixed. Or to use the wrapper type trick to delay execution, but that probably can't be generalized to any crate.

Well, even if the filter were implemented, I don't think the approach of using field types at the bounds of the where clause would work in some cases. e.g., that approach still does not interact well with structs that have private types. (see #48054 for more. there is a workaround for that problem as well, but IIRC that workaround only works with auto-traits)

EDIT: see also dtolnay/syn#370

this simple code

Those lines are not causing the problem. The problem is that the way of changing the where clause in the generate_fields_type_gen does not work well with the current Rust's type system.

[CAD97]

it'd be a breaking change

Just to clarify the space here a bit,

• Turning a trait implementation which errors into a trait implementation which compiles but is unsatisfiable / never applies is unfortunate, but not breaking.
• Turning a trait implementation which errors into a trait implementation which compiles and is satisfiable / can be utilized is a breaking change, though arguably a bug fix and allowable.

It becomes breaking because code generation could rely on code not compiling to enforce soundness constraints. However, this applies to literally any change which results in more code compiling; the code generation which is broken by such a change was only tenuously sound in the first place, as it was relying on a satisfiable bound actually being unsatisfiable (an error).

The former case is provably sound and does not break; the impl is bounded on the condition which makes the impl sound, and if that condition does not hold, the impl does not apply. The "minor breaking" change is that the unsatisfiable impl goes from an immediate error on definition to only an error when trying to use the impl; this delayed communication is why I've suggested that trivially false bounds could be a deny-by-default lint.

[Ekleog]

@taiki-e

Those lines are not causing the problem. The problem is that the way of changing the where clause in the generate_fields_type_gen does not work well with the current Rust's type system.

Oh… I'm stupid I had missed the fact that make_where_clause returns a mutable reference… Sorry for the noise, and thank you!

(And so, for @CAD97 this explains why I was saying it was a breaking change in my message: I was thinking this behavior was coming from either syn or rustc, which was a wrong assumption)

[joshtriplett]

I'm trying to figure out the current state of this feature. What's the current status of trivial_bounds? Are there still blocking issues that would prevent this from being stabilized? Or does this just need a stabilization report and FCP?

The last update I found about blocking concerns, across the several issues about this, seems to be from 2019 talking about limitations in chalk.

[dhedey]

Just to add, I approve of the attributes here: #48214 (comment)

I want to conditionally implement a Trait in a derive macro on a "transparent" wrapper style new-type depending on if a given Trait is implemented on the inner type. Naively making use of a where <concrete_inner_type>: Trait would allow for this behaviour, but the warns/compile errors get in the way.

I'd love to be able to add the where clause and for the above to just work - and the two annotations would allow this to work as-expected:

 #[allow(trivially_false_bounds)]
 #[allow(trivially_true_bounds)]

This would allow an easy way for macro-writers to avoid the problem of handling generics and avoiding #26925

As a work-around in my case, I'll try introducing some new trait Wrapped with associated type Inner = X and add a blanket impl of Trait for Wrapped<Inner: Trait> - but other cases don't have such clear work-arounds. EDIT: Doesn't really work because standard impls of Trait fail with:

conflicting implementations of trait `Trait` for type `T`
downstream crates may implement trait `Wrapped<_>` for type `T`

EDIT: Looks like this workaround should work: #48214 (comment)

[cybersoulK]

ran into this issue when trying to serialize my type with:

#[derive(borsh::BorshSerialize, borsh::BorshDeserialize)]
struct MyType {
   range: RangeInclusive<u32>
   ...
}

[joshlf]

Chiming in to provide another use case: zerocopy also falls into the category of crates which rely on trivially-false bounds failing in order to guarantee that our derive-emitted code is sound. E.g., all of the derives in this file should fail, but with trivial_bounds enabled, only one of them fails. As long as there's some way for us to fail compilation when a certain type doesn't implement a certain trait, then we're fine. #[deny(trivially_false_bounds)] sounds like it'd work, but from our perspective it doesn't matter if that's the solution or if a different solution is chosen.

One potential concern is that we already have versions of our crate published which don't use #[deny(trivially_false_bounds)], so depending on how this feature is stabilized, it could result in those versions of our crate becoming unsound.

[cramertj]

@joshlf To clarify, with trivial_bounds enabled, those derives will not trigger a compilation failure, but the resulting unsound trait implementations also won't be present because their bound is false, correct?

[RalfJung]

@joshlf if what you really want is "check if type T implements trait Trait, then I'd express this like this:

fn test_trait(_x: impl Trait) {}
fn test_t_trait(x: T) { test_trait(x) }

It doesn't seem necessary to rely on trivial trait bounds for that?

[joshlf]

Apologies, it looks like I misunderstood what this feature did: I was incorrectly assuming that trivial bounds were simply removed. I probably should have realized that that behavior makes no sense lol.

In case it's useful, here's the error message that I saw that made me think "oh, if I enable trivial_bounds, this trait impl will start compiling." While that's technically true, I didn't realize it'd compile because the trait would be considered unimplemented. Maybe there's a way to make this more clear in the error message?

error[E0277]: the trait bound `HasPadding<AsBytes2, true>: ShouldBe<false>` is not satisfied
  --> tests/ui-nightly/struct.rs:23:10
   |
23 | #[derive(AsBytes)]
   |          ^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<AsBytes2, true>`
   |
   = help: the trait `ShouldBe<true>` is implemented for `HasPadding<AsBytes2, true>`
   = help: see issue #48214
   = help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
   = note: this error originates in the derive macro `AsBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

[joshlf]

I may have found a bug. I changed the PR linked above to use static_assertions::assert_impl_all! to confirm that even if the derive compiles successfully, it doesn't emit an unsound impl. See e.g. this line. However, the assert_impl_all! invocations seem to succeed (see the compiler error output*). It seems that the traits are not actually implemented (see *), but I would expect the assert_impl_all! invocations to fail.

* There is one error in the error output; that's due to the fact that we derive FromBytes, which is a sub-trait of FromZeroes, which is not implemented.

[RalfJung]

How does assert_impl_all! work?

[joshlf]

How does assert_impl_all! work?

It calls assert_impl! once for each trait:

macro_rules! assert_impl {
    (for($($generic:tt)*) $ty:ty: $($rest:tt)*) => {
        const _: () = {
            fn assert_impl<$($generic)*>() {
                // Construct an expression using `True`/`False` and their
                // operators, that corresponds to the provided expression.
                let _: $crate::True = $crate::_does_impl!($ty: $($rest)*);
            }
        };
    };
    ($ty:ty: $($rest:tt)*) => {
        // Construct an expression using `True`/`False` and their operators,
        // that corresponds to the provided expression.
        const _: $crate::True = $crate::_does_impl!($ty: $($rest)*);
    };
}

does_impl! is defined here:

/// Returns `True` or `False` depending on whether the given type implements the
/// given trait boolean expression. Can be used in const contexts if it doesn't
/// depend on outer generic parameters.
///
/// This is the core of `assert_impl`.
#[doc(hidden)]
#[macro_export(local_inner_macros)]
macro_rules! _does_impl {
    ($ty:ty: $($rest:tt)*) => {{
        #[allow(unused_imports)]
        use $crate::{
            _bool::{True, False},
            _core::{marker::PhantomData, ops::Deref},
        };

        // Fallback trait that returns false if the type does not implement a
        // given trait.
        trait DoesntImpl {
            const DOES_IMPL: False = False;
        }
        impl<T: ?Sized> DoesntImpl for T {}

        // Construct an expression using `True`/`False` and their operators,
        // that corresponds to the provided expression.
        *_does_impl!(@boolexpr($ty,) $($rest)*)
    }};

    (@boolexpr($($args:tt)*) ($($expr:tt)*)) => {
        _does_impl!(@boolexpr($($args)*) $($expr)*)
    };
    (@boolexpr($($args:tt)*) !($($expr:tt)*)) => {
        _does_impl!(@boolexpr($($args)*) $($expr)*).not()
    };
    (@boolexpr($($args:tt)*) ($($left:tt)*) | $($right:tt)*) => {{
        let left = _does_impl!(@boolexpr($($args)*) $($left)*);
        let right = _does_impl!(@boolexpr($($args)*) $($right)*);
        left.or(right)
    }};
    (@boolexpr($($args:tt)*) ($($left:tt)*) & $($right:tt)*) => {{
        let left = _does_impl!(@boolexpr($($args)*) $($left)*);
        let right = _does_impl!(@boolexpr($($args)*) $($right)*);
        left.and(right)
    }};
    (@boolexpr($($args:tt)*) !($($left:tt)*) | $($right:tt)*) => {{
        _does_impl!(@boolexpr($($args)*) (!($($left)*)) | $($right)*)
    }};
    (@boolexpr($($args:tt)*) !($($left:tt)*) & $($right:tt)*) => {{
        _does_impl!(@boolexpr($($args)*) (!($($left)*)) & $($right)*)
    }};
    (@boolexpr($($args:tt)*) !$left:ident | $($right:tt)*) => {{
        _does_impl!(@boolexpr($($args)*) !($left) | $($right)*)
    }};
    (@boolexpr($($args:tt)*) !$left:ident & $($right:tt)*) => {{
        _does_impl!(@boolexpr($($args)*) !($left) & $($right)*)
    }};
    (@boolexpr($($args:tt)*) $left:ident | $($right:tt)*) => {
        _does_impl!(@boolexpr($($args)*) ($left) | $($right)*)
    };
    (@boolexpr($($args:tt)*) $left:ident & $($right:tt)*) => {{
        _does_impl!(@boolexpr($($args)*) ($left) & $($right)*)
    }};
    (@boolexpr($($args:tt)*) !$expr:ident) => {
        _does_impl!(@boolexpr($($args)*) !($expr))
    };
    (@boolexpr($($args:tt)*) !$expr:path) => {
        _does_impl!(@boolexpr($($args)*) !($expr))
    };
    (@boolexpr($($args:tt)*) $expr:ident) => {
        _does_impl!(@base($($args)*) $expr)
    };
    (@boolexpr($($args:tt)*) $expr:path) => {
        _does_impl!(@base($($args)*) $expr)
    };

    (@base($ty:ty, $($args:tt)*) $($trait:tt)*) => {{
        // Base case: computes whether `ty` implements `trait`.
        struct Wrapper<T: ?Sized>(PhantomData<T>);

        #[allow(dead_code)]
        impl<T: ?Sized + $($trait)*> Wrapper<T> {
            const DOES_IMPL: True = True;
        }

        // If `$type: $trait`, the `_does_impl` inherent method on `Wrapper`
        // will be called, and return `True`. Otherwise, the trait method will
        // be called, which returns `False`.
        &<Wrapper<$ty>>::DOES_IMPL
    }};
}

[RalfJung]

That macro looks... interesting:

        const _: () = {
            fn assert_impl<$($generic)*>() {
                // Construct an expression using `True`/`False` and their
                // operators, that corresponds to the provided expression.
                let _: $crate::True = $crate::_does_impl!($ty: $($rest)*);
            }
        };

This is a generic function that is never called. Looks like with this feature we no longer type-check it properly?

I don't know what we guarantee about how much we type-check never-called generic functions.

It would help to distill the behavior you are seeing to a small macro-free example.

[CatsAreFluffy]

Is the "Chalk" mentioned in the original post related to/the same as the new solver enabled by -Znext-solver? (Also, the example in #48214 (comment) works in the current nightly with -Znext-solver but not without it.)

[matthewjasper]

Yes, I've updated op for that

[cramertj]

#![feature(trivial_bounds)] only required where bounds are... trivial

Maybe this has been obvious to others, but it wasn't to me! The trivial_bounds feature gate and warning are actually only needed in cases where the bound includes no type or lifetime parameters. One can imitate the same behavior by simply adding a type or lifetime parameter to the bound artificially.

That is, this does not compile on stable today:

fn iterate_over_u8(mut x: u8) where u8: Iterator<Item = String> {
    while let Some(s) = (&mut x).next() { println!("{s}") }
}

but this does:

fn iterate_over_u8<'a>(mut x: &'a u8) where &'a u8: Iterator<Item = String> {
    while let Some(s) = (&mut x).next() { println!("{s}") }
}

Both are similarly nonsensical bounds, but the former compiles because the bound isn't evaluated until 'a is substituted at a callsite. This means that my bounds generation example above does not require this feature:

Amended bounds generation example

// BEGIN GENERATED CODE
struct S;

#[diagnostic::on_unimplemented(message = "binding generation of `Clone for S` failed because ...")]
trait CloneForSBindingGenerationFailure {}

impl<'a> Clone for S where &'a (): CloneForSBindingGenerationFailure {
    fn clone(&self) -> Self {
        unreachable!()
    }
}
// END GENERATED CODE

fn main() {
    fn take_clone(_: impl Clone) {}
    take_clone(S); // error[E0277]: binding generation of `Clone for S` failed because ...
}