Reject unconstrained lifetimes in type_of(assoc_ty) instead of during wfcheck of the impl item #127973
+507
−493
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
aaaed98
to
403b050
Compare
This comment has been minimized.
This comment has been minimized.
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
403b050
to
dbbcbf9
Compare
oli-obk
commented
Jul 22, 2024
| MyFrom::my_from(self.0).ok().unwrap() | ||
| } | ||
| } | ||
|
|
||
| fn main() { | ||
| let _pos: Phantom1<DummyT<()>> = Scope::new().my_index(); | ||
| //~^ ERROR: no method named `my_index` found for struct `Scope` |
There was a problem hiding this comment.
This one is funny and should probably have some more taint tracking to get eliminated (same with the TAIT cycle above)
| pub struct GenericPredicates<'tcx> { | ||
| pub parent: Option<DefId>, | ||
| pub predicates: &'tcx [(Clause<'tcx>, Span)], | ||
| pub effects_min_tys: &'tcx ty::List<Ty<'tcx>>, | ||
| pub tainted_by_errors: Result<(), ErrorGuaranteed>, |
There was a problem hiding this comment.
Explain what it means to be tainted here
There was a problem hiding this comment.
Done. If preferred I can also register a dummy predicate {error}: Sized or sth, that would probably result in similar behaviour
|
FWIW I tried an alternative design that would taint while these can be resolved (see https://github.com/oli-obk/rust/pull/new/uplift_wf_checks3), we still have several new cycles:
|
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
61cec80
to
719d6e8
Compare
This comment has been minimized.
This comment has been minimized.
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
719d6e8
to
5229b23
Compare
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
5229b23
to
ecab78c
Compare
This comment has been minimized.
This comment has been minimized.
oli-obk
commented
Jul 24, 2024
Comment on lines
+637
to
+604
| if let Err(guar) = unsatisfied_predicates.error_reported() { | ||
| return guar; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is necessary to avoid bogus diagnostics like
error[E0599]: the method `my_index` exists for struct `Scope<_>`, but its trait bounds were not satisfied
--> $DIR/ice-failed-to-resolve-instance-for-110696.rs:50:51
|
LL | struct Scope<T>(Phantom2<DummyT<T>>);
| --------------- method `my_index` not found for this struct because it doesn't satisfy `Scope<{type error}>: MyIndex<DummyT<{type error}>>`
...
LL | let _pos: Phantom1<DummyT<()>> = Scope::new().my_index();
| ^^^^^^^^ method cannot be called on `Scope<_>` due to unsatisfied trait bounds
|
note: trait bound `{type error}: Sized` was not satisfied
--> $DIR/ice-failed-to-resolve-instance-for-110696.rs:41:6
|
LL | impl<T: MyFrom<Phantom2<DummyT<U>>>, U> MyIndex<DummyT<T>> for Scope<U> {
| ^ ^ ------------------ --------
| | |
| | unsatisfied trait bound introduced here
| unsatisfied trait bound introduced here
= help: items from traits can only be used if the trait is implemented and in scope
note: `MyIndex` defines an item `my_index`, perhaps you need to implement it
--> $DIR/ice-failed-to-resolve-instance-for-110696.rs:9:1
|
LL | trait MyIndex<T> {
| ^^^^^^^^^^^^^^^^
help: consider relaxing the type parameter's implicit `Sized` bound
|
LL | impl<T: ?Sized + MyFrom<Phantom2<DummyT<U>>>, U> MyIndex<DummyT<T>> for Scope<U> {
| ++++++++
help: consider relaxing the type parameter's implicit `Sized` bound
|
LL | impl<T: MyFrom<Phantom2<DummyT<U>>>, U: ?Sized> MyIndex<DummyT<T>> for Scope<U> {
| ++++++++
Not sure yet how to generally avoid these (I don't want to just hide the predicate, I want to hide the entire error), but this change doesn't make things worse really, so 🤷
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
ecab78c
to
a1ad9fd
Compare
|
☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts. |
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
a1ad9fd
to
4938b6f
Compare
This comment has been minimized.
This comment has been minimized.
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
4938b6f
to
ed019dc
Compare
This comment has been minimized.
This comment has been minimized.
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
ed019dc
to
525f049
Compare
This comment has been minimized.
This comment has been minimized.
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
525f049
to
12a5681
Compare
This comment has been minimized.
This comment has been minimized.
Dylan-DPC
added
S-waiting-on-author
alex-semenyuk
removed
the
S-waiting-on-author
alex-semenyuk
added
the
S-waiting-on-review
Dylan-DPC
added
S-waiting-on-author
… wfcheck of the impl item
…aint the query result
oli-obk
force-pushed
the
uplift_wf_checks
branch
from
12a5681
to
3377c2f
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Of all of the tests you've linked, as far as I can tell only only one of them continues to ICE (#125874). Am I testing them wrong, or is this true? I think we should probably reformulate this PR; it seems a bit too invasive for me to be comfortable with landing it, and I think we can find an approach that requires tracking less state than what this PR does. |
I wonder if we can get most of the way just by stripping lifetimes that are unmentioned in the signature in lowering or generics_of, and then making the error reporting on "unknown lifetime" figure out that it's an unconstrained one |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Opened to discuss as part of https://rust-lang.zulipchat.com/#narrow/stream/144729-t-types/topic/unconstrained.20lifetimes.20in.20impls
Lifetimes are allowed to be unconstrained on impls because it made some libcore macros simpler and it was a breaking change to fix it. This becomes a problem when the lifetime is used in a nonconstraining way (e.g. in an associated item), but in a way that it can be used (e.g. via associated types). Associated functions are not an issue, because they must already have a compatible signature to the trait's assoc function decls, which means unconstrained lifetimes can't be a part of them (except via assoc types).
While the current system is sound, it also doesn't mention in the diagnostic why the lifetime is not permitted when it is permitted in other situations.
Instead of rejecting those lifetimes in wfcheck, we now reject them in
type_ofof associated types. Thus, during hir ty lowering of assoc types, we report any types with unconstrained lifetimes. since this is happening on the hir, we have a fairly good span to report to the user.We could instead walk the hir and look for the offending lifetime (just for improving the span), but I have a deeper reason for this change:
We can now move the remainder of the check from wfcheck to explicit_predicates_of, which already computes the necessary information, and is on a regular query code path, so we can taint it and suppress follow-up errors and ICEs
fixes #123141
fixes #124350
fixes #125874
fixes #126942