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

Add ConstParamTy trait #108161

Merged
merged 17 commits into from
May 2, 2023
Merged

Add ConstParamTy trait #108161

merged 17 commits into from
May 2, 2023

Conversation

WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Feb 17, 2023

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

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 17, 2023
@rustbot

This comment was marked as resolved.

Copy link
Member

@BoxyUwU BoxyUwU left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
library/core/src/marker.rs Outdated Show resolved Hide resolved

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

_ => return Err(ConstParamTyImplementationError::NotAnAdtOrBuiltinAllowed),
Copy link
Member

@BoxyUwU BoxyUwU Feb 17, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@WaffleLapkin WaffleLapkin Mar 3, 2023

Choose a reason for hiding this comment

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

generic_const_parameter_types

Is it okay that this feature does not exist?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 {}

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Mar 3, 2023

@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 Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Mar 9, 2023

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

@pnkfelix
Copy link
Member

pnkfelix commented Apr 27, 2023

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

@rustbot 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
@rust-log-analyzer

This comment has been minimized.

@rustbot 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
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

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

@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin
Copy link
Member Author

@rustbot ready

@rustbot 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
//[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
Copy link
Member

Choose a reason for hiding this comment

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

this is hilarious actually
nice find :)

(`StructuralEq` is shallow for some reason...)
@BoxyUwU
Copy link
Member

BoxyUwU commented May 1, 2023

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

@bors
Copy link
Contributor

bors commented May 1, 2023

📌 Commit c317546 has been approved by BoxyUwU

It is now in the queue for this repository.

@bors 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
bors added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2023
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
@bors bors merged commit b727132 into rust-lang:master May 2, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 2, 2023
/// }
/// ```
#[unstable(feature = "internal_impls_macro", issue = "none")]
macro marker_impls {
Copy link
Member

@Veykril Veykril May 20, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

For future reference: this was changed in #111810

@WaffleLapkin WaffleLapkin deleted the const_param_ty branch May 22, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants