Deduplicate object safety errors on impl dyn Trait { .. }
#121200
Closed
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
`impl dyn Trait {}` has an implicit `dyn Trait: 'static` bound. We add it in
`inferred_outlives_of` for more context in errors. With this we point at
`impl dyn Trait` and not to it's associated functions, deduplicating errors
to a single one per `impl dyn Trait` instead of one for it plus one for each
associated function.
|
r? @davidtwco rustbot has assigned @davidtwco. Use r? to explicitly pick a reviewer |
rustbot
added
S-waiting-on-review
estebank
changed the title
impl dyn Trait { .. }impl dyn Trait { .. }
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 16, 2024
…r=<try>
Deduplicate object safety errors on `impl dyn Trait { .. }`
`impl dyn Trait {}` has an implicit `dyn Trait: 'static` bound. We add it in `inferred_outlives_of` for more context in errors. With this we point at `impl dyn Trait` and not to it's associated functions, deduplicating errors to a single one per `impl dyn Trait` instead of one for it plus one for each associated function.
CC rust-lang#83246, as this is necessary but not sufficient to help with the lack of tracking of `'static` obligations.
fmease
reviewed
Feb 16, 2024
| | ----- this trait cannot be made into an object... | ||
| LL | const N: usize; | ||
| | ^ ...because it contains this associated `const` | ||
| = help: consider moving `N` to another trait |
There was a problem hiding this comment.
Totally unrelated but if tcx.features().generic_const_items we could suggest alternatively, consider constraining `N` so it does not apply to trait objects // const N: usize where Self: Sized;
compiler-errors
requested changes
Feb 16, 2024
| @@ -46,6 +47,24 @@ fn inferred_outlives_of(tcx: TyCtxt<'_>, item_def_id: LocalDefId) -> &[(ty::Clau | |||
| &[] | |||
| } | |||
| } | |||
| DefKind::Impl { of_trait: false } | |||
There was a problem hiding this comment.
I'd prefer a solution that doesn't involve changing the rules for where we add implicit outlives obligations. This code is really delicate, and I'm worried this could turn out to be unsound without us really knowing why.
Is it possible to instead modify the object safety violation error reporting code to instead prefer pointing at Self type rather than arguments? We can get away with corner cases in diagnostics reporting code, but can't really get away with it in good-path code 😅
There was a problem hiding this comment.
Also, could you explain why adding this outlives obligation actually solves the problem? It's not necessarily clear why this is fixing what it does, and I'm not yet convinced its worth the changes to the inferred_outlives_of function for such a special case.
| // more context in errors. With this we point at `impl dyn Trait` and not to it's | ||
| // associated functions, deduplicating errors to a single one per `impl dyn Trait` instead | ||
| // of one for it plus one for each associated function. | ||
| let ty = tcx.type_of(item_def_id).instantiate_identity(); |
There was a problem hiding this comment.
Also, this should at least be asserting that the implicit object outlives is actually static:
let ty::Dynamic(_, outlives, _) = *ty.kind() else {
bug!("expected dyn self type");
};
assert_eq!(outlives, tcx.regions.re_static);
Then at least if the object outlives bound computation changes down the road, this will ICE.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-author
|
The job Click to see the possible cause of the failure (guessed by this bot) |
I'm not sure if that's actually true in general (*). Just so you know In any case I think it's the wrong approach to look at implicit outlives bounds. #83246 is strictly about default object lifetimes (which I'm fairly acquainted with by now). So any PR that wants to improve diagnostics for them should look at the trait object lifetime directly instead of any derived outlives bounds. (*) I'm saying that I don't know if (pseudo syntax): Γ, 'a0, …, T0, … ⊢ dyn for<'h0, …> Trait<P0('a0), …, Q0(T0), …, R0('h0), …> + 'r
'r ∊ {'static, 'a0, …, 'h0, …}
Pn, Qm, Ro parametrized lifetime/type
-------------------------------------------------------------------------------------
Γ, 'a0, …, T0, … ⊢ dyn for<'h0, …> Trait<P0('a0), …, Q0(T0), …, R0('h0), …> + 'r : 'rSo if |
Dylan-DPC
removed
the
S-waiting-on-perf
|
@estebank |
|
☔ The latest upstream changes (presumably #131511) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
impl dyn Trait {}has an implicitdyn Trait: 'staticbound. We add it ininferred_outlives_offor more context in errors. With this we point atimpl dyn Traitand not to it's associated functions, deduplicating errors to a single one perimpl dyn Traitinstead of one for it plus one for each associated function.CC #83246, as this is necessary but not sufficient to help with the lack of tracking of
'staticobligations.