-
Notifications
You must be signed in to change notification settings - Fork 90
Add several new analyzers #603
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
Conversation
# Conflicts: # doc/analyzers/index.md # src/Microsoft.VisualStudio.Composition.Analyzers/AnalyzerReleases.Unshipped.md # src/Microsoft.VisualStudio.Composition.Analyzers/README.md # src/Microsoft.VisualStudio.Composition.Analyzers/Strings.Designer.cs # src/Microsoft.VisualStudio.Composition.Analyzers/Strings.resx
AArnott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. Thanks so much.
There's a lot of code here though, so I have quite a few comments.
...Studio.Composition.Analyzers.Tests/VSMEF004ExportWithoutImportingConstructorAnalyzerTests.cs
Show resolved
Hide resolved
...Studio.Composition.Analyzers.Tests/VSMEF004ExportWithoutImportingConstructorAnalyzerTests.cs
Outdated
Show resolved
Hide resolved
...Studio.Composition.Analyzers.Tests/VSMEF004ExportWithoutImportingConstructorAnalyzerTests.cs
Show resolved
Hide resolved
...Studio.Composition.Analyzers.Tests/VSMEF004ExportWithoutImportingConstructorAnalyzerTests.cs
Outdated
Show resolved
Hide resolved
...Studio.Composition.Analyzers.Tests/VSMEF004ExportWithoutImportingConstructorAnalyzerTests.cs
Outdated
Show resolved
Hide resolved
...icrosoft.VisualStudio.Composition.Analyzers/VSMEF005MultipleImportingConstructorsAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.VisualStudio.Composition.Analyzers/VSMEF007DuplicateImportAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.VisualStudio.Composition.Analyzers/VSMEF007DuplicateImportAnalyzer.cs
Outdated
Show resolved
Hide resolved
....Composition.Analyzers.CodeFixes/VSMEF004ExportWithoutImportingConstructorCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
....Composition.Analyzers.CodeFixes/VSMEF004ExportWithoutImportingConstructorCodeFixProvider.cs
Show resolved
Hide resolved
Matches configured code style and addresses diagnostic.
Addresses TODO. Small change in code fix to put this constructor first, and apply formatting.
Add a bunch more test cases. Also some pattern matching and other clean up.
Don't warn about nullable value types (int?) or AllowDefault on value types, since these have different semantics than nullable reference types.
AArnott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better. Do you have ideas for resolving the last two comments?
|
I believe all feedback has been addressed. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
AArnott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions about InheritedExportAttribute. Ideally you can support that as well. But if you'd rather not, let's just be sure it results in too few diagnostics rather than false positives.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@drewnoakes do you have a preference to merge vs. squash? |
|
I'm gonna gamble that you want the PR completed either way. :) Thank you very much for your contribution! |
|
Are packages created automatically? Once a version is available, I can try it in a few repos I own and give it a bit more validation. |
|
Typically yes, they would show up here. |
|
Did the optprof run work? I'm not seeing a new analyzers package yet. |
|
No. Now we're in this weird catch-22 world where the optprof run failed because of the new analyzers. |
|
Thanks for the pointer. So far I've only seen false positives. Will get a fix together over the next day or so I hope. Let me know if this is blocking anything important and I'll try and accelerate the timeline. |
|
Fixed in #648 |
|
Was there any progress on the OptProf run unblocking a new package version? If there are still build failures, I'll take a look (though I'm not sure where to find them). |
|
I'm working on the OptProf issues this week (I was OOF for the prior two weeks). They aren't build failures. They are failures in Optprof tests that I have no insight into. We're switching OptProf systems to resolve. |
Fixes #257
Fixes #287
Fixes #317
New analyzers for:
[ImportingConstructor][ImportingConstructor][Import]AllowDefaultMostly coded using GitHub Copilot.