[experiment] fulfill diagnostics: do not look into T: IntoIterator
#99719
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
T-compiler
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
This comment has been minimized.
This comment has been minimized.
eddyb
reviewed
Jul 25, 2022
compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs
Outdated
Show resolved
Hide resolved
eddyb
reviewed
Jul 25, 2022
| let Some(trait_ref) = tcx.impl_trait_ref(id) else { | ||
| span_bug!(tcx.def_span(id), "expected impl, found `{:?} for `{:?}`", tcx.def_kind(id), id); | ||
| }; | ||
| tcx.uses_unique_generic_params(trait_ref.substs, IgnoreRegions::No).is_ok() |
There was a problem hiding this comment.
Huh, fascinating, never seen this before.
eddyb
reviewed
Jul 25, 2022
| @@ -1412,6 +1412,10 @@ rustc_queries! { | |||
| separate_provide_extern | |||
| } | |||
|
|
|||
| query is_general_impl(def_id: DefId) -> bool { | |||
There was a problem hiding this comment.
I wonder if this needs to use a qualifier like "fully" or "maximally" to apply to "general" (I suppose "general" is better than "blanket"? at least a doc comment here could explain the connection).
eddyb
reviewed
Jul 25, 2022
Comment on lines
+143
to
+154
| match self.evaluate_candidate(stack, &c) { | ||
| Ok(eval) if eval.may_apply() => Ok(Some(c)), | ||
| Ok(_) => Ok(None), | ||
| Err(OverflowError::Canonical) => Err(Overflow(OverflowError::Canonical)), | ||
| Err(OverflowError::ErrorReporting) => Err(ErrorReporting), | ||
| Err(OverflowError::Error(e)) => Err(Overflow(OverflowError::Error(e))), | ||
| } | ||
| } | ||
| _ => Ok(Some(c)), | ||
| }) | ||
| .flat_map(Result::transpose) | ||
| .collect::<Result<Vec<_>, _>>()?; |
There was a problem hiding this comment.
Hmm, I see, this is a copy of the winnowing code below. What about making if candidates.len() == 1 { below additionally check that "this candidate isn't a fully-blanket impl", and just not return early?
It could be framed as "only early-succeed when the candidate is a refinement of the obligation", where e.g. "$0: Iterator is not a refinement of $0: IntoIterator".
Then such candidates would hit the winnowing step, which already exists and wouldn't need duplication.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
estebank
reviewed
Jul 25, 2022
Comment on lines
-80
to
-84
| note: required because of the requirements on the impl of `Foo<'_, '_, u8>` for `str` | ||
| --> $DIR/substs-ppaux.rs:11:17 | ||
| | | ||
| LL | impl<'a,'b,T,S> Foo<'a, 'b, S> for T {} | ||
| | ^^^^^^^^^^^^^^ ^ |
There was a problem hiding this comment.
🤔 should we be pointing at blanket impls always, if any, on E0277s?
estebank
reviewed
Jul 25, 2022
| --> $DIR/issue-62009-1.rs:12:15 | ||
| | | ||
| LL | (|_| 2333).await; | ||
| | ^^^^^^ `[closure@$DIR/issue-62009-1.rs:12:6: 12:9]` is not a future | ||
| | ^^^^^^ `[closure@$DIR/issue-62009-1.rs:12:6: 12:9]` cannot be converted to a future |
There was a problem hiding this comment.
I really hate how we display closures 😩
Ideally we should number them and display a span label naming all the ones that are involved, but this will require handling that on every error independently :-/
There was a problem hiding this comment.
related discussion on irlo wants to display fn items more like closures, specifically so they don't look like fn pointers.
Perhaps lying and calling the closure impl FnOnce(_) -> {integer} would work? Or not abusing impl trait, {closure}: FnOnce(_) -> {integer}.
There is a potential way to number closures automatically, but it would still require touching every location that's naming types; roughly one of:
fn DiagnosticBuilder::name_type(ty: &Ty) -> impl Display; provides an indexed name for{closure#0}and registers to show a map of indexed to the normal display formatting.- Add functionality to
Print/PrettyPrinterto do this numbering, require printing types to usePrintinstead ofDisplay(e.g. here usingDiagnosticBuilderas the printing context), and do thename_typestuff through the existingPrint/PrettyPrintermachinery.
Either way it seems reasonable to add a PrettyPrinter::pretty_print_closure or similar.
estebank
reviewed
Jul 25, 2022
Comment on lines
-1
to
-20
| error[E0599]: the method `f` exists for struct `S`, but its trait bounds were not satisfied | ||
| error[E0599]: no method named `f` found for struct `S` in the current scope | ||
| --> $DIR/method-unsatified-assoc-type-predicate.rs:30:7 | ||
| | | ||
| LL | struct S; | ||
| | -------- | ||
| | | | ||
| | method `f` not found for this struct | ||
| | doesn't satisfy `<S as X>::Y<i32> = i32` | ||
| | doesn't satisfy `S: M` | ||
| | -------- method `f` not found for this struct | ||
| ... | ||
| LL | a.f(); | ||
| | ^ method cannot be called on `S` due to unsatisfied trait bounds | ||
| | ^ method not found in `S` | ||
| | | ||
| note: trait bound `<S as X>::Y<i32> = i32` was not satisfied | ||
| --> $DIR/method-unsatified-assoc-type-predicate.rs:14:11 | ||
| = help: items from traits can only be used if the trait is implemented and in scope | ||
| note: `M` defines an item `f`, perhaps you need to implement it | ||
| --> $DIR/method-unsatified-assoc-type-predicate.rs:10:1 | ||
| | | ||
| LL | impl<T: X<Y<i32> = i32>> M for T {} | ||
| | ^^^^^^^^^^^^ - - | ||
| | | | ||
| | unsatisfied trait bound introduced here |
There was a problem hiding this comment.
This would qualify as a regression, right? Maybe we need a two-step mechanism, where we create the error first and then we try to resolve without accounting for trait bounds?
There was a problem hiding this comment.
IMO this suggests that it's the right error:
note: `M` defines an item `f`, perhaps you need to implement it
At most we could show the impl as "hey btw this M impl exists but doesn't apply here" (which I think we do sometimes? maybe not for blanket impls? or is it for an error that's unambiguous about which trait is, and just not method call resolution failures?)
There was a problem hiding this comment.
But in this case it is a blanket impl, which means that the user can't impl M themselves due to orphan rules, right?
There was a problem hiding this comment.
As an analogy (to avoid the M/X names here), you can implement IntoIterator yourself on your own types, the only time it's disallowed is if you also implemented Iterator (in which case you have IntoIterator already handled by the blanket impl so the error isn't a concern).
If the type wasn't a locally-defined struct S;, maybe I could see "perhaps you need to implement it" being suboptimal, but it's fine here.
|
☔ The latest upstream changes (presumably #100151) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
the
S-waiting-on-author
|
don't have the capacity to continue working on this rn and an generally not too happy with this approach. |
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.
implements the idea behind #99531 (comment) manually for
IntoIterator. I think it's pretty much exclusively an improvement.I am not sure what's the best way to actually implement the full approach outlined by @eddyb but it definitely seems like something worth pursuing.
r? @ghost