Prohibit conflicting bounds in dyn, even with different generics.#157710
Prohibit conflicting bounds in dyn, even with different generics.#157710theemathas wants to merge 6 commits into
dyn, even with different generics.#157710Conversation
7792410 to
67244f1
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@bors try |
This comment has been minimized.
This comment has been minimized.
Prohibit conflicting bounds in `dyn`, even with different generics.
This comment has been minimized.
This comment has been minimized.
Prohibit conflicting bounds in `dyn`, even with different generics.
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot cancel See #157814 |
|
🗑️ Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
67244f1 to
7251016
Compare
This comment has been minimized.
This comment has been minimized.
7251016 to
b5838a9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // FIXME: Improve diagnostics by pointing to | ||
| // where the bound is specified. | ||
| .with_note(format!("`{name}` is specified to be `{old_term}`")) | ||
| .with_note(format!("`{name}` is also specified to be `{term}`")) | ||
| .emit(); |
There was a problem hiding this comment.
How can I get the span where the associated type bound is specified (e.g., in the supertrait clause)?
| // This clause specifies that the `kind` is equal to `term`. | ||
| // We record this, and check for duplicates. | ||
| if let Some((old_term, old_pred)) = | ||
| seen_projection_bounds_ignoring_generics.insert(kind, (term, pred)) | ||
| && old_term != term |
There was a problem hiding this comment.
The one crater regression effectively had a : Trait<i32, Assoc = i32> + Trait<bool, Assoc = bool> supertrait bound. I want to allow this. To do so, I need to check whether an older predicate's generics could be unified with the new predicate's generics. How can I do this? I found the can_eq method, but I don't know how to obtain the param_env argument required for it.
There was a problem hiding this comment.
As alluded to in #146548 (comment) and #133397, we don't really have access to semantic equality (normalization followed by type relating) in HIR ty lowering (specifically in item signatures).
can_eq is an overapproximation; anyway, obtaining the ParamEnv (tcx.param_env(self.item_def_id())) is basically impossible in item signatures due to query cycles galore (e.g., it needs to contain implied bounds but implicit_infer needs already-lowered types: cycle).
For inherent associated types, we currently use DeepRejectCtxt::…::types_may_unify but that's icky because DeepRejectCtxt is only meant for fast paths but now it leaks into the semantics of the language. The plan is to get rid of that again (...).
What's left is == which obviously doesn't properly handle infers _, bound vars and alias types. That might be the best option, unfortunately. I don't know how T-types thinks about this...
There was a problem hiding this comment.
Checking with == is unsound, since that would make two different generic type variables compare as unequal types, which will allow rust code that runs into the unsoundness this PR was trying to fix in the first place.
What I need is an operation that can tell me either "it's impossible for these two types to ever have their generics instantiated so that they become the same concrete type" or "unsure".
There was a problem hiding this comment.
Of course 🤦.
That's ty::DeepRejectCtxt::relate_infer_infer(tcx).args_may_unify(…) but it's questionable if T-types is going to accept that for the aforesaid reason 🤔 .
There was a problem hiding this comment.
Sounds like it's either we use some hacky thing, or we break one crate in crater (and possibly an unknown number of crates that crater can't see) with what appears to be a legimitate use case.
Do we do an I-types-nominated?
|
I am unsure what I should do with the tests that now no longer compile. Any suggestions? Also, I'm very unsure if I handled the binder stuff correctly. I'd like a closer look on those stuff. Also, what's up with the build failures in CI? It builds fine locally. r? @fmease |
|
HIR ty lowering was modified cc @fmease |
…ics. See rust-lang#154662 These tests will be fixed in a subsequent commit.
Fixes rust-lang#154662 In a `dyn` type, if multiple bounds are specified for the same associated item, then we previously accepted them if the relevant trait's generics are different, even if the bounds conflict. This was unsound, since those generics could end up being instantiated with identical concrete types, causing the `dyn` type to have two different "values" for the same bound. Thus, we prohibit multiple bounds for the same associated item, even if the trait's generics are different. As a side effect of this change, we also now allow duplicated bounds in a `dyn` type, as long as the bounds are syntactically identical. The now-unused `OverlappingAsssocItemConstraints` will be removed in a subsequent commit.
Fixes <rust-lang#150936> The last place where `OverlappingAsssocItemConstraints::Forbidden` was constructed was removed in a previous commit. As a result, this code path is now unreachable.
7a7f34c to
8bbea5e
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I've rebased. The previous CI failures were due to #157653 (since CI has to merge with the main branch before building). This has now been resolved. |
This comment has been minimized.
This comment has been minimized.
…ciated types in dyn. This issue previously only affected trait aliases. This PR causes the same issue to also affect supertraits.
0c0275a to
a186b79
Compare
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
View all comments
Fixes #154662, fixes #150936.
In a
dyntype, if multiple bounds are specified for the same associated item, then we previously accepted them if the relevant trait's generics are different, even if the bounds conflict.This was unsound, since those generics could end up being instantiated with identical concrete types, causing the
dyntype to have two different "values" for the same bound.Thus, we prohibit multiple bounds for the same associated item, even if the trait's generics are different.
As a side effect of this change, we also now allow duplicated bounds in a
dyntype, as long as the bounds are syntactically identical.