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

Trait bounds not properly propagated in #[derive(SmartPointer)] #127647

Closed
Kixunil opened this issue Jul 12, 2024 · 6 comments
Closed

Trait bounds not properly propagated in #[derive(SmartPointer)] #127647

Kixunil opened this issue Jul 12, 2024 · 6 comments
Labels
C-bug Category: This is a bug. F-derive_coerce_pointee Feature: RFC 3621's oft-renamed implementation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Kixunil
Copy link
Contributor

Kixunil commented Jul 12, 2024

When deriving SmartPointer on a struct with additional bounds the resulting impl is missing a bound resulting in a compile error. This behavior was not explicitly mentioned in SmartPointer RFC, so I assume it's a bug. See #123430

I tried this code: (playground)

#![feature(derive_smart_pointer)]

#[derive(core::marker::SmartPointer)]
#[repr(transparent)]
// Remove the OnDrop bound to make it compile but then the functionality is lost
pub struct Ptr<'a, #[pointee] T: OnDrop + ?Sized, X> {
    data: &'a mut T,
    x: core::marker::PhantomData<X>,
}

pub trait OnDrop {
    fn on_drop(&mut self);
}

I expected to see this happen: the code compiles and provides a smart pointer that executes on_drop when the smart pointer is dropped.

Instead, this happened: compile error:

error[E0277]: the trait bound `__S: OnDrop` is not satisfied
 --> src/lib.rs:3:10
  |
3 | #[derive(core::marker::SmartPointer)]
  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `OnDrop` is not implemented for `__S`
  |
note: required by a bound in `Ptr`
 --> src/lib.rs:6:34
  |
6 | pub struct Ptr<'a, #[pointee] T: OnDrop + ?Sized, X> {
  |                                  ^^^^^^ required by this bound in `Ptr`
  = note: this error originates in the derive macro `core::marker::SmartPointer` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `playground` (lib) due to 1 previous error

Meta

rustc --version --verbose:

1.81.0-nightly
2024-07-11 5315cbe15b79533f380b
@Kixunil Kixunil added the C-bug Category: This is a bug. label Jul 12, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 12, 2024
@tgross35
Copy link
Contributor

Note this is a pretty incomplete feature so there may be some things that just haven't gotten updated yet, but it is good to have the bug report.

@rustbot label +F-derive_smart_pointer +T-compiler -needs-triage

@rustbot rustbot added F-derive_coerce_pointee Feature: RFC 3621's oft-renamed implementation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 12, 2024
@Darksonn
Copy link
Contributor

Thank you for catching this. It looks like we will need this to be fixed before we can use the macro for its intended purpose with linked lists, as the relevant smart pointer is defined with a trait bound:

#[repr(transparent)]
pub struct ListArc<T, const ID: u64 = 0>
where
    T: ListArcSafe<ID> + ?Sized,
{
    arc: Arc<T>,
}

It would make sense to add a test that ensures that the relevant parts of the linked list example compiles. (link 1, link 2).

cc @dingxiangfei2009

@Kixunil
Copy link
Contributor Author

Kixunil commented Jul 13, 2024

Oh, I've just noticed that when you expand the macro the bounds are not applied to the __S parameter which needs them too because it's used as an argument into Ptr generics. It looks like #125575 doesn't copy the bounds (but I have no compiler knowledge so this is just an educated guess). Edit: I've linked it below.

@dingxiangfei2009
Copy link
Contributor

cc @Darksonn I have one open question. There could be more convoluted bounds that involves the pointee type like the following.

#[derive(SmartPointer)]
#[repr(transparent)]
pub struct Ptr<#[pointee] T>
where
    T: Trait<T>,
{ .. }

pub trait Trait<U> { .. }

For now, #127681 does not rewrite T into __S. Should the macro apply this substitution? I feel that the answer is probably "depends".

@Darksonn
Copy link
Contributor

This is what I have in the RFC:

For every trait bound declared on the trait, add it twice to the trait implementation. Once exactly as written, and once with every instance of the #[pointee] parameter replaced with U.

Does that work?

@dingxiangfei2009
Copy link
Contributor

Okay #127681 is updated, so that given a #[pointee] T generic parameter a U: Trait<T> bound is commuted to a U: Trait<__S> bound at both the generic and where-clause position, regardless of whether U is bound or free.

So now

  • struct<#[pointee] T, U: Trait<T>>... generates an impl<T, __S: Unsize<T> + ?Sized, U: Trait<T> + Trait<__S>>
  • struct<#[pointee] T: Trait<T>>... generates an impl<T: Trait<T>, __S: Unsize<T> + ?Sized + Trait<__S>>
  • struct<#[pointee] T: Trait<T>> where SomeType<T>: Trait<T> + ... generates an impl<T: Trait<T>, __S: Unsize<T> + ?Sized> where SomeType<T>: Trait<T> + ..., SomeType<__S>: Trait<__S> + ...

Tests are added for these cases.

tgross35 added a commit to tgross35/rust that referenced this issue Jul 31, 2024
… r=compiler-errors

derive(SmartPointer): rewrite bounds in where and generic bounds

Fix rust-lang#127647

Due to the `Unsize` bounds, we need to commute the bounds on the pointee type to the new self type.

cc `@Darksonn`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 31, 2024
… r=compiler-errors

derive(SmartPointer): rewrite bounds in where and generic bounds

Fix rust-lang#127647

Due to the `Unsize` bounds, we need to commute the bounds on the pointee type to the new self type.

cc ``@Darksonn``
@bors bors closed this as completed in 563f938 Jul 31, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 31, 2024
Rollup merge of rust-lang#127681 - dingxiangfei2009:smart-ptr-bounds, r=compiler-errors

derive(SmartPointer): rewrite bounds in where and generic bounds

Fix rust-lang#127647

Due to the `Unsize` bounds, we need to commute the bounds on the pointee type to the new self type.

cc ```@Darksonn```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-derive_coerce_pointee Feature: RFC 3621's oft-renamed implementation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants