-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move autoderef to rustc_hir_analysis
#106170
Conversation
r? @davidtwco (rustbot has picked a reviewer for you, use r? to override) |
cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki |
r? @lcnr might have some thoughts about this -- leaving this PR for them when they return from holiday, though this is a very low-pri pr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with one nit, does this conflict with #106171 (comment)?
@@ -690,6 +690,7 @@ impl<'tcx> InferCtxt<'tcx> { | |||
typeck_results: None, | |||
fallback_has_occurred: false, | |||
normalize_fn_sig: Box::new(|fn_sig| fn_sig), | |||
autoderef_steps: Box::new(|ty| vec![(ty, vec![])]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this ICE instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my hope was that we only rely on autoderef_steps
in errors during typeck. I guess we also autoderef when checking method receivers?
If so, don't mind this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So right now the current users of autoderef_steps
are obligation cause codes that only come from typeck, but there's nothing stopping someone adding a check that uses this during other trait error reporting code, for example, so I'd rather not ICE here.
I'll rename it to autoderef_steps_for_diagnostic
so people know it's best effort...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me after adding debug_assert
daadade
to
c8334ce
Compare
Addressed nit @bors r=lcnr |
…sis, r=lcnr Move autoderef to `rustc_hir_analysis` Not sure if this is a change we actually want, but autoderef really is only (functionally) used by `rustc_hir_analysis` and `rustc_hir_typeck`, so it probably should live there. Instead, implement a separate autoderef helper in `TypeErrCtxt` for the one use-case that goes against the ordering of the crate graph..
…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#103236 (doc: rewrite doc for signed int::{carrying_add,borrowing_sub}) - rust-lang#103800 (Stabilize `::{core,std}::pin::pin!`) - rust-lang#106097 (Migrate mir_build diagnostics 2 of 3) - rust-lang#106170 (Move autoderef to `rustc_hir_analysis`) - rust-lang#106323 (Stabilize f16c_target_feature) - rust-lang#106360 (Tweak E0277 `&`-removal suggestions) - rust-lang#106524 (Label `struct/enum constructor` instead of `fn item`, mention that it should be called on type mismatch) - rust-lang#106739 (Remove `<dyn AstConv<'tcx>>::fun(c, ...)` calls in favour of `c.astconv().fun(...)`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Not sure if this is a change we actually want, but autoderef really is only (functionally) used by
rustc_hir_analysis
andrustc_hir_typeck
, so it probably should live there.Instead, implement a separate autoderef helper in
TypeErrCtxt
for the one use-case that goes against the ordering of the crate graph..