Prevent use of language-specific symbols in language-agnostic layers #4564
Conversation
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.CodeAnalysis" Version="$(MicrosoftCodeAnalysisVersion)" /> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.Workspaces.Common" Version="$(MicrosoftCodeAnalysisVersion)" /> |
There was a problem hiding this comment.
I couldn't find how to have a conditional include here that would say include the C# workspace nuget if the project name (or assembly name) contains CSharp, include VB for vb and fallback to the common.
@sharwell any idea?
There was a problem hiding this comment.
IMO the projitems should never have ProjectReference, but other people feel differently about this. There certainly isn't "one right way".
There was a problem hiding this comment.
I think the most common expectation would be Workspaces.Utilities references language-agnostic only, and Workspaces.Utilities.CSharp references the C# assemblies (i.e. split the shared projects the same way normal assemblies are split).
There was a problem hiding this comment.
I also prefer not to have references in shared projects and to be honest with new project system I have stopped using shared project, I simply add manual reference to the files/folder I want to share. But I also understand how it makes it easier to share this common dependency.
Regarding Workspaces.Utilities.CSharp we currently don't have it and don't have C# specific code. I was thinking about a Choose just to help with avoiding the reference on each project.
If you all agree then I suggest to keep what I have done in this PR, otherwise I am open to suggestion :)
There was a problem hiding this comment.
I have no preference here - I am fine with whatever is easier for you @Evangelink :-)
src/Microsoft.CodeAnalysis.Analyzers/CSharp/Microsoft.CodeAnalysis.CSharp.Analyzers.csproj
Show resolved
Hide resolved
src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpDoNotUseStackallocInLoops.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Security/Helpers/InsecureObjectGraphResult.cs
Outdated
Show resolved
Hide resolved
...Core.Analyzers/Security/CSharpDataSetDataTableInIFormatterSerializableObjectGraphAnalyzer.cs
Show resolved
Hide resolved
| @@ -78,7 +77,7 @@ public Location GetLocation() | |||
| /// Gets the display string of <see cref="InsecureSymbol"/> or <see cref="InsecureAttribute"/>. | |||
| /// </summary> | |||
| /// <returns>Display string of <see cref="InsecureSymbol"/> or <see cref="InsecureAttribute"/>.</returns> | |||
| public string GetDisplayString() | |||
| public string GetDisplayString(Func<TypedConstant, string> typedConstantToString) | |||
There was a problem hiding this comment.
@dotpaul I went for a bit "hacky" solution but trying to have this class abstract was causing too many changes and it was hard to by-pass the static GetOrCreate method.
|
Hum I guess that this needs to be changed to target |
Codecov Report
@@ Coverage Diff @@
## master #4564 +/- ##
==========================================
- Coverage 95.87% 95.85% -0.02%
==========================================
Files 1182 1193 +11
Lines 268934 268953 +19
Branches 16232 16222 -10
==========================================
- Hits 257838 257812 -26
- Misses 9015 9072 +57
+ Partials 2081 2069 -12 |
|
Not sure what is going-on here, I can't repro locally. |
|
|
||
| Rule ID | Category | Severity | Notes | ||
| --------|----------|----------|------- | ||
| CA2014 | Reliability | Warning | CSharpDoNotUseStackallocInLoopsAnalyzer, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2014) |
There was a problem hiding this comment.
If you define a base type DoNotUseStackallocInLoopsAnalyzer in the language-agnostic layer and have the descriptor in this base type instead of the C# specific type, then you will not need any changes to any of the Analyzer releases file. This will also ensure that all analyzer releases and released analyzers are in a single file in the core project. Would you be able to make this change?
There was a problem hiding this comment.
Overall LGTM. I'd like to avoid any changes to AnalyzerReleases as part of the PR by avoiding moving descriptors to language specific layer. I can approve and merge once that is done.
Thanks for the work @Evangelink!
|
Tagging @jmarolf again - seems like random tests in |
Fix #4323