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

Add line numbers and columns to error messages spanning multiple files #47780

Merged
merged 3 commits into from
Jan 30, 2018

Conversation

varkor
Copy link
Member

@varkor varkor commented Jan 26, 2018

If an error message is emitted that spans several files, only the
primary file currently has line and column data attached. This is
useful information, even in files other than the one in which the error
occurs. We can often work out which line and column the error
corresponds to in other files — in this case it is helpful to add them
(in the case of ambiguity, the first relevant line/column is picked,
which is still helpful than none).

If an error message is emitted that spans several files, only the
primary file currently has line and column data attached. This is
useful information, even in files other than the one in which the error
occurs. We can often work out which line and column the error
corresponds to in other files — in this case it is helpful to add them
(in the case of ambiguity, the first relevant line/column is picked,
which is still helpful than none).
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2018
18 | _
| ^
|
::: $DIR/main.rs:15:5
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! =)

@@ -42,31 +42,31 @@ error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found
28 | deep!();
| -------- in this macro invocation (#1)
|
::: <deep macros>
::: <deep macros>:1:1
Copy link
Contributor

@nikomatsakis nikomatsakis Jan 26, 2018

Choose a reason for hiding this comment

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

this doesn't really feel like a win here... but I guess it's ok? Is an actual file line reflected here?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could check if annotated_file.file.name is FileName::Real(path_buf) to present the line and col, but honestly in reality I don't think it is going to be that much of a problem, and actually beneficial in real, verbose, macros.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was ambivalent about this case: since it didn't seem to be decreasing readability, I decided not to special case it here. The line number is relative to the start of the macro, rather than the file in this case. It's also behind a -Z flag, so I thought it would affect people minimally regardless.

let loc = if let Some(first_line) = annotated_file.lines.first() {
let col = if let Some(first_annotation) = first_line.annotations.first() {
format!(":{}", first_annotation.start_col + 1)
} else { "".to_string() };
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave the empty string on it's own line? It is easier to skip the code if the blocks are consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, will do!

@@ -42,31 +42,31 @@ error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found
28 | deep!();
| -------- in this macro invocation (#1)
|
::: <deep macros>
::: <deep macros>:1:1
Copy link
Contributor

Choose a reason for hiding this comment

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

We could check if annotated_file.file.name is FileName::Real(path_buf) to present the line and col, but honestly in reality I don't think it is going to be that much of a problem, and actually beneficial in real, verbose, macros.

@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 27, 2018

📌 Commit a21b7b3 has been approved by estebank

@nikomatsakis nikomatsakis 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 29, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Jan 29, 2018
…r=estebank

Add line numbers and columns to error messages spanning multiple files

If an error message is emitted that spans several files, only the
primary file currently has line and column data attached. This is
useful information, even in files other than the one in which the error
occurs. We can often work out which line and column the error
corresponds to in other files — in this case it is helpful to add them
(in the case of ambiguity, the first relevant line/column is picked,
which is still helpful than none).
kennytm added a commit to kennytm/rust that referenced this pull request Jan 30, 2018
…r=estebank

Add line numbers and columns to error messages spanning multiple files

If an error message is emitted that spans several files, only the
primary file currently has line and column data attached. This is
useful information, even in files other than the one in which the error
occurs. We can often work out which line and column the error
corresponds to in other files — in this case it is helpful to add them
(in the case of ambiguity, the first relevant line/column is picked,
which is still helpful than none).
bors added a commit that referenced this pull request Jan 30, 2018
Rollup of 12 pull requests

- Successful merges: #47515, #47603, #47718, #47732, #47760, #47780, #47822, #47826, #47836, #47839, #47853, #47855
- Failed merges:
@bors bors merged commit a21b7b3 into rust-lang:master Jan 30, 2018
@varkor varkor deleted the cross-file-errors-line-col branch January 30, 2018 15:53
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants