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

Fix issue #41652 #41668

Merged
merged 1 commit into from
May 7, 2017
Merged

Fix issue #41652 #41668

merged 1 commit into from
May 7, 2017

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented May 1, 2017

Fix issue #41652. Don't print anything in render_source_line() if no source code is given.

(cc @jonathandturner #34789)

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@kennytm
Copy link
Member Author

kennytm commented May 1, 2017

The error happens because imported modules have no associated source code. As I mentioned in #41652, on stable this leads to a span pointing nowhere 👻

note: candidate #1 is defined in the trait `b::Tr`
  --> b/src/lib.rs:11:5
   |
11 | 
   |  _____^ starting here...
12 | |
   | |__________________________^ ...ending here
   = help: to disambiguate the method call, write `b::Tr::read(3)` instead

and causes ICE after #41245 because of source_string[0..ann.start_col] (where source_string == "").

In this PR, the error will be printed as

note: candidate #1 is defined in the trait `b::Tr`
 --> /full/path/to/b/src/lib.rs:11:5
  |
  = help: to disambiguate the method call, write `b::Tr::read(3)` instead

Note the line number is still printed, but without any source code following it. While this is certainly better than the current situation, I don't know if it is good enough.

Alternatively we could ensure FileMap actually contains the source code when reporting error, but I don't see an easy way to do so without heavily modifying libsyntax ☺️.

@kennytm kennytm force-pushed the fix-issue-41652 branch 2 times, most recently from 5d326ab to 1edad2d Compare May 1, 2017 09:37
@arielb1
Copy link
Contributor

arielb1 commented May 1, 2017

Could you have a UI test for that?

r? @jonathandturner

@rust-highfive rust-highfive assigned sophiajt and unassigned arielb1 May 1, 2017
@kennytm kennytm force-pushed the fix-issue-41652 branch from 1edad2d to ce92a54 Compare May 1, 2017 09:57
@kennytm
Copy link
Member Author

kennytm commented May 1, 2017

@arielb1 Added (ce92a54). In that case, should I just remove the compile-fail test?

@arielb1
Copy link
Contributor

arielb1 commented May 1, 2017

Yeah. There's no need for duplicate ui + cfail tests. Just a UI test so we can make sure the output is sane.

@kennytm kennytm force-pushed the fix-issue-41652 branch from ce92a54 to e063243 Compare May 1, 2017 10:14
@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2017
@arielb1
Copy link
Contributor

arielb1 commented May 2, 2017

@jonathandturner can you review this change?

@sophiajt
Copy link
Contributor

sophiajt commented May 3, 2017

This seems better than what we have, so we may want to approve just based on that. How difficult would it be to have this instead?

note: candidate #1 is defined in the trait `b::Tr`
help: to disambiguate the method call, write `b::Tr::read(3)` instead

Since starting to draw the span but not drawing anything doesn't seem to help the user much.

Don't print the source code in emit_message_default() and
render_source_line() if the source code is None.
@kennytm kennytm force-pushed the fix-issue-41652 branch from e063243 to 81bfdc8 Compare May 3, 2017 05:00
@kennytm
Copy link
Member Author

kennytm commented May 3, 2017

@jonathandturner Looks like skipping the whole iteration works. The output is now

error: no method named `f` found for type `{integer}` in the current scope
  --> $DIR/issue_41652.rs:19:11
   |
19 |         3.f()
   |           ^
   |
   = note: found the following associated functions; to be used as methods, functions must have a `self` parameter
note: candidate #1 is defined in the trait `issue_41652_b::Tr`
   = help: to disambiguate the method call, write `issue_41652_b::Tr::f(3)` instead

error: aborting due to previous error

@Mark-Simulacrum
Copy link
Member

@jonathandturner Just checking in here, looks like this PR is ready for another review.

@sophiajt
Copy link
Contributor

sophiajt commented May 7, 2017

Great!

@bors r+

@bors
Copy link
Contributor

bors commented May 7, 2017

📌 Commit 81bfdc8 has been approved by jonathandturner

@bors
Copy link
Contributor

bors commented May 7, 2017

⌛ Testing commit 81bfdc8 with merge 5b31bf8...

bors added a commit that referenced this pull request May 7, 2017
Fix issue #41652

Fix issue #41652. Don't print anything in `render_source_line()` if no source code is given.

(cc @jonathandturner #34789)
@bors
Copy link
Contributor

bors commented May 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: jonathandturner
Pushing 5b31bf8 to master...

@bors bors merged commit 81bfdc8 into rust-lang:master May 7, 2017
@kennytm kennytm deleted the fix-issue-41652 branch May 7, 2017 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants