-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
mGCA: Make trait object types with type-level associated consts dyn compatible if all of the latter are specified via bindings #150843
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
base: main
Are you sure you want to change the base?
Conversation
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. This PR modifies |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
mGCA: Make trait object types with type associated consts dyn compatible if the latter are specified via bindings
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
mGCA: Make trait object types with type associated consts dyn compatible if the latter are specified via bindings
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
mGCA: Move tests for assoc const bindings (formerly ACE) into dedicated directory & replace more mentions of ACE Split out of PR #150843. As discussed. Somewhat obvious underlying principle: If the test checks basic or core parts of assoc const bindings and nothing else, move it, otherwise leave it even if it contains ACEs. Motivation: It makes a lot easier for me to continue working on ACE efficiently. r? @BoxyUwU
mGCA: Move tests for assoc const bindings (formerly ACE) into dedicated directory & replace more mentions of ACE Split out of PR rust-lang/rust#150843. As discussed. Somewhat obvious underlying principle: If the test checks basic or core parts of assoc const bindings and nothing else, move it, otherwise leave it even if it contains ACEs. Motivation: It makes a lot easier for me to continue working on ACE efficiently. r? @BoxyUwU
mGCA: Move tests for assoc const bindings (formerly ACE) into dedicated directory & replace more mentions of ACE Split out of PR rust-lang/rust#150843. As discussed. Somewhat obvious underlying principle: If the test checks basic or core parts of assoc const bindings and nothing else, move it, otherwise leave it even if it contains ACEs. Motivation: It makes a lot easier for me to continue working on ACE efficiently. r? @BoxyUwU
| // Collect all associated items that weren't specified and compute the list of projection | ||
| // bounds which we'll later turn into existential ones. | ||
| // For why we keep projections whose associated item has a `Self: Sized` bound, | ||
| // see <https://github.com/rust-lang/rust/pull/140684>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we say sth like "we keep them around so that we the resulting ty after HIR ty lowering still contains the user written types on the rhs of the assoc item binding, see https://github.com/...". clicking the link is annoying 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now say:
We intentionally keep around projections whose associated item has a
Self: Sized
bound in order to be able to wfcheck the RHS, allow the RHS to constrain generic
parameters and to imply bounds.
See also #140684.
| } | ||
|
|
||
| fn main() { | ||
| let _: dyn Trait; //~ ERROR the trait `Trait` is not dyn compatible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add another case with dyn Trait<K = 0> and check that errors
| @@ -0,0 +1,27 @@ | |||
| // Ensure that we can successfully mangle & demangle trait object types w/ assoc const bindings. | |||
|
|
|||
| // FIXME(fmease): Legacy mangling still crashes. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME(mgca)
i wanna know this when looking at mgca issues not jsut when looking at your FIXMEs :3
|
|
||
| // This is a run-pass test to ensure that codegen can actually deal with such method instances | ||
| // (e.g., const projections normalize flawlessly to something concrete, symbols get mangled | ||
| // properly properly) and to ensure that the generated code "received" the correct values from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
properly properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked over all the commits, got some nits and you've still got the other thing to fix with type consts' types having Self in them until approval though
...but require all assoc consts to be specified via bindings.
We used to lower such bad defaulted const args in trait object types to
`{type error}`; now correctly lower them to `{const error}`.
The added tests used to ICE prior to this change.
…would otherwise ref `Self` The added test used to ICE prior to this change.
This fully rewords the diagnostic that was previously only emitted for assoc ty bindings. That's because it incorrectly called trait aliases *type aliases* and didn't really make it clear what the root cause is. The added test used to ICE prior to this change. I've double-checked that the preexisting test I've modified still ICEs in nightly-2025-03-29.
… `Self` (barring `Self` projections)
|
I've addressed all review comments (in one way or another). I've now added a 10th commit in which I render traits dyn incompatible if the type of the type assoc const mentions the Ready for rereview. |
Under feature
min_generic_const_args(mGCA) (#132980), render traits with non-parametrized type-level associated constants (i.e.,#[type_const]ones) dyn compatible but force the user to specify all type-level associated consts in the trait object type via bindings (either directly, via supertrait bounds and/or behind trait aliases) just like associated types, their sibling.Fixes #130300 (feature request).
Fixes #136063 (bug).
Fixes #137260 (bug).
Fixes #137514 (bug).
While I'm accounting for most illegal
Selfreferences via const projections & params, I'm intentionally ignoring RUST-123140 (and duplicates) in this PR which is to be tackled some other time.Additional context: Crate
rustc-demanglehad to be updated to fix v0 demangling. I've patched it in PR rust-lang/rustc-demangle#87 which was was released in version 0.1.27 via PR rust-lang/rustc-demangle#88.