Skip to content
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

Update dependencies from dotnet/roslyn-analyzers, config new analyzers #99343

Merged
merged 6 commits into from
Mar 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions eng/CodeAnalysis.src.globalconfig
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,12 @@ dotnet_diagnostic.CA1512.severity = warning
# CA1513: Use ObjectDisposedException throw helper
dotnet_diagnostic.CA1513.severity = warning

# CA1514: Avoid redundant length argument
dotnet_diagnostic.CA1514.severity = warning

# CA1515: Consider making public types internal
dotnet_diagnostic.CA1515.severity = none

# CA1700: Do not name enum values 'Reserved'
dotnet_diagnostic.CA1700.severity = none

Expand Down Expand Up @@ -492,6 +498,9 @@ dotnet_diagnostic.CA1869.severity = warning
# CA1870: Use a cached 'SearchValues' instance
dotnet_diagnostic.CA1870.severity = warning

# CA1871: Do not pass a nullable struct to 'ArgumentNullException.ThrowIfNull'
dotnet_diagnostic.CA1871.severity = warning

# CA2000: Dispose objects before losing scope
dotnet_diagnostic.CA2000.severity = none

Expand Down Expand Up @@ -679,6 +688,18 @@ dotnet_diagnostic.CA2260.severity = warning
# CA2261: Do not use ConfigureAwaitOptions.SuppressThrowing with Task<TResult>
dotnet_diagnostic.CA2261.severity = warning

# CA2262: Set 'MaxResponseHeadersLength' properly
dotnet_diagnostic.CA2262.severity = warning

# CA2263: Prefer generic overload when type is known
dotnet_diagnostic.CA2263.severity = info
Copy link
Member

Choose a reason for hiding this comment

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

Is there a problem / difficulty with this diagnostic that prevents us from having it be a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a some cases where the suggested API is calling the type overload internally, I assume we would not like to flag them, let me know if you have other suggestion. There also some that looks could be more efficient and should be fixed, I will update them with this PR, overall around 200 findings with duplicates
CA2263 findings.txt

Copy link
Member

Choose a reason for hiding this comment

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

I think we should go through and prepare the change to fix them, and then evaluate any cases we believe are problematic. Two reasons:

  1. One of the reasons we aggressively consume new analyzers into dotnet/runtime is to vet the new analyzers. dotnet/runtime is a large codebase, and is a good first line of defense against problems with an analyzer's implementation or even high-level goal. It'd be good to understand which of those 200 warnings are real things to be fixed and which are noise that might demand changes to the analyzer.
  2. We're hopefully only adding analyzers we believe lead to making code better. We want that betterness in dotnet/runtime.

Copy link
Member

Choose a reason for hiding this comment

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

(That can/should certainly be separate from this PR.)

Copy link
Contributor Author

@buyaa-n buyaa-n Mar 6, 2024

Choose a reason for hiding this comment

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

I think we should go through and prepare the change to fix them, and then evaluate any cases we believe are problematic. Two reasons:

Sure, I did evaluate them and concluded that those are not issue with the analyzer and also not worth fixing in the runtime, here is the reasons, please let me know if you have any comment/suggestion:
Excluded these cases from fixing:

  1. Excluded fixing the cases when the project builds for netstandard or .NET framework where the generic overload doesn't exit
  2. Cases where the generic overload implementation just calling the Type overload, like: public T CreateDelegate<T>() where T : Delegate => (T)CreateDelegate(typeof(T));. I think there is nothing to fix in the analyzer, unless we want to check the implementation, which I did not think worth a fix, because the analyzer suggestion is more about AOT compliance, external applications should change to use the generic overload if they care about AOT or similar tooling
  3. The generic overload implementation using typeof(T) internally (this is kind of similar to the case 2 but the implementation not exactly calling the Type overload). For example: Marshal.SizeOf<T>(), Marshal.PtrToStructure<T>(nint) etc. By my evaluation these doesn't look worth fixing, but not that sure with my evaluation. No fix needed for the analyzer with the same reason as above, from AOT perspective this is not false positive.

Here are the all cases that I did not fix: CA2263 findings_NotFixed.txt

(That can/should certainly be separate from this PR.)

I have fixed the ones that worth fixing, PTAL

Copy link
Contributor Author

@buyaa-n buyaa-n Mar 11, 2024

Choose a reason for hiding this comment

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

This analyzer is first and foremost a code style analyzer/fixer. I do not think it is useless, but I am not sure whether it should be enabled by default. Do we have a prior art for analyzers like that?

