Update SA1121 and SA1404 to detect global using aliases, using GetImportScopes and compiling expression trees #3678
Conversation
|
Tests will fail. That can be fixed and code cleaned up later on. I am more interested in the big picture right now. Did you have something like this in mind, @sharwell? I haven't wrapped my head around your light-up code completely. I consider my own new code more or less unreadable, but it seems to work :-) I assume that some of it could be generated, but that is beyond me at the moment. Note that GetImportScopes wasn't available until Roslyn 4.3.0, so with this approach the original issue isn't really resolved perfectly. Are you ok with these rules being broken for Roslyn versions between 4.0.0 (introduction of c# 10) and 4.3.0 (introduction of GetImportScopes)? Regarding the failing tests: C# 10 tests fail because of the issue with GetImportScopes. What to do?
|
bjornhellander
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3678 +/- ##
==========================================
+ Coverage 94.37% 97.45% +3.08%
==========================================
Files 924 930 +6
Lines 109815 110055 +240
Branches 3302 3308 +6
==========================================
+ Hits 103635 107255 +3620
+ Misses 5170 1836 -3334
+ Partials 1010 964 -46 |
bjornhellander
force-pushed
the
feature/global-using-alias-3
branch
from
0d855cd
to
7ffc577
Compare
|
I moved the tests to the c# 11 test project, because of reasons listed above. Ok? |
That seems fine |
...p.Analyzers/StyleCop.Analyzers.Test.CSharp11/Lightup/IImportScopeWrapperCSharp11UnitTests.cs
Outdated
Show resolved
Hide resolved
| using StyleCop.Analyzers.Lightup; | ||
| using Xunit; | ||
|
|
||
| public class IImportScopeWrapperCSharp11UnitTests |
There was a problem hiding this comment.
I think that I missed the existing lightup test pattern. Is the idea that there are tests for each language version? Simple tests for c# 6 that verify fallback behavior etc, inherit that one for c#7 through 10, and "cut the inheritance chain" with this new test class for c# 11 (and inherit that one for c# 12 like I have done)?
There was a problem hiding this comment.
I updated this, but didn't manage to make it play nicely together with the DerivedTestGenerator the way I described it above. I instead ended up with inheriting all the way from the c# 7 project and up. Does it look ok anyway?
StyleCop.Analyzers/StyleCop.Analyzers/Helpers/SyntaxTreeHelpers.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public static bool SupportsGetImportScopes { get; } | ||
|
|
||
| public static ImmutableArray<IImportScopeWrapper> GetImportScopes(this SemanticModel semanticModel, int position, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
I didn't add any lightup tests for this class, but relied on the SA1121 and SA1404 tests instead. Does that feel good enough? Feels slightly more complicated to add these as well, since I would need to get a SemanticModel instance somehow.
This comment was marked as resolved.
This comment was marked as resolved.
| var obj = CreateImportScope(numberOfAliasSymbols); | ||
| Assert.True(IImportScopeWrapper.IsInstance(obj)); | ||
| var wrapper = IImportScopeWrapper.FromObject(obj); | ||
| Assert.Equal(obj.Aliases, wrapper.Aliases); |
There was a problem hiding this comment.
I updated this test to verify the actual alias symbols returned from the wrapper. Used the Moq package to be able to create IAliasSymbol instances more easily. Ok to add that dependency?
|
Any more comments on this one, @sharwell? I added comments for some concerns that I have myself. |
bjornhellander
force-pushed
the
feature/global-using-alias-3
branch
from
2a49344
to
41b03e7
Compare
bjornhellander
force-pushed
the
feature/global-using-alias-3
branch
from
41b03e7
to
65ab771
Compare
|
Rebased and squashed. |
|
Sorry for the delays, will try to get to this one soon. The occasional pings are welcome. |
Another attempt to fix #3594.
SA1404 also suffered from the same problem.