Skip to content

Stop showing smart tags for subsequent 'Unnecessary' diagnostics #41854

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.

Sign up for GitHub

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

Jump to bottom
Merged
6 commits merged into from
Feb 26, 2020
Merged

Stop showing smart tags for subsequent 'Unnecessary' diagnostics #41854

6 commits merged into from
Feb 26, 2020

Conversation

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 21, 2020 •
edited
Loading

Fixes #41849

Before (my 16.5-p3 installation):

image

After (debugging Roslyn):

image

The third commit attempt to fix the pit of failure I fell into originally. It removes access to UnnecessaryWithoutSuggestionDescriptor and replaces it with CreateUnnecessaryWithoutSuggestionDiagnostic.

(There could honestly be a helper that feeds IEnumerable<Location>/params Location[] to context.ReportDiagnostic for you, probably.)

@jnm2 jnm2 requested a review from a team as a code owner February 21, 2020 03:27
@jnm2 jnm2 mentioned this pull request Feb 21, 2020
Closed
sharwell
sharwell reviewed Feb 22, 2020
View reviewed changes
src/CodeStyle/Core/Analyzers/AbstractCodeStyleDiagnosticAnalyzer.cs Outdated
Copy link
Contributor

@sharwell sharwell Feb 22, 2020

Choose a reason for hiding this comment

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

@mavasani did we ever add the ability to specify faded regions without additional diagnostics?

Copy link
Contributor

@mavasani mavasani Feb 22, 2020

Choose a reason for hiding this comment

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

Yes, @dibarbet added the helper API to create a diagnostic with additional locations for fading also fixed up one of the analyzers. We should clean up all our IDE analyzers to use the same approach instead of using separate fading diagnostics.

Copy link
Contributor Author

@jnm2 jnm2 Feb 22, 2020

Choose a reason for hiding this comment

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

Would it make sense for me to add that cleanup as a commit to this PR?

Copy link
Contributor Author

@jnm2 jnm2 Feb 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Would that fix mean removing just UnnecessaryWithoutSuggestion or both that and Unnecessary? (Guessing the latter)

Copy link
Contributor Author

@jnm2 jnm2 Feb 22, 2020

Choose a reason for hiding this comment

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

DiagnosticHelper.CreateWithLocationTags itself needs a helper

Copy link
Contributor Author

@jnm2 jnm2 Feb 22, 2020

Choose a reason for hiding this comment

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

Does 1812d31 seem like a good direction to go in? If not, let me know what you're thinking or just take over and I'll close the PR.

Copy link
Contributor

@sharwell sharwell Feb 22, 2020

Choose a reason for hiding this comment

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

I think UnnecessaryWithSuggestion is still needed, because it has a diagnostic ID that gets picked up by a code fix. UnnecessaryWithoutSuggestion exists solely for the fading.

Copy link
Contributor Author

@jnm2 jnm2 Feb 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@sharwell @mavasani The helper API created by @dibarbet does not fade the additional locations unless the diagnostic descriptor used also has the 'Unnecessary' tag. The need to have the 'Unnecessary' tag both in the descriptor and in the additional locations seems redundant and possibly an oversight. This means that the new API can't be used to report a diagnostic on a non-faded span and additional faded spans at the same time: the first requires the descriptor to not have the 'Unnecessary' tag, and the second requires the opposite.

Plenty of analyzers using UnnecessaryWithoutSuggestion are just this kind: they are fading spans other than the main reported span which itself should not be faded, and one analyzer even uses UnnecessaryWithoutSuggestion to fade a span specifically without allowing Ctrl+. to register an action for that span.

I'm not sure what the best course of action is, but my first thought is that this new API is not ready to be used in the timeframe for this PR to be merged. Does this sound right to you?

Copy link
Contributor Author

@jnm2 jnm2 Feb 25, 2020

Choose a reason for hiding this comment

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

@CyrusNajmabadi Before you keep reviewing, I'm going to drop all but the first two commits (the ones that actually fix the titular bug).

@sharwell @mavasani @CyrusNajmabadi I can't do the cleanup mentioned above without direction. See the explanation of the situations I ran into that the new CreateWithLocationTags can't handle.

@jnm2 jnm2 force-pushed the unnecessary_without_repeated_smart_tag branch from 1812d31 to d7e923f Compare February 22, 2020 15:45
CyrusNajmabadi
CyrusNajmabadi reviewed Feb 25, 2020
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Feb 25, 2020
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Feb 25, 2020
View reviewed changes
@jnm2 jnm2 force-pushed the unnecessary_without_repeated_smart_tag branch from d7e923f to 6ce79b9 Compare February 25, 2020 21:55
CyrusNajmabadi
CyrusNajmabadi reviewed Feb 25, 2020
View reviewed changes
...tures/Core/Portable/SimplifyInterpolation/AbstractSimplifyInterpolationDiagnosticAnalyzer.cs Outdated
var severity = option.Notification.Severity;

for (var i = 0; i < unnecessaryLocations.Length; i++)
foreach (var subsequentLocation in unnecessaryLocations.Skip(1))
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 25, 2020

Choose a reason for hiding this comment

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

for (var i = 1; i < unnecessaryLocations.Length; i++)

Copy link
Contributor Author

@jnm2 jnm2 Feb 26, 2020

Choose a reason for hiding this comment

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

I can do this but I'm reluctant without knowing why this would be preferable.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 26, 2020

Choose a reason for hiding this comment

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

One reason is to just avoid the linq allocation. Second is just that it feels clearer to me. :)

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Feb 26, 2020

thanks!

ghost
ghost approved these changes Feb 26, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost merged commit fb7580c into dotnet:master Feb 26, 2020
@jnm2 jnm2 deleted the unnecessary_without_repeated_smart_tag branch February 26, 2020 20:09
@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Feb 26, 2020

Thanks @jnm2 !

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

+3 more reviewers

@sharwell sharwell sharwell left review comments

@mavasani mavasani mavasani left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

auto-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Why does UnnecessaryWithoutSuggestionDescriptor still show the smart tag, contrary to docs?

4 participants

@jnm2 @CyrusNajmabadi @sharwell @mavasani