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

Highlight parts of fn in type errors #66682

Merged
merged 2 commits into from
Nov 25, 2019
Merged

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Nov 24, 2019

When a type error arises between two fn items, fn pointers or tuples,
highlight only the differing parts of each.

Examples:

@rust-highfive
Copy link
Collaborator

r? @davidtwco

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 24, 2019
@rust-highfive

This comment has been minimized.

@jonas-schievink
Copy link
Contributor

Nice!

When a type error arises between two fn items, fn pointers or tuples,
highlight only the differing parts of each.
values.0.push_normal(", ".to_string());
}
values.0.push("...".to_string(), !sig2.c_variadic);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a lot of things are repeated twice in this method; could we try to rewrite things so that it isn't repeated by computing the diffs first and then doing all the logic for a single case?

values
}

(ty::FnPtr(sig1), ty::FnDef(did2, substs2)) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

btw, this method is some 325+ LOC... would be good to split it up

src/librustc_errors/diagnostic.rs Outdated Show resolved Hide resolved
src/librustc_errors/diagnostic.rs Outdated Show resolved Hide resolved
src/librustc/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

LGTM, this is a really great improvement! Like @Centril has mentioned, cmp_fn_sig feels quite repetitive, but I can't think of a nice way to improve it. Feel free to r=me if you're happy with this.

@estebank
Copy link
Contributor Author

@bors r=davidtwco

@davidtwco @Centril, I will make a follow up PR to split out fn cmp and will try to look for ways to cleanly deduplicate the code.

@bors
Copy link
Contributor

bors commented Nov 25, 2019

📌 Commit 9d7774c has been approved by davidtwco

@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 Nov 25, 2019
@bors
Copy link
Contributor

bors commented Nov 25, 2019

⌛ Testing commit 9d7774c with merge ab21557...

bors added a commit that referenced this pull request Nov 25, 2019
@bors
Copy link
Contributor

bors commented Nov 25, 2019

☀️ Test successful - checks-azure
Approved by: davidtwco
Pushing ab21557 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 25, 2019
@bors bors merged commit 9d7774c into rust-lang:master Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants