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.

Sign up for GitHub

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

Jump to bottom

ConstParamTy: require Eq as supertrait #116125

Merged
merged 2 commits into from
Sep 26, 2023
Merged

ConstParamTy: require Eq as supertrait #116125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f86c69
fix ICE due to empty span and empty suggestions
RalfJung Sep 24, 2023
ad50963
ConstParamTy: require Eq
RalfJung Sep 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_builtin_macros/src/deriving/bounds.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub fn expand_deriving_const_param_ty(
path: path_std!(marker::ConstParamTy),
skip_path_as_bound: false,
needs_copy_as_bound_if_packed: false,
additional_bounds: Vec::new(),
additional_bounds: vec![ty::Ty::Path(path_std!(cmp::Eq))],
supports_unions: false,
methods: Vec::new(),
associated_types: Vec::new(),
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_resolve/src/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2596,7 +2596,9 @@ fn show_candidates(
);
if let [first, .., last] = &path[..] {
let sp = first.ident.span.until(last.ident.span);
if sp.can_be_used_for_suggestions() {
// Our suggestion is empty, so make sure the span is not empty (or we'd ICE).
// Can happen for derive-generated spans.
if sp.can_be_used_for_suggestions() && !sp.is_empty() {
err.span_suggestion_verbose(
sp,
format!("if you import `{}`, refer to it directly", last.ident),
Expand Down
7 changes: 6 additions & 1 deletion library/core/src/marker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,11 +986,16 @@ pub trait Tuple {}
pub trait PointerLike {}

/// A marker for types which can be used as types of `const` generic parameters.
///
/// These types must have a proper equivalence relation (`Eq`) and it must be automatically
Copy link
Member

Choose a reason for hiding this comment

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

Is this "just in case"? Like, there are no types that implement StructuralEq that don't also implement Eq, right, at least without a manual StructuralEq impl?

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if you could add a test showing where possibly dubious code goes from pass -> err.

Copy link
Member Author

@RalfJung RalfJung Sep 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Like, there are no types that implement StructuralEq that don't also implement Eq, right, at least without a manual StructuralEq impl?

Not quite.
A generic type with derive(Eq) will implement StructuralEq without any bounds on the generic parameters.

Is this "just in case"?

This is mostly to make the 2nd half of the comment coherent: we want to say that valtree equality agrees with ==. Without at least a PartialEq bound, that statement is ill-formed, since the type might not even have ==.

Copy link
Member Author

@RalfJung RalfJung Sep 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Would be nice if you could add a test showing where possibly dubious code goes from pass -> err.

I don't know any such test (without a manual StructualEq impl), but currently the invariants that achieve that ConstParamTy implies Eq are rather subtle. This makes them a lot more obvious.

Copy link
Member

Choose a reason for hiding this comment

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

currently the invariants that achieve that ConstParamTy implies Eq are rather subtle. This makes them a lot more obvious.

Yeah, ok, that's all I wanted to really know. This isn't necessarily for "correctness" (but perhaps only by accident do we imply the right thing due to recursively checking fields for ConstParamTy in impls).

Copy link
Member Author

Choose a reason for hiding this comment

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

I also view this as preparation for removing the StructuralEq trait entirely (at which point only the StructuralPartialEq bound would be left otherwise).

I can do that all in the same PR if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Nah, I'll probably have more questions about behavioral changes on that PR anyways 😅

/// derived (`StructuralPartialEq`). There's a hard-coded check in the compiler ensuring
/// that all fields are also `ConstParamTy`, which implies that recursively, all fields
/// are `StructuralPartialEq`.
#[lang = "const_param_ty"]
#[unstable(feature = "adt_const_params", issue = "95174")]
#[rustc_on_unimplemented(message = "`{Self}` can't be used as a const parameter type")]
#[allow(multiple_supertrait_upcastable)]
pub trait ConstParamTy: StructuralEq + StructuralPartialEq {}
pub trait ConstParamTy: StructuralEq + StructuralPartialEq + Eq {}

/// Derive macro generating an impl of the trait `ConstParamTy`.
#[rustc_builtin_macro]
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/const-generics/adt_const_params/const_param_ty_impl_no_structural_eq.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ struct CantParam(ImplementsConstParamTy);
impl std::marker::ConstParamTy for CantParam {}
//~^ error: the type `CantParam` does not `#[derive(Eq)]`
//~| error: the type `CantParam` does not `#[derive(PartialEq)]`
//~| the trait bound `CantParam: Eq` is not satisfied

#[derive(std::marker::ConstParamTy)]
//~^ error: the type `CantParamDerive` does not `#[derive(Eq)]`
//~| error: the type `CantParamDerive` does not `#[derive(PartialEq)]`
//~| the trait bound `CantParamDerive: Eq` is not satisfied
struct CantParamDerive(ImplementsConstParamTy);

fn check<T: std::marker::ConstParamTy>() {}
Expand Down
35 changes: 32 additions & 3 deletions tests/ui/const-generics/adt_const_params/const_param_ty_impl_no_structural_eq.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
error[E0277]: the trait bound `CantParam: Eq` is not satisfied
--> $DIR/const_param_ty_impl_no_structural_eq.rs:10:36
|
LL | impl std::marker::ConstParamTy for CantParam {}
| ^^^^^^^^^ the trait `Eq` is not implemented for `CantParam`
|
note: required by a bound in `ConstParamTy`
--> $SRC_DIR/core/src/marker.rs:LL:COL
help: consider annotating `CantParam` with `#[derive(Eq)]`
|
LL + #[derive(Eq)]
LL | struct CantParam(ImplementsConstParamTy);
|

error[E0277]: the type `CantParam` does not `#[derive(PartialEq)]`
--> $DIR/const_param_ty_impl_no_structural_eq.rs:10:36
|
Expand All @@ -16,8 +30,23 @@ LL | impl std::marker::ConstParamTy for CantParam {}
note: required by a bound in `ConstParamTy`
--> $SRC_DIR/core/src/marker.rs:LL:COL

error[E0277]: the trait bound `CantParamDerive: Eq` is not satisfied
--> $DIR/const_param_ty_impl_no_structural_eq.rs:15:10
|
LL | #[derive(std::marker::ConstParamTy)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Eq` is not implemented for `CantParamDerive`
|
note: required by a bound in `ConstParamTy`
--> $SRC_DIR/core/src/marker.rs:LL:COL
= note: this error originates in the derive macro `std::marker::ConstParamTy` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider annotating `CantParamDerive` with `#[derive(Eq)]`
|
LL + #[derive(Eq)]
LL | struct CantParamDerive(ImplementsConstParamTy);
|

error[E0277]: the type `CantParamDerive` does not `#[derive(PartialEq)]`
--> $DIR/const_param_ty_impl_no_structural_eq.rs:14:10
--> $DIR/const_param_ty_impl_no_structural_eq.rs:15:10
|
LL | #[derive(std::marker::ConstParamTy)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `StructuralPartialEq` is not implemented for `CantParamDerive`
Expand All @@ -27,7 +56,7 @@ note: required by a bound in `ConstParamTy`
= note: this error originates in the derive macro `std::marker::ConstParamTy` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the type `CantParamDerive` does not `#[derive(Eq)]`
--> $DIR/const_param_ty_impl_no_structural_eq.rs:14:10
--> $DIR/const_param_ty_impl_no_structural_eq.rs:15:10
|
LL | #[derive(std::marker::ConstParamTy)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `StructuralEq` is not implemented for `CantParamDerive`
Expand All @@ -36,6 +65,6 @@ note: required by a bound in `ConstParamTy`
--> $SRC_DIR/core/src/marker.rs:LL:COL
= note: this error originates in the derive macro `std::marker::ConstParamTy` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 4 previous errors
error: aborting due to 6 previous errors

For more information about this error, try `rustc --explain E0277`.
Loading