Skip to content

Fix issue where 2 IDE analyzers are using IDE0035#24590

Merged
chborl merged 5 commits intodotnet:masterfrom
chborl:twoIDs
Feb 5, 2018
Merged

Fix issue where 2 IDE analyzers are using IDE0035#24590
chborl merged 5 commits intodotnet:masterfrom
chborl:twoIDs

Conversation

@chborl
Copy link
Contributor

@chborl chborl commented Feb 2, 2018

Customer scenario

Both the Validate Format String and Remove Unreachable Code analyzers use the same ID, so a user would see the same code for both of these issues and if they suppress either one it would suppress both.

Bugs this fixes

#23983

Workarounds, if any

none

Risk

No code risk, but it is a breaking change. If a customer previously suppressed the Validate Format String analzyer, it would start firing again because it has a new ID number. @kuhlenh will change it in our documentation and add it to the breaking change log.

Performance impact

none

Is this a regression from a previous update?

no

Root cause analysis

How was the bug found?

Test documentation updated?

@chborl chborl requested a review from a team as a code owner February 2, 2018 00:11
@jasonmalinowski
Copy link
Member

@chborl Can we add a test that we don't have duplicates? Something that just uses reflection to grab all the const values in this type and makes sure there aren't any duplicates? It's not perfect, but would have caught this.

@jinujoseph I'm guessing we want this in 15.7?

@jinujoseph
Copy link
Contributor

yes , lets take this for 15.7

Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

👍 but file an issue to address Jason's feedback as a follow-up item.

📝 I am particularly in favor of changing the ID of the format string analyzer, as it seems more likely for people to be depending on the suppressions for unreachable code. In other words, the approach taken here is the less-breaking of the two possible changes.

@chborl
Copy link
Contributor Author

chborl commented Feb 5, 2018

@jasonmalinowski , I've added a test for duplicate IDs so hopefully this won't happen in the future.

public void UniqueIDEDiagnosticIds()
{
Type type = typeof(IDEDiagnosticIds);
List<string> ListOfIDEDiagnosticIds = type.GetFields().Select(x => x.GetValue(null).ToString()).ToList();
Copy link
Member

Choose a reason for hiding this comment

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

nit: local, so lowercase 'l'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed.

@chborl chborl merged commit aba151c into dotnet:master Feb 5, 2018
@sharwell
Copy link
Contributor

sharwell commented Feb 5, 2018

📝 Someone needs to backport this to 15.7 since it was merged before retargeting.

@chborl
Copy link
Contributor Author

chborl commented Feb 5, 2018

Thanks, @sharwell! I'll put this in 15.7 as well.

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.

4 participants