Add `ConstParamTy` trait #108161

WaffleLapkin · 2023-02-17T13:50:01Z

This is a bit sketch, but idk.
r? @BoxyUwU

Yet to be done:

~~Figure out if it's okay to implement StructuralEq for primitives / possibly remove their special casing~~ (it should be okay, but maybe not in this PR...)
Maybe refactor the code a little bit
Use a macro to make impls a bit nicer

Future work:

Actually™ use the trait when checking if a const generic type is allowed
Really refactor the surrounding code
Refactor marker.rs into multiple modules for each "theme" of markers

BoxyUwU

Thanks this is great :D

Could you move the tests to tests/ui/const-generics/adt_const_params/ apparently this directory doesn't yet exist so you'll have to make it 😅
There's a typo in the current added test const_patam_ty_... rather than const_param_ty_...
I think we ought to have test that each of the special cases in visit_implementations_of_const_param_ty actually has an impl present 🤔 And also tests that when the bounds dont hold on those special cases we correctly get an error. And also that the builtin types that dont have a special case, don't implement ConstParamTy (i.e. FnDef/Closure/FnPtr/&mut/*mut/*const note that this is a non exhaustive list apparently we have a lot of non StructuralEq builtin types x3)
A test that when you have struct Foo(ImplementsConstParamTy) and you try to do impl ConstParamTy for Foo it fails because no StructuralEq would also be good :D

library/core/src/marker.rs

BoxyUwU · 2023-02-17T18:07:27Z

compiler/rustc_trait_selection/src/traits/misc.rs

+
+        &ty::Adt(adt, substs) => (adt, substs),
+
+        _ => return Err(ConstParamTyImplementationError::NotAnAdtOrBuiltinAllowed),


Looking through TyKind I think we need more special cases here 😭, some of them are going to need to evaluate obligations to see if its allowed or not though you can probably use ObligationCtxt for that in this fn 🤔

ty::Str should be usable and probably require [u8]: ConstParamTy to act as if Str was struct Str([u8]).

Should add an impl for impl ConstParamTy for str in core

ty::Array should be usable and require T: ConstParamTy

Should add an impl for impl<T: ConstParamTy, const N: usize> ConstParamTy for [T; N] in core

ty::Slice should be usable and require T: ConstParamTy

Should add an impl for impl<T: ConstParamTy> ConstParamTy for [T] in core

ty::Ref when its Mutability::Not and require T: ConstParamTy

Should add an impl for impl<T: ConstParamTy> ConstParamTy for &'_ T in core

ty::Tuple should be usable and require T: ConstParamTy for all the elements

Should add an impl in core via a macro? How does std do its current impls for tuples xd

I think eventually we want some logic around ty::FnDef and ty::Closure but that is likely going to require builtin impl candidates, and without feature(generic_const_parameter_types) you can't even write an adt_const_param that has one of those types. Could you add a FIXME(generic_const_parameter_types) here to make ty::FnDef/ty::Closure work?

edit: Oh a test that [NotConstParamTy; 0] does NOT implement ConstParamTy from the blanket impl in core would be phenomenal

generic_const_parameter_types

Is it okay that this feature does not exist?

yea its not yet implemented lol but that will be what i call the feature gate when its done 😅

this match needs to actually evaluate obligations checking the things i said should be required so that you cant even write an impl like impl<T> ConstParamTy for &T {}