Then this rule should start with "IDE" prefix https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/categories#style-rules. I think "IDE" rules should not added in roslyn-analyzers repo, probably in roslyn?

Not sure we could move there ...

Copy link
Member

Choose a reason for hiding this comment

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

In some cases, the fix suggested by the analyzer may result in a minor performance improvement. Example: Replacing Enum.GetName with EnumGetName<TEnum> will avoid boxing. I am not aware of anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some cases, the fix suggested by the analyzer may result in a minor performance improvement. Example: Replacing Enum.GetName with EnumGetName<TEnum> will avoid boxing. I am not aware of anything else.

Yes, and that is why I fixed those cases with this PR, thought the cases where the generic overload implementation just calling the Type overload, will have some perf degradation, for other case like Marshal.SizeOf<T>() and Marshal.SizeOf(Type) the perf impact was not obvious, so did not fix. Anyway, I don't think with the analyzer we could evaluate the perf impact.

Based on your and @stephentoub's feedback I put up a PR that fixes all cases excluding the projects that target down-level platforms and System.Linq.Expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This analyzer is first and foremost a code style analyzer/fixer.

If that's really the case, we shouldn't have it at all; we're generally not in the business of including style analyzers in this library.

But is that really the case?

I think that is the case, so what we should do with the analyzer now?

Copy link
Member

Choose a reason for hiding this comment

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

I think that is the case, so what we should do with the analyzer now?

The fact that it catches things like changing Enum calls from non-generic to generic makes me still excited about it. I was just fixing up an internal codebase and found it useful specifically for that.


# CA2264: Do not pass a non-nullable value to 'ArgumentNullException.ThrowIfNull'
dotnet_diagnostic.CA2264.severity = warning

# CA2265: Do not compare Span<T> to 'null' or 'default'
dotnet_diagnostic.CA2265.severity = warning

# CA2300: Do not use insecure deserializer BinaryFormatter
dotnet_diagnostic.CA2300.severity = none

Expand Down
21 changes: 21 additions & 0 deletions eng/CodeAnalysis.test.globalconfig
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,12 @@ dotnet_diagnostic.CA1512.severity = none
# CA1513: Use ObjectDisposedException throw helper
dotnet_diagnostic.CA1513.severity = none

# CA1514: Avoid redundant length argument
dotnet_diagnostic.CA1514.severity = none

# CA1515: Consider making public types internal
dotnet_diagnostic.CA1515.severity = none

# CA1700: Do not name enum values 'Reserved'
dotnet_diagnostic.CA1700.severity = none

Expand Down Expand Up @@ -489,6 +495,9 @@ dotnet_diagnostic.CA1869.severity = none
# CA1870: Use a cached 'SearchValues' instance
dotnet_diagnostic.CA1870.severity = none

# CA1871: Do not pass a nullable struct to 'ArgumentNullException.ThrowIfNull'
dotnet_diagnostic.CA1871.severity = none

# CA2000: Dispose objects before losing scope
dotnet_diagnostic.CA2000.severity = none

Expand Down Expand Up @@ -675,6 +684,18 @@ dotnet_diagnostic.CA2260.severity = none
# CA2261: Do not use ConfigureAwaitOptions.SuppressThrowing with Task<TResult>
dotnet_diagnostic.CA2261.severity = none

# CA2262: Set 'MaxResponseHeadersLength' properly
dotnet_diagnostic.CA2262.severity = none

# CA2263: Prefer generic overload when type is known
dotnet_diagnostic.CA2263.severity = none

# CA2264: Do not pass a non-nullable value to 'ArgumentNullException.ThrowIfNull'
dotnet_diagnostic.CA2264.severity = none

# CA2265: Do not compare Span<T> to 'null' or 'default'
dotnet_diagnostic.CA2265.severity = none

# CA2300: Do not use insecure deserializer BinaryFormatter
dotnet_diagnostic.CA2300.severity = none

