Remove DebugWithInfcx machinery
#126075
Merged
Remove DebugWithInfcx machinery
#126075
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
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #126108) made this pull request unmergeable. Please resolve the merge conflicts. |
|
r=me after rebase |
compiler-errors
force-pushed
the
remove-debugwithinfcx
branch
from
01082f6 to
bcf672a
Compare
|
@bors r=lcnr |
bors
added
S-waiting-on-bors
This comment has been minimized.
This comment has been minimized.
|
whoops @bors r- |
bors
added
S-waiting-on-author
compiler-errors
force-pushed
the
remove-debugwithinfcx
branch
from
bcf672a to
ce09f5d
Compare
rustbot
added
the
WG-trait-system-refactor
|
@bors r=lcnr |
bors
added
S-waiting-on-bors
This comment has been minimized.
This comment has been minimized.
compiler-errors
force-pushed
the
remove-debugwithinfcx
branch
from
ce09f5d to
0fc18e3
Compare
|
@bors r=lcnr |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 12, 2024
…llaumeGomez Rollup of 9 pull requests Successful merges: - rust-lang#126039 (Promote `arm64ec-pc-windows-msvc` to tier 2) - rust-lang#126075 (Remove `DebugWithInfcx` machinery) - rust-lang#126228 (Provide correct parent for nested anon const) - rust-lang#126232 (interpret: dyn trait metadata check: equate traits in a proper way) - rust-lang#126242 (Simplify provider api to improve llvm ir) - rust-lang#126294 (coverage: Replace the old span refiner with a single function) - rust-lang#126295 (No uninitalized report in a pre-returned match arm) - rust-lang#126312 (Update `rustc-perf` submodule) - rust-lang#126322 (Follow up to splitting core's PanicInfo and std's PanicInfo) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 12, 2024
Rollup merge of rust-lang#126075 - compiler-errors:remove-debugwithinfcx, r=lcnr Remove `DebugWithInfcx` machinery This PR removes `DebugWithInfcx` after having a lot of second thoughts about it due to recent type system uplifting work. We could add it back later if we want, but I don't think the amount of boilerplate in the complier and the existence of (kindof) hacks like `NoInfcx` currently justify the existence of `DebugWithInfcx`, especially since it's not even being used anywhere in the compiler currently. The motivation for `DebugWithInfcx` is that we want to be able to print infcx-aware information, such as universe information[^1] (though if there are other usages that I'm overlooking, please let me know). I think there are probably more tailored solutions that can specifically be employed in places where this infcx-aware printing is necessary. For example, one way of achieving this is by implementing a custom `FmtPrinter` which overloads `ty_infer_name` (perhaps also extending it to have overrideable stubs for printing placeholders too) to print the `?u.i` name for an infer var. This will necessitate uplifting `Print` from `rustc_middle::ty::print`, but this seems a bit more extensible and reusable than `DebugWithInfcx`. One of the problems w/ `DebugWithInfcx` is its opt-in-ness. Even if a compiler dev adds a new `debug!(ty)` in a context where there is an `infcx` we can access, they have to *opt-in* to using `DebugWithInfcx` with something like `debug!(infcx.with(ty))`. This feels to me like it risks a lot of boilerplate, and very easy to just forget adding it at all, especially in cases like `#[instrument]`. A second problem is the `NoInfcx` type itself. It's necessary to have this dummy infcx implementation since we often want to print types outside of the scope of a valid `Infcx`. Right now, `NoInfcx` is only *partially* a valid implementation of `InferCtxtLike`, except for the methods that we specifically need for `DebugWithInfcx`. As I work on uplifting the trait solver, I actually want to add a lot more methods to `InferCtxtLike` and having to add `unreachable!("this should never be called")` stubs for uplifted methods like `next_ty_var` is quite annoying. In reality, I actually only *really* care about the second problem -- we could, perhaps, instead just try to get rid of `NoInfcx` and just just duplicate `Debug` and `DebugWithInfcx` for most types. If we're okay with duplicating all these implementations (though most of them would just be trivial `#[derive(Debug, DebugWithInfcx)]`), I'd be okay with that too 🤔 r? `@BoxyUwU` `@lcnr` would like to know your thoughts -- happy to discuss this further, mainly trying to bring this problem up [^1]: Which in my experience is only really necessary when we're debugging things like generalizer bugs.
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.
This PR removes
DebugWithInfcxafter having a lot of second thoughts about it due to recent type system uplifting work. We could add it back later if we want, but I don't think the amount of boilerplate in the complier and the existence of (kindof) hacks likeNoInfcxcurrently justify the existence ofDebugWithInfcx, especially since it's not even being used anywhere in the compiler currently.The motivation for
DebugWithInfcxis that we want to be able to print infcx-aware information, such as universe information1 (though if there are other usages that I'm overlooking, please let me know). I think there are probably more tailored solutions that can specifically be employed in places where this infcx-aware printing is necessary. For example, one way of achieving this is by implementing a customFmtPrinterwhich overloadsty_infer_name(perhaps also extending it to have overrideable stubs for printing placeholders too) to print the?u.iname for an infer var. This will necessitate upliftingPrintfromrustc_middle::ty::print, but this seems a bit more extensible and reusable thanDebugWithInfcx.One of the problems w/
DebugWithInfcxis its opt-in-ness. Even if a compiler dev adds a newdebug!(ty)in a context where there is aninfcxwe can access, they have to opt-in to usingDebugWithInfcxwith something likedebug!(infcx.with(ty)). This feels to me like it risks a lot of boilerplate, and very easy to just forget adding it at all, especially in cases like#[instrument].A second problem is the
NoInfcxtype itself. It's necessary to have this dummy infcx implementation since we often want to print types outside of the scope of a validInfcx. Right now,NoInfcxis only partially a valid implementation ofInferCtxtLike, except for the methods that we specifically need forDebugWithInfcx. As I work on uplifting the trait solver, I actually want to add a lot more methods toInferCtxtLikeand having to addunreachable!("this should never be called")stubs for uplifted methods likenext_ty_varis quite annoying.In reality, I actually only really care about the second problem -- we could, perhaps, instead just try to get rid of
NoInfcxand just just duplicateDebugandDebugWithInfcxfor most types. If we're okay with duplicating all these implementations (though most of them would just be trivial#[derive(Debug, DebugWithInfcx)]), I'd be okay with that too 🤔r? @BoxyUwU @lcnr would like to know your thoughts -- happy to discuss this further, mainly trying to bring this problem up
Footnotes
Which in my experience is only really necessary when we're debugging things like generalizer bugs. ↩