KalleOlaviNiemitalo
commented
2 years ago The bug is here:
According to SARIF 2.1.0, the same run must not have both results where the suppressions property value is an array, and results where the property is omitted or the value is null. The easiest fix would be to write an empty array to each result that is not suppressed. Alternatively, you could omit the properties if no results in the run are suppressed, but then you'd have to buffer the diagnostics rather than write them immediately in the public LogDiagnostic method.
In SARIF 1.0.0 however, the suppressionStates property shall be omitted if the resujlt is not suppressed. SarifV1ErrorLogger is correct in this respect.
https://github.com/sarif-standard/sarif-spec-v1/blob/2f03d36da4db9242ca853cd259b0afe2c121703e/document/FileFormat.mdk#L1738-L1745 https://github.com/dotnet/roslyn/blob/35fda5cf56445624cb0837b8b9ddbd9aa8f98b95/src/Compilers/Core/Portable/CommandLine/SarifV1ErrorLogger.cs#L73-L78
vatsalyaagrawal
Version Used: Roslyn 4.2.0-4.22220.5 (432d17a8) in .NET SDK 6.0.302
Steps to Reproduce:
dotnet buildthe following project.SarifSuppressionDemo.csproj
```XMLClass1.cs
```C# namespace SarifSuppressionDemo; using System.Diagnostics.CodeAnalysis; public class Class1 { [SuppressMessage("Microsoft.CodeQuality.Analyzers", "CA1822:MarkMembersAsStatic", Justification = "Demonstrate suppression in SARIF log.")] public int M(string s) => s.Length; } ```Examine the generated
log.sariffile.Expected Behavior:
Every
resultobject inruns[0].resultsshould have asuppressionsproperty with an array value, the array being empty if the result is not suppressed.Expected log.sarif
```JSON { "$schema": "http://json.schemastore.org/sarif-2.1.0", "version": "2.1.0", "runs": [ { "results": [ { "ruleId": "CA1014", "ruleIndex": 0, "level": "warning", "message": { "text": "Mark assemblies with CLSCompliant" }, "suppressions": [], "properties": { "warningLevel": 1 } }, { "ruleId": "CA1062", "ruleIndex": 1, "level": "warning", "message": { "text": "In externally visible method 'int Class1.M(string s)', validate parameter 's' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument." }, "suppressions": [], "locations": [ { "physicalLocation": { "artifactLocation": { "uri": "file:///C:/Projects/SarifSuppressionDemo/Class1.cs" }, "region": { "startLine": 8, "startColumn": 31, "endLine": 8, "endColumn": 32 } } } ], "properties": { "warningLevel": 1 } }, { "ruleId": "CA1822", "ruleIndex": 2, "level": "warning", "message": { "text": "Member 'M' does not access instance data and can be marked as static" }, "suppressions": [ { "kind": "inSource", "justification": "Demonstrate suppression in SARIF log." } ], "locations": [ { "physicalLocation": { "artifactLocation": { "uri": "file:///C:/Projects/SarifSuppressionDemo/Class1.cs" }, "region": { "startLine": 8, "startColumn": 16, "endLine": 8, "endColumn": 17 } } } ], "properties": { "warningLevel": 1 } } ], "tool": { "driver": { "name": "Microsoft (R) Visual C# Compiler", "version": "4.2.0-4.22220.5 (432d17a8)", "dottedQuadFileVersion": "4.2.0.0", "semanticVersion": "4.2.0", "language": "en-US", "rules": [ { "id": "CA1014", "shortDescription": { "text": "Mark assemblies with CLSCompliant" }, "fullDescription": { "text": "The Common Language Specification (CLS) defines naming restrictions, data types, and rules to which assemblies must conform if they will be used across programming languages. Good design dictates that all assemblies explicitly indicate CLS compliance by using CLSCompliantAttribute . If this attribute is not present on an assembly, the assembly is not compliant." }, "defaultConfiguration": { "enabled": false }, "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1014", "properties": { "category": "Design", "tags": [ "PortedFromFxCop", "Telemetry", "EnabledRuleInAggressiveMode", "CompilationEnd" ] } }, { "id": "CA1062", "shortDescription": { "text": "Validate arguments of public methods" }, "fullDescription": { "text": "An externally visible method dereferences one of its reference arguments without verifying whether that argument is null (Nothing in Visual Basic). All reference arguments that are passed to externally visible methods should be checked against null. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument. If the method is designed to be called only by known assemblies, you should make the method internal." }, "defaultConfiguration": { "enabled": false }, "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1062", "properties": { "category": "Design", "tags": [ "PortedFromFxCop", "Dataflow", "Telemetry", "EnabledRuleInAggressiveMode" ] } }, { "id": "CA1822", "shortDescription": { "text": "Mark members as static" }, "fullDescription": { "text": "Members that do not access instance data or call instance methods can be marked as static. After you mark the methods as static, the compiler will emit nonvirtual call sites to these members. This can give you a measurable performance gain for performance-sensitive code." }, "defaultConfiguration": { "level": "note" }, "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1822", "properties": { "category": "Performance", "tags": [ "PortedFromFxCop", "Telemetry", "EnabledRuleInAggressiveMode" ] } } ] } }, "columnKind": "utf16CodeUnits" } ] } ```Actual Behavior:
The suppressed warning
runs[0].results[2]has asuppressionsproperty whose value is an array with one element, but the otherresultobjects do not have asuppressionsproperty. This violates the requirement in SARIF 2.1.0 section 3.27.23:Actual log.sarif
```JSON { "$schema": "http://json.schemastore.org/sarif-2.1.0", "version": "2.1.0", "runs": [ { "results": [ { "ruleId": "CA1014", "ruleIndex": 0, "level": "warning", "message": { "text": "Mark assemblies with CLSCompliant" }, "properties": { "warningLevel": 1 } }, { "ruleId": "CA1062", "ruleIndex": 1, "level": "warning", "message": { "text": "In externally visible method 'int Class1.M(string s)', validate parameter 's' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument." }, "locations": [ { "physicalLocation": { "artifactLocation": { "uri": "file:///C:/Projects/SarifSuppressionDemo/Class1.cs" }, "region": { "startLine": 8, "startColumn": 31, "endLine": 8, "endColumn": 32 } } } ], "properties": { "warningLevel": 1 } }, { "ruleId": "CA1822", "ruleIndex": 2, "level": "warning", "message": { "text": "Member 'M' does not access instance data and can be marked as static" }, "suppressions": [ { "kind": "inSource", "justification": "Demonstrate suppression in SARIF log." } ], "locations": [ { "physicalLocation": { "artifactLocation": { "uri": "file:///C:/Projects/SarifSuppressionDemo/Class1.cs" }, "region": { "startLine": 8, "startColumn": 16, "endLine": 8, "endColumn": 17 } } } ], "properties": { "warningLevel": 1 } } ], "tool": { "driver": { "name": "Microsoft (R) Visual C# Compiler", "version": "4.2.0-4.22220.5 (432d17a8)", "dottedQuadFileVersion": "4.2.0.0", "semanticVersion": "4.2.0", "language": "en-US", "rules": [ { "id": "CA1014", "shortDescription": { "text": "Mark assemblies with CLSCompliant" }, "fullDescription": { "text": "The Common Language Specification (CLS) defines naming restrictions, data types, and rules to which assemblies must conform if they will be used across programming languages. Good design dictates that all assemblies explicitly indicate CLS compliance by using CLSCompliantAttribute . If this attribute is not present on an assembly, the assembly is not compliant." }, "defaultConfiguration": { "enabled": false }, "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1014", "properties": { "category": "Design", "tags": [ "PortedFromFxCop", "Telemetry", "EnabledRuleInAggressiveMode", "CompilationEnd" ] } }, { "id": "CA1062", "shortDescription": { "text": "Validate arguments of public methods" }, "fullDescription": { "text": "An externally visible method dereferences one of its reference arguments without verifying whether that argument is null (Nothing in Visual Basic). All reference arguments that are passed to externally visible methods should be checked against null. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument. If the method is designed to be called only by known assemblies, you should make the method internal." }, "defaultConfiguration": { "enabled": false }, "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1062", "properties": { "category": "Design", "tags": [ "PortedFromFxCop", "Dataflow", "Telemetry", "EnabledRuleInAggressiveMode" ] } }, { "id": "CA1822", "shortDescription": { "text": "Mark members as static" }, "fullDescription": { "text": "Members that do not access instance data or call instance methods can be marked as static. After you mark the methods as static, the compiler will emit nonvirtual call sites to these members. This can give you a measurable performance gain for performance-sensitive code." }, "defaultConfiguration": { "level": "note" }, "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1822", "properties": { "category": "Performance", "tags": [ "PortedFromFxCop", "Telemetry", "EnabledRuleInAggressiveMode" ] } } ] } }, "columnKind": "utf16CodeUnits" } ] } ```