Expand Down
8 changes: 4 additions & 4 deletions eng/Version.Details.xml
Original file line number Diff line number Diff line change
Expand Up @@ -404,13 +404,13 @@
<Uri>https://github.com/dotnet/roslyn</Uri>
<Sha>77372c66fd54927312b5b0a2e399e192f74445c9</Sha>
</Dependency>
<Dependency Name="Microsoft.CodeAnalysis.Analyzers" Version="3.11.0-beta1.24122.2">
<Dependency Name="Microsoft.CodeAnalysis.Analyzers" Version="3.11.0-beta1.24155.1">
<Uri>https://github.com/dotnet/roslyn-analyzers</Uri>
<Sha>ba8b7f2c3ae092d0301f0c5e49bd30340af553c8</Sha>
<Sha>628d239421a3ae54464c60901fd9b831b3f01efa</Sha>
</Dependency>
<Dependency Name="Microsoft.CodeAnalysis.NetAnalyzers" Version="9.0.0-preview.24122.2">
<Dependency Name="Microsoft.CodeAnalysis.NetAnalyzers" Version="9.0.0-preview.24155.1">
<Uri>https://github.com/dotnet/roslyn-analyzers</Uri>
<Sha>ba8b7f2c3ae092d0301f0c5e49bd30340af553c8</Sha>
<Sha>628d239421a3ae54464c60901fd9b831b3f01efa</Sha>
</Dependency>
<!-- Intermediate is necessary for source build. -->
<Dependency Name="Microsoft.SourceBuild.Intermediate.roslyn" Version="4.10.0-2.24114.13">
Expand Down
4 changes: 2 additions & 2 deletions eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
</ItemGroup>
<PropertyGroup>
<!-- dotnet/roslyn-analyzers dependencies -->
<MicrosoftCodeAnalysisAnalyzersVersion>3.11.0-beta1.24122.2</MicrosoftCodeAnalysisAnalyzersVersion>
<MicrosoftCodeAnalysisNetAnalyzersVersion>9.0.0-preview.24122.2</MicrosoftCodeAnalysisNetAnalyzersVersion>
<MicrosoftCodeAnalysisAnalyzersVersion>3.11.0-beta1.24155.1</MicrosoftCodeAnalysisAnalyzersVersion>
<MicrosoftCodeAnalysisNetAnalyzersVersion>9.0.0-preview.24155.1</MicrosoftCodeAnalysisNetAnalyzersVersion>
<!-- dotnet/roslyn dependencies -->
<!--
These versions should not be used by any project that contributes to the design-time experience in VS, such as an analyzer, code-fix, or generator assembly.
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/tools/Common/Compiler/InstructionSetSupport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ public class InstructionSetSupportBuilder
private static Dictionary<TargetArchitecture, Dictionary<string, InstructionSet>> ComputeInstructionSetSupport()
{
var supportMatrix = new Dictionary<TargetArchitecture, Dictionary<string, InstructionSet>>();
foreach (TargetArchitecture arch in Enum.GetValues(typeof(TargetArchitecture)))
foreach (TargetArchitecture arch in Enum.GetValues<TargetArchitecture>())
{
supportMatrix[arch] = ComputeInstructSetSupportForArch(arch);
}
Expand All @@ -186,7 +186,7 @@ private static Dictionary<TargetArchitecture, Dictionary<string, InstructionSet>
private static Dictionary<TargetArchitecture, InstructionSetFlags> ComputeNonSpecifiableInstructionSetSupport()
{
var matrix = new Dictionary<TargetArchitecture, InstructionSetFlags>();
foreach (TargetArchitecture arch in Enum.GetValues(typeof(TargetArchitecture)))
foreach (TargetArchitecture arch in Enum.GetValues<TargetArchitecture>())
{
matrix[arch] = ComputeNonSpecifiableInstructionSetSupportForArch(arch);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ public partial class MethodSemantics
{
public override string ToString()
{
string str = Enum.GetName(typeof(MethodSemanticsAttributes), Attributes);
string str = Enum.GetName(Attributes);
return str + " : " + Method.ToString();
}
}
Expand Down Expand Up @@ -903,7 +903,7 @@ public partial class PropertySignature
{
public override string ToString()
{
return string.Join(" ", Enum.GetName(typeof(CallingConventions), CallingConvention),
return string.Join(" ", Enum.GetName(CallingConvention),
Type.ToString()) + "(" + ToString(Parameters) + ")";
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/libraries/Common/src/System/Resources/ResourceWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,11 @@ private static ResourceTypeCode FindTypeCode(object? value, List<string> types)
if (typeName.StartsWith("ResourceTypeCode.", StringComparison.Ordinal))
{
typeName = typeName.Substring(17); // Remove through '.'
#if NETCOREAPP
ResourceTypeCode typeCode = Enum.Parse<ResourceTypeCode>(typeName);
#else
ResourceTypeCode typeCode = (ResourceTypeCode)Enum.Parse(typeof(ResourceTypeCode), typeName);
#endif
return typeCode;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public class DatabaseGeneratedAttribute : Attribute
/// <param name="databaseGeneratedOption">The pattern used to generate values for the property in the database.</param>
public DatabaseGeneratedAttribute(DatabaseGeneratedOption databaseGeneratedOption)
{
if (!(Enum.IsDefined(typeof(DatabaseGeneratedOption), databaseGeneratedOption)))
if (!Enum.IsDefined(databaseGeneratedOption))
{
throw new ArgumentOutOfRangeException(nameof(databaseGeneratedOption));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ private static void InitializingTraceSource(object sender, InitializingTraceSour

if (!string.IsNullOrEmpty(sourceElement.SwitchValue))
{
traceSource.Switch.Level = (SourceLevels)Enum.Parse(typeof(SourceLevels), sourceElement.SwitchValue);
traceSource.Switch.Level = Enum.Parse<SourceLevels>(sourceElement.SwitchValue);
}
}
}
Expand All @@ -74,7 +74,7 @@ private static void InitializingTraceSource(object sender, InitializingTraceSour
// The SwitchValue changed; just update our internalSwitch.
if (!string.IsNullOrEmpty(sourceElement.SwitchValue))
{
traceSource.Switch.Level = (SourceLevels)Enum.Parse(typeof(SourceLevels), sourceElement.SwitchValue);
traceSource.Switch.Level = Enum.Parse<SourceLevels>(sourceElement.SwitchValue);
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public EventLogEntryType EntryType
get => _entryType;
set
{
if (!Enum.IsDefined(typeof(EventLogEntryType), value))
if (!Enum.IsDefined(value))
throw new InvalidEnumArgumentException(nameof(EntryType), (int)value, typeof(EventLogEntryType));

_entryType = value;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1292,7 +1292,7 @@ public void WriteEntry(string message, EventLogEntryType type, int eventID, shor
if (Source.Length == 0)
throw new ArgumentException(SR.NeedSourceToWrite);

if (!Enum.IsDefined(typeof(EventLogEntryType), type))
if (!Enum.IsDefined(type))
throw new InvalidEnumArgumentException(nameof(type), (int)type, typeof(EventLogEntryType));

string currentMachineName = machineName;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public PerformanceCounterType CounterType
}
set
{
if (!Enum.IsDefined(typeof(PerformanceCounterType), value))
if (!Enum.IsDefined(value))
throw new InvalidEnumArgumentException(nameof(PerformanceCounterType), (int)value, typeof(PerformanceCounterType));

_counterType = value;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5498,7 +5498,7 @@ private string CreateManifestString()
if (channelInfo.Attribs != null)
{
EventChannelAttribute attribs = channelInfo.Attribs;
if (Enum.IsDefined(typeof(EventChannelType), attribs.EventChannelType))
if (Enum.IsDefined(attribs.EventChannelType))
channelType = attribs.EventChannelType.ToString();
enabled = attribs.Enabled;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ protected override QilNode VisitQilExpression(QilExpression qil)
foreach (QilNode n in fdecls)
{
// i.e. <Function id="$a"/>
this.writer.WriteStartElement(Enum.GetName(typeof(QilNodeType), n.NodeType)!);
this.writer.WriteStartElement(Enum.GetName(n.NodeType)!);
this.writer.WriteAttributeString("id", _ngen.NameOf(n));
WriteXmlType(n);

Expand Down Expand Up @@ -277,7 +277,7 @@ protected override void BeforeVisit(QilNode node)
WriteAnnotations(node.Annotation);

// Call WriteStartElement
this.writer.WriteStartElement("", Enum.GetName(typeof(QilNodeType), node.NodeType)!, "");
this.writer.WriteStartElement("", Enum.GetName(node.NodeType)!, "");

// Write common attributes
#if QIL_TRACE_NODE_CREATION
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ internal ref struct RegexWriter
#if DEBUG
static RegexWriter()
{
Debug.Assert(!Enum.IsDefined(typeof(RegexNodeKind), BeforeChild));
Debug.Assert(!Enum.IsDefined(typeof(RegexNodeKind), AfterChild));
Debug.Assert(!Enum.IsDefined(BeforeChild));
Debug.Assert(!Enum.IsDefined(AfterChild));
}
#endif

Expand Down
4 changes: 1 addition & 3 deletions src/tasks/Common/FileCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ public FileCache(string? cacheFilePath, TaskLoggingHelper log)
Enabled = true;
if (File.Exists(cacheFilePath))
{
_oldCache = (CompilerCache?)JsonSerializer.Deserialize(File.ReadAllText(cacheFilePath),
typeof(CompilerCache),
s_jsonOptions);
_oldCache = JsonSerializer.Deserialize<CompilerCache>(File.ReadAllText(cacheFilePath), s_jsonOptions);
}

_oldCache ??= new();
Expand Down
Loading