-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Rework "long type names" printing logic #136328
Conversation
r? @spastorino rustbot has assigned @spastorino. Use |
Some changes occurred in match checking cc @Nadrieril Some changes occurred in need_type_info.rs cc @lcnr |
pub fn short_string<'a, T>(self, p: T, path: &mut Option<PathBuf>) -> String | ||
where | ||
T: Print<'tcx, FmtPrinter<'a, 'tcx>> + Lift<TyCtxt<'tcx>> + Copy + Hash, | ||
<T as Lift<TyCtxt<'tcx>>>::Lifted: Print<'tcx, FmtPrinter<'a, 'tcx>>, | ||
{ |
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.
With these changes now anything that can be printed can be passed to this, and the printing logic already handles the shortening gracefully. Look at the long E0277 error for an example of the effect.
@@ -635,19 +629,13 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { | |||
self.suggest_cloning(err, place_ty, expr, Some(use_spans)); | |||
} | |||
|
|||
let mut path = None; | |||
let ty = self.infcx.tcx.short_ty_string(place_ty, &mut path); | |||
let ty = self.infcx.tcx.short_string(place_ty, err.long_ty_path()); |
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.
This is an example of the intended way of using this. This is the safest way of creating a short string because there's no way of forgetting to print the "full name written to path
" message.
if self.should_truncate() { | ||
write!(self, "@...}}")?; | ||
return Ok(()); |
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.
This changes the closure printing so when the closure is right at the edge of being replaced with ...
, we print it as {closure@...}
.
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.
This file was in the repo but wasn't mod
ded 😵
@@ -29,7 +29,7 @@ error[E0308]: mismatched types | |||
--> $DIR/coerce-expect-unsized-ascribed.rs:14:27 | |||
| | |||
LL | let _ = type_ascribe!(Box::new( { |x| (x as u8) }), Box<dyn Fn(i32) -> _>); | |||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Box<dyn Fn(i32) -> u8>`, found `Box<{closure@coerce-expect-unsized-ascribed.rs:14:39}>` | |||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Box<dyn Fn(i32) -> u8>`, found `Box<{closure@...}>` |
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.
Example of the "short closure" logic mentioned before.
@@ -7,14 +7,16 @@ LL | | Err::<(), _>( | |||
LL | | Ok::<_, ()>( | |||
... | | |||
LL | | ) | |||
| |_____^ type mismatch resolving `<Result<Result<(), Result<Result<(), ...>, ...>>, ...> as Future>::Error == Foo` | |||
| |_____^ type mismatch resolving `<Result<..., ()> as Future>::Error == Foo` |
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.
Example of the allowlisting of "known short" types so we don't replace them with ...
unnecessarily.
error[E0277]: the trait bound `(..., ..., ..., ...): Trait` is not satisfied | ||
--> $DIR/long-e0277.rs:13:21 | ||
| | ||
LL | require_trait::<D>(); | ||
| ^ unsatisfied trait bound | ||
| | ||
= help: the trait `Trait` is not implemented for `(..., ..., ..., ...)` | ||
help: this trait has no implementations, consider adding one | ||
--> $DIR/long-e0277.rs:8:1 | ||
| | ||
LL | trait Trait {} | ||
| ^^^^^^^^^^^ | ||
note: required by a bound in `require_trait` | ||
--> $DIR/long-e0277.rs:10:21 | ||
| | ||
LL | fn require_trait<T: Trait>() {} | ||
| ^^^^^ required by this bound in `require_trait` | ||
= note: the full name for the type has been written to '$TEST_BUILD_DIR/diagnostic-width/long-e0277/long-e0277.long-type-hash.txt' | ||
= note: consider using `--verbose` to print the full type name to the console |
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.
This is the error I had set out to clean up, and ended up being a slightly larger refactor than I intended. You can see that the bound is reusing the "short type" printing logic now.
= note: the full name for the type has been written to '$TEST_BUILD_DIR/traits/on_unimplemented_long_types/on_unimplemented_long_types.long-type-hash.txt' | ||
= note: consider using `--verbose` to print the full type name to the console |
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.
Another benefit of making Diag
carry the path: it's impossible to have redundant notes now.
#136315 will conflict with this PR. |
@@ -1,6 +1,6 @@ | |||
//@ revisions: ascii unicode | |||
//@[ascii] compile-flags: --diagnostic-width=40 | |||
//@[unicode] compile-flags: -Zunstable-options --error-format=human-unicode --diagnostic-width=40 | |||
//@[ascii] compile-flags: --diagnostic-width=40 -Zwrite-long-types-to-disk=yes |
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.
Random comment while I was in the neighborhood.
When using -Zwrite-long-types-to-disk=yes
, we usually normalize away the hash in the long type file name, but that's not enough. Using a compare-mode will embed the compare mode in the long type file path as well.
The path note: the full name for the type has been written to '$TEST_BUILD_DIR/diagnostic-width/E0271.unicode/E0271.long-type-hash.txt'
will become note: the full name for the type has been written to '$TEST_BUILD_DIR/diagnostic-width.$compare-mode/E0271.unicode/E0271.long-type-hash.txt'
and the test will fail.
I encountered this while looking over the test failures under the new solver, i.e. ./x test bla --compare-mode=next-solver
. While this specific test will currently fail for different reasons, it should pass in general without any changes required to diagnostics. (you can check that this test will fail under e.g. --compare-mode polonius
)
For that, we'd need either:
- compiletest to take care of all normalizations needing to be done due to compare-modes
- expand what we manually normalize in the tests, taking care of the compare-mode suffix, as well as the possible hash
Since we already do part of the latter, I personally went with option 2 in #136310, and expanded the regex to look for the complete file path to remove the subdirectories.
For cases that have a hash suffix to normalize away, like in this E0271 test, it looks like:
// The regex below normalizes the long type file name to make it suitable for compare-modes.
//@ normalize-stderr: "'\$TEST_BUILD_DIR/.*\.long-type-\d+.txt'" -> "'$$TEST_BUILD_DIR/$$FILE.long-type-hash.txt'"
One could remove the comment to avoid reblessing the new line numbers. (Or simplify the regex, I went with making sure only this path output would be changed. The hash suffix normalization doesn't do that, nor did the .nll/ -> ""
compare-mode normalization)
I already took care of long-E0308
and so on in that other PR (so they will conflict/require reblessing), but the same issue will also appear in the new long-e0277.rs
test here.
Thanks for coming to my TED talk.
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.
Took your regex verbatim :)
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.
Thanks, this is a very nice cleanup! I left some questions/feedback.
if let Some(path) = &self.long_ty_path { | ||
self.note(format!( | ||
"the full name for the type has been written to '{}'", | ||
path.display() |
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.
Question: not necessarily for this PR, but this doesn't handle path-remapping (--remap-path-prefix
or -Zremap-path-scope=diagnostics
and such), right?
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.
Correct. We should change it to do so.
write!(self, "@...}}")?; | ||
return Ok(()); | ||
} else { | ||
// FIXME(eddyb) should use `def_span`. |
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.
Question: is that FIXME still accurate, because I see a def_span
invocation below?
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.
I noticed that too. I don't know. There's a call to def_span
, but I'm unsure if it is also meant to apply to non-local types.
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.
I didn't bother digging earlier than 55871aa, but at that moment of time, we certainly weren't using def_span
. Along the 6 years since that, this comment got shuffled around so I think it became outdated but no one noticed. We are now indeed properly printing the def path through def_span
.
(Previously, the printing code here was re-implementing path printing with ::
and <
and such.)
☔ The latest upstream changes (presumably #136332) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot author |
af50c82
to
abaf557
Compare
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.
Thanks, the changes LGTM. You can r=me and @lqd after removing the outdated FIXME comment.
write!(self, "@...}}")?; | ||
return Ok(()); | ||
} else { | ||
// FIXME(eddyb) should use `def_span`. |
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.
I didn't bother digging earlier than 55871aa, but at that moment of time, we certainly weren't using def_span
. Along the 6 years since that, this comment got shuffled around so I think it became outdated but no one noticed. We are now indeed properly printing the def path through def_span
.
(Previously, the printing code here was re-implementing path printing with ::
and <
and such.)
| -------- ^ expected `Box<dyn Future<Output = ...> + Send>`, found type parameter `F` | ||
| -------- ^ expected `Box<dyn Future<Output = i32> + Send>`, found type parameter `F` |
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.
Remark: this is a nice heuristic :)
Make it so more type-system types can be printed in a shortened version (like `Predicate`s). Centralize printing the information about the "full type name path". Make the "long type path" for the file where long types are written part of `Diag`, so that it becomes easier to keep track of it, and ensure it will always will be printed out last in the diagnostic by making its addition to the output implicit. Tweak the shortening of types in "expected/found" labels. Remove dead file `note.rs`.
abaf557
to
0751e90
Compare
@@ -2,7 +2,7 @@ error[E0308]: mismatched types | |||
--> $DIR/box-future-wrong-output.rs:20:39 | |||
| | |||
LL | let _: BoxFuture<'static, bool> = async {}.boxed(); | |||
| ------------------------ ^^^^^^^^^^^^^^^^ expected `bool`, found `()` | |||
| ------------------------ ^^^^^^^^^^^^^^^^ expected `Pin<Box<...>>`, found `Pin<Box<dyn Future<Output = ()> + Send>>` |
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.
I'll take a look at this later, I'd like us to keep the previous output for this case in particular.
@bors r=jieyouxu,lqd |
Rework "long type names" printing logic Make it so more type-system types can be printed in a shortened version (like `Predicate`s). Centralize printing the information about the "full type name path". Make the "long type path" for the file where long types are written part of `Diag`, so that it becomes easier to keep track of it, and ensure it will always will be printed out last in the diagnostic by making its addition to the output implicit. Tweak the shortening of types in "expected/found" labels. Remove dead file `note.rs`.
Rollup of 11 pull requests Successful merges: - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu) - rust-lang#134740 (Add amdgpu target) - rust-lang#135836 (bootstrap: only build `crt{begin,end}.o` when compiling to MUSL) - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe}) - rust-lang#136279 (Rename `tcx.ensure()` to `tcx.ensure_ok()`, and improve the associated docs) - rust-lang#136283 (Update encode_utf16 to mention it is native endian) - rust-lang#136309 (set rustc dylib on manually constructed rustc command) - rust-lang#136328 (Rework "long type names" printing logic) - rust-lang#136339 (CompileTest: Add Directives to Ignore `arm-unknown-*` Targets) - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`) - rust-lang#136368 (Make comma separated lists of anything easier to make for errors) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 11 pull requests Successful merges: - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu) - rust-lang#134740 (Add amdgpu target) - rust-lang#135836 (bootstrap: only build `crt{begin,end}.o` when compiling to MUSL) - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe}) - rust-lang#136279 (Rename `tcx.ensure()` to `tcx.ensure_ok()`, and improve the associated docs) - rust-lang#136283 (Update encode_utf16 to mention it is native endian) - rust-lang#136309 (set rustc dylib on manually constructed rustc command) - rust-lang#136328 (Rework "long type names" printing logic) - rust-lang#136339 (CompileTest: Add Directives to Ignore `arm-unknown-*` Targets) - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`) - rust-lang#136368 (Make comma separated lists of anything easier to make for errors) r? `@ghost` `@rustbot` modify labels: rollup
Rework "long type names" printing logic Make it so more type-system types can be printed in a shortened version (like `Predicate`s). Centralize printing the information about the "full type name path". Make the "long type path" for the file where long types are written part of `Diag`, so that it becomes easier to keep track of it, and ensure it will always will be printed out last in the diagnostic by making its addition to the output implicit. Tweak the shortening of types in "expected/found" labels. Remove dead file `note.rs`.
Rollup of 10 pull requests Successful merges: - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu) - rust-lang#134740 (Add amdgpu target) - rust-lang#135836 (bootstrap: only build `crt{begin,end}.o` when compiling to MUSL) - rust-lang#136279 (Rename `tcx.ensure()` to `tcx.ensure_ok()`, and improve the associated docs) - rust-lang#136283 (Update encode_utf16 to mention it is native endian) - rust-lang#136309 (set rustc dylib on manually constructed rustc command) - rust-lang#136328 (Rework "long type names" printing logic) - rust-lang#136339 (CompileTest: Add Directives to Ignore `arm-unknown-*` Targets) - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`) - rust-lang#136368 (Make comma separated lists of anything easier to make for errors) r? `@ghost` `@rustbot` modify labels: rollup try-job: dist-powerpc64-linux
Rework "long type names" printing logic Make it so more type-system types can be printed in a shortened version (like `Predicate`s). Centralize printing the information about the "full type name path". Make the "long type path" for the file where long types are written part of `Diag`, so that it becomes easier to keep track of it, and ensure it will always will be printed out last in the diagnostic by making its addition to the output implicit. Tweak the shortening of types in "expected/found" labels. Remove dead file `note.rs`.
Rollup of 5 pull requests Successful merges: - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu) - rust-lang#135836 (bootstrap: only build `crt{begin,end}.o` when compiling to MUSL) - rust-lang#136279 (Rename `tcx.ensure()` to `tcx.ensure_ok()`, and improve the associated docs) - rust-lang#136328 (Rework "long type names" printing logic) - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`) r? `@ghost` `@rustbot` modify labels: rollup try-job: dist-powerpc64-linux
Rollup of 11 pull requests Successful merges: - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu) - rust-lang#134740 (Add amdgpu target) - rust-lang#135836 (bootstrap: only build `crt{begin,end}.o` when compiling to MUSL) - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe}) - rust-lang#136279 (Rename `tcx.ensure()` to `tcx.ensure_ok()`, and improve the associated docs) - rust-lang#136283 (Update encode_utf16 to mention it is native endian) - rust-lang#136309 (set rustc dylib on manually constructed rustc command) - rust-lang#136328 (Rework "long type names" printing logic) - rust-lang#136339 (CompileTest: Add Directives to Ignore `arm-unknown-*` Targets) - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`) - rust-lang#136368 (Make comma separated lists of anything easier to make for errors) r? `@ghost` `@rustbot` modify labels: rollup try-job: dist-powerpc64-linux
Rollup of 11 pull requests Successful merges: - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu) - rust-lang#134740 (Add amdgpu target) - rust-lang#135836 (bootstrap: only build `crt{begin,end}.o` when compiling to MUSL) - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe}) - rust-lang#136279 (Rename `tcx.ensure()` to `tcx.ensure_ok()`, and improve the associated docs) - rust-lang#136283 (Update encode_utf16 to mention it is native endian) - rust-lang#136309 (set rustc dylib on manually constructed rustc command) - rust-lang#136328 (Rework "long type names" printing logic) - rust-lang#136339 (CompileTest: Add Directives to Ignore `arm-unknown-*` Targets) - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`) - rust-lang#136368 (Make comma separated lists of anything easier to make for errors) r? `@ghost` `@rustbot` modify labels: rollup try-job: dist-powerpc64-linux
Rework "long type names" printing logic Make it so more type-system types can be printed in a shortened version (like `Predicate`s). Centralize printing the information about the "full type name path". Make the "long type path" for the file where long types are written part of `Diag`, so that it becomes easier to keep track of it, and ensure it will always will be printed out last in the diagnostic by making its addition to the output implicit. Tweak the shortening of types in "expected/found" labels. Remove dead file `note.rs`.
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu) - rust-lang#134740 (Add amdgpu target) - rust-lang#136133 (Fix sentence in process::abort) - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe}) - rust-lang#136279 (Rename `tcx.ensure()` to `tcx.ensure_ok()`, and improve the associated docs) - rust-lang#136328 (Rework "long type names" printing logic) - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`) - rust-lang#136368 (Make comma separated lists of anything easier to make for errors) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu) - rust-lang#134740 (Add amdgpu target) - rust-lang#136133 (Fix sentence in process::abort) - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe}) - rust-lang#136279 (Rename `tcx.ensure()` to `tcx.ensure_ok()`, and improve the associated docs) - rust-lang#136328 (Rework "long type names" printing logic) - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`) - rust-lang#136368 (Make comma separated lists of anything easier to make for errors) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu) - rust-lang#134740 (Add amdgpu target) - rust-lang#136133 (Fix sentence in process::abort) - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe}) - rust-lang#136279 (Rename `tcx.ensure()` to `tcx.ensure_ok()`, and improve the associated docs) - rust-lang#136328 (Rework "long type names" printing logic) - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`) - rust-lang#136368 (Make comma separated lists of anything easier to make for errors) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu) - rust-lang#136133 (Fix sentence in process::abort) - rust-lang#136279 (Rename `tcx.ensure()` to `tcx.ensure_ok()`, and improve the associated docs) - rust-lang#136328 (Rework "long type names" printing logic) - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`) - rust-lang#136368 (Make comma separated lists of anything easier to make for errors) - rust-lang#136412 (Tweak fn pointer suggestion span) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#136328 - estebank:long-ty-path, r=jieyouxu,lqd Rework "long type names" printing logic Make it so more type-system types can be printed in a shortened version (like `Predicate`s). Centralize printing the information about the "full type name path". Make the "long type path" for the file where long types are written part of `Diag`, so that it becomes easier to keep track of it, and ensure it will always will be printed out last in the diagnostic by making its addition to the output implicit. Tweak the shortening of types in "expected/found" labels. Remove dead file `note.rs`.
Make it so more type-system types can be printed in a shortened version (like
Predicate
s).Centralize printing the information about the "full type name path".
Make the "long type path" for the file where long types are written part of
Diag
, so that it becomes easier to keep track of it, and ensure it will always will be printed out last in the diagnostic by making its addition to the output implicit.Tweak the shortening of types in "expected/found" labels.
Remove dead file
note.rs
.