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

Adds related spans and error grouping for duplicate identifier errors #25328

Merged

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jun 30, 2018

Fixes #25324

This adds related spans for cross-file "duplicate identifier" type errors (for up to the first 5 related spans, chosen by fair dice roll, as to avoid cluttering the output too much), and also compacts instances where many of such errors appear across a pair of files down to a single error message (the limit having been arbitrarily chosen as 8 instances).

Examples:
image
image

image
image

@mhegazy
Copy link
Contributor

mhegazy commented Jun 30, 2018

What about the ones in the same file, are you going to add the related info to the binder duplicate declaration diagnostics?

@@ -0,0 +1,12 @@
// @pretty: true
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just log the additional info to the .errors baseline. it is nice that we have --pretty tests, but do not think all of the tests should be --pretty tests. they are hard to read really in baseline format.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is also good to see how this change affects our existing tests too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I should now be recording related info in the error baselines.

"category": "Message",
"code": 6201
},
"Conflicts here.": {
Copy link
Contributor

Choose a reason for hiding this comment

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

@DanielRosenwasser comments on the messages?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit whimsical, but would be nice if it looked like:

file1.ts (1, 2): Error: Duplicate identifier 'foo'.
  + file2.ts (2, 3): "foo" was also declared here.
  + file3.ts (2, 3): and here.

Copy link
Member Author

Choose a reason for hiding this comment

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

In what output? The error baselines?

Copy link
Member Author

@weswigham weswigham Jun 30, 2018

Choose a reason for hiding this comment

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

Ohh, i get it, the different messages based on count. Certainly doable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done/10 with 🍚

@weswigham weswigham merged commit 7084e6c into microsoft:master Jul 2, 2018
@weswigham weswigham deleted the related-spans-for-duplicate-identifier branch July 2, 2018 17:47
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.

2 participants