some additional need_type_info.rs cleanup
#97703
Conversation
rustbot
added
the
T-compiler
|
Some changes occured in need_type_info.rs cc @lcnr |
|
r? @wesleywiser (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| Res::Def(DefKind::TyAlias, _) => { | ||
| // FIXME: Ideally we should support this. For that | ||
| // we have to map back from the self type to the | ||
| // type alias though. That's difficult. | ||
| // | ||
| // See the `need_type_info/type-alias.rs` test for | ||
| // some examples. | ||
| } |
There was a problem hiding this comment.
@oli-obk you might want to chime in here
There was a problem hiding this comment.
@GuillaumeGomez is working on it
There was a problem hiding this comment.
Moving slowly but surely! (you can take a look at my work in progress there: https://github.com/rust-lang/rust/compare/master...GuillaumeGomez:rustc-middle-ty-alias?expand=1)
| _ => warn!( | ||
| "unexpected path: def={:?} substs={:?} path={:?}", | ||
| def, substs, path, | ||
| ), |
There was a problem hiding this comment.
Is turning this into a warn instead of bug intended?
There was a problem hiding this comment.
yeah, don't want to ICE here and getting it wrong only worsens diagnostics.
feels safer than using a bug!
There was a problem hiding this comment.
What about using delay_span_bug instead?
There was a problem hiding this comment.
delay_span_bug won't ever trigger, considering that we're always emitting an error
There was a problem hiding this comment.
It does protect against a subsequent refactor making us hit this codepath without an error being emitted.
There was a problem hiding this comment.
the FindInferSourceVisitor should not be relevant to whether we emit an error. I do not think this is an issue. Or at least I think the guard for that shouldn't be at this point. We currently guard against it by fn emit_inference_failure_err returning a DiagnosticBuilder<'tcx, ErrorGuaranteed>.
| /// Returns the substs corresponding to the generic parameters | ||
| /// of this item, excluding `Self`. | ||
| pub fn own_substs_no_defaults( |
There was a problem hiding this comment.
Let's make it clear in the naming and docs that this is only meant for use in diagnostics code.
There was a problem hiding this comment.
added a comment, don't want to make these method names even more unwieldy 😅
|
r? @estebank |
it feels arbitrary to have `Ty` and `Const` directly in that module and to not have `GenericArg` and `GenericArgKind` there. Writing `ty::GenericArg` can also feel clearer than importing it. Using `ty::subst::GenericArg` however is ugly.
| }) | ||
| .count(); | ||
| let generic_args = | ||
| &generics.own_substs(substs)[generics.own_counts().lifetimes..][..num_args]; |
There was a problem hiding this comment.
that caused the ICE in #97698 (comment). Writing big PRs sure is difficult 😅
|
@bors r+ |
|
📌 Commit d6b28f3 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 6 pull requests Successful merges: - rust-lang#96868 (Stabilize explicit_generic_args_with_impl_trait) - rust-lang#97703 (some additional `need_type_info.rs` cleanup) - rust-lang#97812 (Suggest to swap a struct and a trait in trait impls) - rust-lang#97958 (ExitStatus docs fixups) - rust-lang#97967 (Mention `infer::Trace` methods on `infer::At` methods' docs) - rust-lang#97972 (Update #[doc(html_playground_url)] documentation to mention what the request will be) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
also fixes #97698, fixes #97806
cc @estebank