Wouldn't it duplicate the bounds we have anyway in the core? I don't really see the reason why we need to also check them here (type_allowed_to_implement_copy doesn't, as an example).

yes it would duplicate the bounds we have in core in the same way that if you have a struct Foo<T>(T); the impl needs to write T: ConstParamTy and rustc needs to check that T: ConstParamTy holds. also dont really care if type_allowed_to_implement_copy is a bit buggy and relies on std not doing impls wrong because i'd rather the compiler just not allow you to get this wrong.

My point is: we already depend on std being correct in a myriad of ways, for all intents and purposes from the perspective of the compiler std must be correct.

I don't see why we should rely on std being correct with regards to this, its easily checkable and no where is it clear that implementing Copy or ConstParamTy is actually unsafe inside of std

tests/ui/const-generics/const_patam_ty_impl_bad_field.rs

WaffleLapkin · 2023-03-03T20:02:36Z

@BoxyUwU I think I did almost all the things you've asked for.

I did not implement the trait for tuples, because it kind-of requires refactoring of StructuralEq (tuples currently use a different path in pattern matching and don't implement it...).

If you don't have any objections I would propose to merge this as-is and leave the following tasks as follow-ups (in order in which they should probably be done):

Refactor marker.rs into multiple modules for each "theme" of markers, so it's easier to read & maintain
Refactor const patterns so that they always use StructuralEq (and don't special-case builtins)
Implement ConstParamTy for tuples
Actually™ use ConstParamTy when checking if a const generic type is allowed under feature(adt_const_params)
Really refactor the surrounding code (optional)...
Make StructuralEq not shallow (?maybe) (Add ConstParamTy trait #108161 (comment))

library/core/src/marker.rs

bors · 2023-03-09T03:00:38Z

☔ The latest upstream changes (presumably #108919) made this pull request unmergeable. Please resolve the merge conflicts.

pnkfelix · 2023-04-27T14:26:59Z

Discussed in T-compiler triage meeting today.

@BoxyUwU says they'll likely r+ as-is after its rebased to address the merge conflicts.

@rustbot label: -S-waiting-on-review +S-waiting-on-author

(how did this work before??)

jyn514

I have not looked at the compiler changes but r=me on the new normalization in compiletest

WaffleLapkin · 2023-04-28T12:56:50Z

@rustbot ready

jyn514 · 2023-04-28T14:04:31Z

tests/ui/const-generics/nested-type.rs

+//[min]~^^^^^^^^^^^^ ERROR `[u8; {
+
+// N.B. it is important that the comment above is not inside the array length,
+//      otherwise it may check for itself, instead of the actual error


this is hilarious actually
nice find :)

(`StructuralEq` is shallow for some reason...)

BoxyUwU · 2023-05-01T16:05:42Z

@bors r+ sorry for this PR taking so long D:

bors · 2023-05-01T16:05:44Z

📌 Commit c317546 has been approved by BoxyUwU

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#105076 (Refactor core::char::EscapeDefault and co. structures) - rust-lang#108161 (Add `ConstParamTy` trait) - rust-lang#108668 (Stabilize debugger_visualizer) - rust-lang#110512 (Fix elaboration with associated type bounds) - rust-lang#110895 (Remove `all` in target_thread_local cfg) - rust-lang#110955 (uplift `clippy::clone_double_ref` as `suspicious_double_ref_op`) - rust-lang#111048 (Mark`feature(return_position_impl_trait_in_trait)` and`feature(async_fn_in_trait)` as not incomplete) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Veykril · 2023-05-20T21:14:30Z

library/core/src/marker.rs

+/// }
+/// ```
+#[unstable(feature = "internal_impls_macro", issue = "none")]
+macro marker_impls {


fwiw this kinda broke things with r-a because it's still really bad at handling textual macro scopes in regards to shadowing 🙃 rust-lang/rust-analyzer#14862

We'll try fixing this on our side in a timely manner obviously, but still raising this in case we can't for architectural reasons.

For future reference: this was changed in #111810

rustbot assigned BoxyUwU Feb 17, 2023

This comment was marked as resolved.

Sign in to view

BoxyUwU requested changes Feb 17, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the const_param_ty branch from ceb93df to 4d4d661 Compare March 3, 2023 19:56

BoxyUwU reviewed Mar 6, 2023

View reviewed changes

library/core/src/marker.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2023

WaffleLapkin added 13 commits April 27, 2023 14:37

Implement StructuralEq for integers, bool and char

518d348

(how did this work before??)

Add a ConstParamTy trait

9a716da

derive(Eq) in a test

c8844e1

Remove feature(const_param_ty_trait), use adt_const_params instead

81a2b85

Rename/move a test

c45c4f2

Check the correct trait when checking ConstParamTy impls

1c54410

Add !StructuralEq test for ConstParamTy

7234d63

Add a macro to conveniently implement marker traits

2205c3f

Impl StructuralEq & ConstParamTy for str, &T, [T; N] and [T]

1bf6bbb

Add FIXMEs

2c5e716

Add a test for [NotParam; 0]: ConstParamTy (not holding)

51355ad

Fix some marker impls

bdb5502

Add ConstParamTy tests

26417a8

rust-cloud-vms bot force-pushed the const_param_ty branch from b0dd920 to 26417a8 Compare April 27, 2023 16:02

This comment has been minimized.

Sign in to view

WaffleLapkin added 2 commits April 27, 2023 17:25

Fix compile test so it normalizes (un)expected error messages

2f70d02

--bless ConstParamTy ui tests

1f44a24

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 27, 2023

jyn514 approved these changes Apr 27, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

fixup tests wrt new normalization

182eee2

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 28, 2023

jyn514 reviewed Apr 28, 2023

View reviewed changes

Fix StructuralEq impls for &T, [T] and [T; N]

c317546

(`StructuralEq` is shallow for some reason...)

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2023

Dylan-DPC mentioned this pull request May 2, 2023

Rollup of 7 pull requests #111089

Merged

bors merged commit b727132 into rust-lang:master May 2, 2023
11 checks passed

rustbot added this to the 1.71.0 milestone May 2, 2023

Veykril reviewed May 20, 2023

View reviewed changes

WaffleLapkin deleted the const_param_ty branch May 22, 2023 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ConstParamTy` trait #108161

Add `ConstParamTy` trait #108161

WaffleLapkin commented Feb 17, 2023 •

edited

Loading

This comment was marked as resolved.

BoxyUwU left a comment •

edited by WaffleLapkin

Loading

BoxyUwU Feb 17, 2023 •

edited

Loading

WaffleLapkin Mar 3, 2023 •

edited

Loading

BoxyUwU Mar 3, 2023

BoxyUwU Mar 6, 2023

WaffleLapkin Mar 7, 2023

BoxyUwU Mar 7, 2023

WaffleLapkin Mar 8, 2023

BoxyUwU Mar 8, 2023

This comment has been minimized.

WaffleLapkin commented Mar 3, 2023 •

edited

Loading

bors commented Mar 9, 2023

pnkfelix commented Apr 27, 2023 •

edited

Loading

This comment has been minimized.

jyn514 left a comment

This comment has been minimized.

WaffleLapkin commented Apr 28, 2023

jyn514 Apr 28, 2023

BoxyUwU commented May 1, 2023

bors commented May 1, 2023

Veykril May 20, 2023 •

edited

Loading

WaffleLapkin May 22, 2023


		&ty::Adt(adt, substs) => (adt, substs),

		_ => return Err(ConstParamTyImplementationError::NotAnAdtOrBuiltinAllowed),

Add ConstParamTy trait #108161

Add ConstParamTy trait #108161

Conversation

WaffleLapkin commented Feb 17, 2023 • edited Loading

This comment was marked as resolved.

BoxyUwU left a comment • edited by WaffleLapkin Loading

Choose a reason for hiding this comment

BoxyUwU Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

WaffleLapkin Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

WaffleLapkin commented Mar 3, 2023 • edited Loading

bors commented Mar 9, 2023

pnkfelix commented Apr 27, 2023 • edited Loading

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

WaffleLapkin commented Apr 28, 2023

Choose a reason for hiding this comment

BoxyUwU commented May 1, 2023

bors commented May 1, 2023

Veykril May 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `ConstParamTy` trait #108161

Add `ConstParamTy` trait #108161

WaffleLapkin commented Feb 17, 2023 •

edited

Loading

BoxyUwU left a comment •

edited by WaffleLapkin

Loading

BoxyUwU Feb 17, 2023 •

edited

Loading

WaffleLapkin Mar 3, 2023 •

edited

Loading

WaffleLapkin commented Mar 3, 2023 •

edited

Loading

pnkfelix commented Apr 27, 2023 •

edited

Loading

Veykril May 20, 2023 •

edited

Loading