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

typeck: when suggesting associated fns, do not show call site as fallback #33330

Merged
merged 1 commit into from
May 3, 2016

Conversation

birkenfeld
Copy link
Contributor

@birkenfeld birkenfeld commented May 2, 2016

In case we cannot produce a span for the location of the definition, just do not show a span at all.

cc: #29121

@rust-highfive
Copy link
Collaborator

r? @nrc

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

@Manishearth
Copy link
Member

This does not fix #29121, that bug is about improving the diagnostics to help in both situations (when an impl is wrong and when a call is wrong)

@birkenfeld
Copy link
Contributor Author

True, should only reference the issue.

idx + 1,
insertion,
impl_ty);
if item_span != span {
Copy link
Member

@Manishearth Manishearth May 2, 2016

Choose a reason for hiding this comment

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

Can we use span_if_local instead with if let? r=me with that nit fixed

@birkenfeld
Copy link
Contributor Author

@Manishearth like that?

@Manishearth
Copy link
Member

@bors r+

yep!

@bors
Copy link
Contributor

bors commented May 2, 2016

📌 Commit 832ce20 has been approved by Manishearth

let impl_span = fcx.tcx().map.def_id_span(impl_did, span);
let item_span = fcx.tcx().map.def_id_span(item.def_id(), impl_span);
let note_span = fcx.tcx().map.span_if_local(item.def_id()).or_else(|| {
fcx.tcx().map.span_if_local(impl_did) });
Copy link
Member

Choose a reason for hiding this comment

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

style nit: newline before closing brace

@birkenfeld
Copy link
Contributor Author

@nrc @Manishearth since this is already rollup-ed, what's the procedure to fix it?

@nrc
Copy link
Member

nrc commented May 2, 2016

@birkenfeld a follow-up PR is probably best, if you're quick, @Manishearth might be able to get it into the rollup - these things often take a few goes to land.

@Manishearth
Copy link
Member

Just fix up this PR and I'll re-rollup. I have scripts for efficiently dealing with rollups

@birkenfeld
Copy link
Contributor Author

@Manishearth added a followup commit here.

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 2, 2016

📌 Commit b7821f9 has been approved by Manishearth

@Manishearth
Copy link
Member

/buildslave/rust-buildbot/slave/auto-linux-64-cross-armhf/build/src/librustc_typeck/check/method/suggest.rs:267:29: 267:42 error: no method named `fileline_note` found for type `&mut syntax::errors::DiagnosticBuilder<'_>` in the current scope
/buildslave/rust-buildbot/slave/auto-linux-64-cross-armhf/build/src/librustc_typeck/check/method/suggest.rs:267                         err.fileline_note(span, &note_str);
                                                                                                                                            ^~~~~~~~~~~~~
error: aborting due to previous error
Build failed, waiting for other jobs to finish...
error: Could not compile `rustc_typeck`.

@Manishearth
Copy link
Member

@bors r- delegate+

@bors
Copy link
Contributor

bors commented May 2, 2016

✌️ @birkenfeld can now approve this pull request

bors added a commit that referenced this pull request May 2, 2016
Rollup of 14 pull requests

- Successful merges: #32756, #33129, #33225, #33260, #33309, #33320, #33323, #33324, #33325, #33330, #33332, #33334, #33335, #33346
- Failed merges:
bors added a commit that referenced this pull request May 3, 2016
Rollup of 14 pull requests

- Successful merges: #32756, #33129, #33225, #33260, #33309, #33320, #33323, #33324, #33325, #33330, #33332, #33334, #33335, #33346
- Failed merges:
@birkenfeld
Copy link
Contributor Author

@Manishearth wait, what? Why is this method missing? It is definitely not missing here (and compiles for me).

@Manishearth
Copy link
Member

Oh, it probably got changed in the rollup, Niko's error message overhaul might have tweaked things. Rebase and try building again?

…back

In case we cannot produce a span for the location of the definition,
just do not show a span at all.

cc: rust-lang#29121
@birkenfeld
Copy link
Contributor Author

@bors r=Manishearth

@bors
Copy link
Contributor

bors commented May 3, 2016

📌 Commit 780f725 has been approved by Manishearth

@bors
Copy link
Contributor

bors commented May 3, 2016

⌛ Testing commit 780f725 with merge 3157691...

bors added a commit that referenced this pull request May 3, 2016
typeck: when suggesting associated fns, do not show call site as fallback

In case we cannot produce a span for the location of the definition, just do not show a span at all.

cc: #29121
@bors bors merged commit 780f725 into rust-lang:master May 3, 2016
@birkenfeld birkenfeld deleted the issue-29121 branch May 4, 2016 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants