Skip to content
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

Deduplicate more sized errors on call exprs #120293

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

estebank
Copy link
Contributor

Change the implicit Sized Obligation Span for call expressions to include the whole expression. This aids the existing deduplication machinery to reduce the number of errors caused by a single unsized expression.

Change the implicit `Sized` `Obligation` `Span` for call expressions to
include the whole expression. This aids the existing deduplication
machinery to reduce the number of errors caused by a single unsized
expression.
@rustbot
Copy link
Collaborator

rustbot commented Jan 24, 2024

r? @nnethercote

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 24, 2024
Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of diagnostic tweaking isn't my forte. But the deduplication is definitely nice. I find some of the ^^^---^^^ markers very hard to read, as mentioned below.

r=me assuming reasonable responses to the concerns below, and if you don't want a second opinion.

@@ -2,7 +2,7 @@ error[E0283]: type annotations needed for `Foo<i32, &str, W, Z>`
--> $DIR/erase-type-params-in-label.rs:2:9
|
LL | let foo = foo(1, "");
| ^^^ --- type must be known at this point
| ^^^ ---------- type must be known at this point
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an orthogonal issue but seems worth mentioning: it took me a while to realize that these --- sequences are pointing to the code above them. It's much less obvious than the ^^^ sequences. I just interpreted it as text to lead the reader's eye towards the ^^^.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's likely I've been overlooking the spans demarcated by these --- markers for years.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮

I'm really surprised to hear! They mark the secondary spans, usually giving you extra context. At least, even if you didn't realize that they have the benefit of bringing interesting code into view (if it didn't happen to be in the lines of the primary span).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure people have suggested using non-ASCII chars for this sort of thing, and presumably there's a reason why it's still all ASCII, e.g. lack of reliable support in all terminals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kind of blocked on having a rustc.toml file where we could customize output, so that people can change their configuration without much hassle when in less supported platforms. We should do this sooner rather than later. :-/

LL | || drop(cx_ref);
LL | || });
| ||_____-^ `&mut Context<'_>` may not be safely transferred across an unwind boundary
| |_____|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orthogonal to this PR, but these arrows are really hard to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to clean up the rendering of overlapping multiline spans, but span labels make them even harder to deal with than usual.

@@ -2,8 +2,9 @@ error[E0631]: type mismatch in closure arguments
--> $DIR/anonymous-higher-ranked-lifetime.rs:2:5
|
LL | f1(|_: (), _: ()| {});
| ^^ -------------- found signature defined here
| |
| ^^^--------------^^^^
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are lots of new occurrences of ^^^---^^^ now. I guess the ^^^ highlighting and the --- highlighting overlap, and the --- wins?

Also, it's weird that the f1 and the {} are both pointed to by the ^ markers, they don't really seem connected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some logic so when primary and secondary spans overlap some bit of information is shown: the largest one is printed "first", and the then each shorter one is printed in order of length, so that at least a bit of each is shown. This is because if, for example in this case, there is a span with a label pointing within a larger one and we weren't doing that, we'd have no way of knowing where the label span ended within the larger span. That's how you end up with this kind of rendering. IIRC, we also tried some tricks with colors to aid here, but don't think they panned out so we didn't land them.

In this case the primary span marks from the fq all the way to the closing ) (including the {}), it's just "visually interrupted" by the span pointing at the closure header.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Not for this PR, but I wonder if changing this:

LL |     f1(|_: (), _: ()| {});
   |     ^^^--------------^^^^
   |     |  |
   |     |  found signature defined here
   |     expected due to this

to this:

LL |     f1(|_: (), _: ()| {});
   |     ^^^^^^^^^^^^^^^^^^^^^
   |     |  --------------
   |     |  |
   |     |  found signature defined here
   |     expected due to this

would help.

W.r.t. my lack of understanding of --- marking the secondary span, I think it's a lot clearer that --- is marking code when the secondary span label is underneath the --- and there's a | leading to it, instead of the label being to the right on the same line.

@estebank
Copy link
Contributor Author

FWIW, deduplication could also have been accomplished by going the other direction, using the path span for the obligation cause of the other cases, either way they get picked up by the "don't show the same trait bound failure twice" logic. That might have been nice, looking at it with fresh eyes, just don't know how much extra effort that would be.

@nnethercote
Copy link
Contributor

FWIW, deduplication could also have been accomplished by going the other direction

I have no opinions about that, so I'll let you decide whether to take my r=me, or ask someone with more diagnostic/span expertise for a second opinion.

@estebank
Copy link
Contributor Author

@bors r=nnethercote

@bors
Copy link
Contributor

bors commented Jan 26, 2024

📌 Commit a984193 has been approved by nnethercote

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 26, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2024
…llaumeGomez

Rollup of 11 pull requests

Successful merges:

 - rust-lang#117906 (Improve display of crate name when hovered)
 - rust-lang#118533 (Suppress unhelpful diagnostics for unresolved top level attributes)
 - rust-lang#120293 (Deduplicate more sized errors on call exprs)
 - rust-lang#120295 (Remove `raw_os_nonzero` feature.)
 - rust-lang#120310 (adapt test for v0 symbol mangling)
 - rust-lang#120342 (Remove various `has_errors` or `err_count` uses)
 - rust-lang#120434 (Revert outdated version of "Add the wasm32-wasi-preview2 target")
 - rust-lang#120445 (Fix some `Arc` allocator leaks)
 - rust-lang#120475 (Improve error message when `cargo build` is used to build the compiler)
 - rust-lang#120476 (Remove some unnecessary check logic for lang items in HIR typeck)
 - rust-lang#120485 (add missing potential_query_instability for keys and values in hashmap)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0a4fd52 into rust-lang:master Jan 30, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 30, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2024
Rollup merge of rust-lang#120293 - estebank:issue-102629, r=nnethercote

Deduplicate more sized errors on call exprs

Change the implicit `Sized` `Obligation` `Span` for call expressions to include the whole expression. This aids the existing deduplication machinery to reduce the number of errors caused by a single unsized expression.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants