Detect uninstantiated classes used in XXXSource attributes - #1012
Detect uninstantiated classes used in XXXSource attributes#1012manfred-brands wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds additional CA1812 suppression coverage so internal classes that are instantiated indirectly by NUnit (via *Source attributes) don’t get flagged as “uninstantiated”.
Changes:
- Extend the CA1812 suppressor to attempt to detect classes referenced from
*Sourceattributes (e.g.,TestCaseSource). - Add a suppressor test covering a class used via
[TestCaseSource(typeof(...))]. - Update NUnit3003 documentation title/summary text to mention test data sources.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/nunit.analyzers/DiagnosticSuppressors/AvoidUninstantiatedInternalClassSuppressor.cs | Expands suppression logic beyond TestFixture to include *Source-referenced classes. |
| src/nunit.analyzers.tests/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressorTests.cs | Adds a unit test for the new suppression scenario. |
| documentation/NUnit3003.md | Updates rule title/config snippet text to include “TestData source”. |
| documentation/index.md | Updates the index entry text for NUnit3003. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mikkelbu
left a comment
mikkelbu
left a comment
There was a problem hiding this comment.
Thanks for the contribution @manfred-brands. What do you think about the comment from Copilot about the check uses the syntax as a string rather than using the semantic model?
We can also complete this as a first step and then use the semantic model in a subsequent enhancement?
I've approved this, but I would like to hear your thoughts about this as I guess you have thought more about this than I have.
|
Sorry @mikkelbu, I should have put this in Draft. I forgot and didn't had a chance to look into the alternative approach. I should have time this weekend. |
Add more tests Add samples to documentation
7255cc7 to
8d47af4
Compare
|
@mikkelbu I have reworked to suppressor to use the SyntaxTree instead of string and also optimized it to only loop over all source trees once. As this is still expensive and a niche operation, I put it behind a configuration option. Most use case would use static members of a class and that class should be declared static instead of suppressing the error. |
Fixes #999