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.

Sign up for GitHub

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

Jump to bottom

typeck: limit number of candidates shown for a single error #33338

Merged
merged 1 commit into from
May 12, 2016
Merged

typeck: limit number of candidates shown for a single error #33338

merged 1 commit into from
May 12, 2016

Conversation

birkenfeld
Copy link
Contributor

No idea if 10/11 is a good limit. Are there any other such limits in rustc currently?

Fixes: #25356

@rust-highfive
Copy link
Collaborator

r? @Aatch

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

Manishearth
Manishearth reviewed May 2, 2016
View reviewed changes
src/librustc_typeck/check/method/suggest.rs
@@ -236,8 +236,10 @@ pub fn report_error<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
mut sources: Vec<CandidateSource>) {
sources.sort();
sources.dedup();
// Dynamic limit to avoid hiding just one candidate, which is silly.
let limit = if sources.len() == 11 { 11 } else { 10 };
Copy link
Member

Choose a reason for hiding this comment

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

good idea

Copy link
Member

Choose a reason for hiding this comment

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

good idea

@Manishearth
Copy link
Member

LGTM. I'd like someone else (@jonathandturner?) to rubber-stamp the number 10 as well, though 😄

@sophiajt
Copy link
Contributor

sophiajt commented May 2, 2016

How difficult would it be to show an elision?

Something like:

src\main.rs:206:9: 206:35 error: multiple applicable methods in scope [E0034]
src\main.rs:206     0.0.max(normal.dot(light_dir)) * res
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~
src\main.rs:206:9: 206:35 note: candidate #1 is defined in an impl of the trait `core::iter::Iterator` for the type `std::ascii::EscapeDefault`
src\main.rs:206     0.0.max(normal.dot(light_dir)) * res
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~
src\main.rs:206:9: 206:35 note: candidate #2 is defined in an impl of the trait `core::iter::Iterator` for the type `std::collections::hash::table::RawBuckets<'_, _, _>`
src\main.rs:206     0.0.max(normal.dot(light_dir)) * res
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~
src\main.rs:206:9: 206:35 note: additional candidates elided.

FWIW, I'm fine stopping at 10, but I think it'd be good to let the user know there were others, they just aren't shown.

@birkenfeld
Copy link
Contributor Author

@jonathandturner it should already say "N more candidates not shown" at the end.

I just found another example in

rust/src/librustc/traits/error_reporting.rs

Line 209 in 90318b8

fn report_similar_impl_candidates(span: Span,
- it stops at 4 and says "and N others" at the end. I could adapt to this.

@sophiajt
Copy link
Contributor

sophiajt commented May 2, 2016

Great, I missed that. If it has the message, I'm fine stopping even sooner than 10. 4 sounds good.

@Manishearth
Copy link
Member

sgtm either way. I'd like to switch to "and N others" though, let's keep things uniform.

@bors delegate+

@bors
Copy link
Contributor

bors commented May 2, 2016

✌️ @birkenfeld can now approve this pull request

@birkenfeld
Copy link
Contributor Author

@bors r=Manishearth

@bors
Copy link
Contributor

bors commented May 3, 2016

📌 Commit 9508180 has been approved by Manishearth

@Manishearth Manishearth mentioned this pull request May 3, 2016
Merged
@bors
Copy link
Contributor

bors commented May 3, 2016

⌛ Testing commit 9508180 with merge db2531f...

@Manishearth Manishearth closed this May 3, 2016
@Manishearth Manishearth reopened this May 3, 2016
@Manishearth Manishearth closed this May 3, 2016
@Manishearth Manishearth reopened this May 3, 2016
@Manishearth
Copy link
Member

@bors clean force

@Manishearth
Copy link
Member

@bors r- clean force

(something's wrong with the queue)

@Manishearth
Copy link
Member

@bors r+ clean

@bors
Copy link
Contributor

bors commented May 3, 2016

📌 Commit 9508180 has been approved by Manishearth

@Manishearth Manishearth closed this May 3, 2016
@Manishearth Manishearth reopened this May 3, 2016
@Manishearth Manishearth closed this May 3, 2016
@Manishearth Manishearth reopened this May 3, 2016
@alexcrichton
Copy link
Member

@bors: retry force clean

bors added a commit that referenced this pull request May 3, 2016
@bors
Auto merge of #33376 - Manishearth:rollup, r=Manishearth
1fb4b5b 
Rollup of 14 pull requests

- Successful merges: #33277, #33294, #33314, #33322, #33333, #33338, #33339, #33340, #33343, #33357, #33363, #33365, #33371, #33372
- Failed merges:
@steveklabnik
Copy link
Member

Is the travis failure here legitimate?

@birkenfeld
Copy link
Contributor Author

Yes; the diagnostics API changed on master. I'll fix it soon.

bors added a commit that referenced this pull request May 3, 2016
@bors
Auto merge of #33376 - Manishearth:rollup, r=Manishearth
f7125fc 
Rollup of 14 pull requests

- Successful merges: #33277, #33294, #33314, #33322, #33333, #33338, #33339, #33340, #33343, #33357, #33363, #33365, #33371, #33372
- Failed merges:
bors added a commit that referenced this pull request May 4, 2016
@bors
Auto merge of #33376 - Manishearth:rollup, r=Manishearth
1895d08 
Rollup of 14 pull requests

- Successful merges: #33277, #33294, #33314, #33322, #33333, #33338, #33339, #33340, #33343, #33357, #33363, #33365, #33371, #33372
- Failed merges:
bors added a commit that referenced this pull request May 4, 2016
@bors
Auto merge of #33376 - Manishearth:rollup, r=Manishearth
7a0ccc4 
Rollup of 14 pull requests

- Successful merges: #33277, #33294, #33314, #33322, #33333, #33338, #33339, #33340, #33343, #33357, #33363, #33365, #33371, #33372
- Failed merges:
@bors
Copy link
Contributor

bors commented May 11, 2016

☔ The latest upstream changes (presumably #33425) made this pull request unmergeable. Please resolve the merge conflicts.

@birkenfeld
typeck: limit number of candidates shown for a single error
6ab93d7 
Limit of 4 taken consistent with limit for "similar impl candidates"
in rustc::traits::error_reporting.

Fixes: rust-lang#25356
@birkenfeld
Copy link
Contributor Author

Rebased.

@jseyfried
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 12, 2016

📌 Commit 6ab93d7 has been approved by jseyfried

@bors
Copy link
Contributor

bors commented May 12, 2016

⌛ Testing commit 6ab93d7 with merge e88defe...

bors added a commit that referenced this pull request May 12, 2016
@bors
Auto merge of #33338 - birkenfeld:issue-25356, r=jseyfried
e88defe 
typeck: limit number of candidates shown for a single error

No idea if 10/11 is a good limit. Are there any other such limits in rustc currently?

Fixes: #25356
@bors
Copy link
Contributor

bors commented May 12, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors bors merged commit 6ab93d7 into rust-lang:master May 12, 2016
@birkenfeld birkenfeld deleted the issue-25356 branch May 12, 2016 17:36
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label May 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Aatch Aatch

Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@birkenfeld @rust-highfive @Manishearth @sophiajt @bors @alexcrichton @steveklabnik @jseyfried @Aatch @bluss