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 option to anonymize line numbers #3

Merged
merged 2 commits into from Jun 13, 2019

Conversation

@phansch
Copy link
Member

commented Jun 9, 2019

With the current diagnostics emitter of Rust, the -Z ui-testing flag
allows to 'anonymize' line numbers in the UI test output.

This means that a line such as:

 2 |     concat!(b'f');

is turned into:

LL |     concat!(b'f');

This is done because it makes the diff of UI test output changes much
less noisy.

To support this with annotate-snippet, we add a second parameter to
DisplayListFormatter that, when true, replaces the line numbers in the
left column with the text LL. The replacement text is always LL and
it does not affect the initial location line number.

In the new annotate-snippet emitter in rustc, we can then use this
parameter depending on whether the -Z ui-testing flag is set or not.

Closes #2
cc rust-lang/rust#59346

@phansch

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

One of my small gripes with Rust is that it doesn't have keyword arguments (yet?). That would make passing these boolean options much more understandable at the call sites. I'm not sure if there's currently a clearer way?

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

keyword arguments

you can always create structs with appropriate field names ;)

src/formatter/mod.rs Outdated Show resolved Hide resolved
phansch added 2 commits Jun 9, 2019
Add option to anonymize line numbers
With the current diagnostics emitter of Rust, the `-Z ui-testing` flag
allows to 'anonymize' line numbers in the UI test output.

This means that a line such as:

     2 |     concat!(b'f');

is turned into:

    LL |     concat!(b'f');

This is done because it makes the diff of UI test output changes much
less noisy.

To support this with `annotate-snippet`, we add a second parameter to
`DisplayListFormatter` that, when true, replaces the line numbers in the
left column with the text `LL`. The replacement text is always `LL` and
it does not affect the initial location line number.

In the new `annotate-snippet` emitter in rustc, we can then use this
parameter depending on whether the `-Z ui-testing` flag is set or not.

@phansch phansch force-pushed the phansch:anonymized_line_numbers branch from 3d97fb9 to d6c36a6 Jun 13, 2019

@phansch

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

Replaced the magic constant.

@zbraniecki what are your feelings regarding the change of the api of the DisplayListFormatter constructor? I feel like it's still fine when passing two booleans. If there turns out to be a need for more configuration, it could be changed later?

@zbraniecki

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

If there turns out to be a need for more configuration, it could be changed later?

Yep, it's an internal API, I'm fine with adjusting it later.

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

I wonder...

@bors r+

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

I guess not

@oli-obk oli-obk merged commit d62b321 into rust-lang:master Jun 13, 2019

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.03%) to 91.176%
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@phansch phansch deleted the phansch:anonymized_line_numbers branch Jun 13, 2019

@phansch

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

Thanks! I think this will also need a new release before it can be used in rustc, right?

@phansch

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

@zbraniecki (or someone else?) could you push a new version to crates.io?

@zbraniecki

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2019

yeah, will do.

@zbraniecki

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2019

done. 0.6.0 released.

@zbraniecki

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

@phansch did you intend to turn also context lines (without numbers) to LL?

I see this:

error[E0003]: Expected token: "="
 --> ../fluent-bundle/examples/resources/en-US/simple.ftl:2:18
  |
2 | input-parse-error Error: Could not parse input `{ $input }`. Reason: { $reason }
  |                   ^^ Expected token: "="
  |
-----------------------------

turned into:

error[E0003]: Expected token: "="
  --> ../fluent-bundle/examples/resources/en-US/simple.ftl:2:18
LL |
LL | input-parse-error Error: Could not parse input `{ $input }`. Reason: { $reason }
LL |                   ^^ Expected token: "="
LL |
-----------------------------

I think I'd expect:

error[E0003]: Expected token: "="
  --> ../fluent-bundle/examples/resources/en-US/simple.ftl:2:18
   |
LL | input-parse-error Error: Could not parse input `{ $input }`. Reason: { $reason }
   |                   ^^ Expected token: "="
   |
-----------------------------

instead.

@phansch

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Yeah I noticed that, too. That's a bug of the current implementation. My current plan is to only show LL for DisplaySourceLine::Content and where DisplayLine::Source has a lineno set. I think that should be enough?

(I've been a bit time restricted the past month or so but I want to get back into it again this month)

phansch added a commit to phansch/rust that referenced this pull request Jul 25, 2019
librustc_errors: Support ui-testing flag in annotate-snippet emitter
This adds support for the `-Z ui-testing` flag to the new
annotate-snippet diagnostic emitter.

The support for the flag was added to `annotate-snippet-rs` in these PRs:

* rust-lang/annotate-snippets-rs#3
* rust-lang/annotate-snippets-rs#5

Closes rust-lang#61811
Centril added a commit to Centril/rust that referenced this pull request Jul 26, 2019
Rollup merge of rust-lang#62985 - phansch:support_ui_testing_flag, r=…
…estebank

librustc_errors: Support ui-testing flag in annotate-snippet emitter

This adds support for the `-Z ui-testing` flag to the new
annotate-snippet diagnostic emitter.

Support for the flag was added to `annotate-snippet-rs` in these PRs:

* rust-lang/annotate-snippets-rs#3
* rust-lang/annotate-snippets-rs#5

r? @estebank

Closes rust-lang#61811
Centril added a commit to Centril/rust that referenced this pull request Jul 26, 2019
Rollup merge of rust-lang#62985 - phansch:support_ui_testing_flag, r=…
…estebank

librustc_errors: Support ui-testing flag in annotate-snippet emitter

This adds support for the `-Z ui-testing` flag to the new
annotate-snippet diagnostic emitter.

Support for the flag was added to `annotate-snippet-rs` in these PRs:

* rust-lang/annotate-snippets-rs#3
* rust-lang/annotate-snippets-rs#5

r? @estebank

Closes rust-lang#61811
Centril added a commit to Centril/rust that referenced this pull request Jul 26, 2019
Rollup merge of rust-lang#62985 - phansch:support_ui_testing_flag, r=…
…estebank

librustc_errors: Support ui-testing flag in annotate-snippet emitter

This adds support for the `-Z ui-testing` flag to the new
annotate-snippet diagnostic emitter.

Support for the flag was added to `annotate-snippet-rs` in these PRs:

* rust-lang/annotate-snippets-rs#3
* rust-lang/annotate-snippets-rs#5

r? @estebank

Closes rust-lang#61811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.