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

Category from Roslyn SARIF 2.1.0 log #155

Closed
KalleOlaviNiemitalo opened this issue Jul 24, 2022 · 4 comments
Closed

Category from Roslyn SARIF 2.1.0 log #155

KalleOlaviNiemitalo opened this issue Jul 24, 2022 · 4 comments

Comments

@KalleOlaviNiemitalo
Copy link

Make SarifParser read the properties.category property that the Roslyn compiler can write into reportingDescriptor objects in SARIF 2.1.0 logs, and call ViolationBuilder.setCategory if the value is a string.

The SARIF 2.1.0 standard does not specify the format or meaning of the properties.category property. It just says that properties is allowed there and the value is a property bag (section 3.8). Regardless, if properties.category is there and has a string value, then I think it is better to use it than leave the category unset.

Taxonomies (section 3.19.3) are a similar concept that is more rigorously defined in the SARIF 2.1.0 standard. However, because each diagnostic rule can have relationships to multiple taxa (in the same taxonomy or in separate taxonomies), it would not be obvious how to choose just one taxon as the category of a Violation. Also, Roslyn never writes any reportingDescriptorRelationship objects to SARIF logs. For those reasons, I'm not proposing support for taxonomies in this issue.

@tomasbjerre
Copy link
Owner

Im putting this on my todo list. If you can supply an example, that qould be great

@KalleOlaviNiemitalo
Copy link
Author

Example log with several categories and a suppressed warning:

log.sarif
{
  "$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": "CA1847",
          "ruleIndex": 1,
          "level": "warning",
          "message": {
            "text": "Use 'string.Contains(char)' instead of 'string.Contains(string)' when searching for a single character"
          },
          "suppressions": [
            {
              "kind": "inSource"
            }
          ],
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "file:///C:/Projects/SarifCategoryDemo/Class1.cs"
                },
                "region": {
                  "startLine": 8,
                  "startColumn": 24,
                  "endLine": 8,
                  "endColumn": 27
                }
              }
            }
          ],
          "properties": {
            "warningLevel": 1
          }
        },
        {
          "ruleId": "CA2201",
          "ruleIndex": 2,
          "level": "warning",
          "message": {
            "text": "Exception type System.Exception is not sufficiently specific"
          },
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "file:///C:/Projects/SarifCategoryDemo/Class1.cs"
                },
                "region": {
                  "startLine": 11,
                  "startColumn": 19,
                  "endLine": 11,
                  "endColumn": 78
                }
              }
            }
          ],
          "properties": {
            "warningLevel": 1
          }
        },
        {
          "ruleId": "CA1305",
          "ruleIndex": 3,
          "level": "warning",
          "message": {
            "text": "The behavior of 'string.Format(string, object)' could vary based on the current user's locale settings. Replace this call in 'Class1.M(string)' with a call to 'string.Format(IFormatProvider, string, params object[])'."
          },
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "file:///C:/Projects/SarifCategoryDemo/Class1.cs"
                },
                "region": {
                  "startLine": 11,
                  "startColumn": 33,
                  "endLine": 11,
                  "endColumn": 77
                }
              }
            }
          ],
          "properties": {
            "warningLevel": 1
          }
        },
        {
          "ruleId": "CA1307",
          "ruleIndex": 4,
          "level": "warning",
          "message": {
            "text": "'string.Contains(string)' has a method overload that takes a 'StringComparison' parameter. Replace this call in 'SarifCategoryDemo.Class1.M(string)' with a call to 'string.Contains(string, System.StringComparison)' for clarity of intent."
          },
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "file:///C:/Projects/SarifCategoryDemo/Class1.cs"
                },
                "region": {
                  "startLine": 8,
                  "startColumn": 13,
                  "endLine": 8,
                  "endColumn": 28
                }
              }
            }
          ],
          "properties": {
            "warningLevel": 1
          }
        },
        {
          "ruleId": "CA1822",
          "ruleIndex": 5,
          "level": "warning",
          "message": {
            "text": "Member 'M' does not access instance data and can be marked as static"
          },
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "file:///C:/Projects/SarifCategoryDemo/Class1.cs"
                },
                "region": {
                  "startLine": 5,
                  "startColumn": 10,
                  "endLine": 5,
                  "endColumn": 11
                }
              }
            }
          ],
          "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": "CA1847",
              "shortDescription": {
                "text": "Use char literal for a single character lookup"
              },
              "fullDescription": {
                "text": "'string.Contains(char)' is available as a better performing overload for single char lookup."
              },
              "defaultConfiguration": {
                "level": "note"
              },
              "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1847",
              "properties": {
                "category": "Performance",
                "tags": [
                  "Telemetry",
                  "EnabledRuleInAggressiveMode"
                ]
              }
            },
            {
              "id": "CA2201",
              "shortDescription": {
                "text": "Do not raise reserved exception types"
              },
              "fullDescription": {
                "text": "An exception of type that is not sufficiently specific or reserved by the runtime should never be raised by user code. This makes the original error difficult to detect and debug. If this exception instance might be thrown, use a different exception type."
              },
              "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2201",
              "properties": {
                "category": "Usage",
                "tags": [
                  "PortedFromFxCop",
                  "Telemetry",
                  "EnabledRuleInAggressiveMode"
                ]
              }
            },
            {
              "id": "CA1305",
              "shortDescription": {
                "text": "Specify IFormatProvider"
              },
              "fullDescription": {
                "text": "A method or constructor calls one or more members that have overloads that accept a System.IFormatProvider parameter, and the method or constructor does not call the overload that takes the IFormatProvider parameter. When a System.Globalization.CultureInfo or IFormatProvider object is not supplied, the default value that is supplied by the overloaded member might not have the effect that you want in all locales. If the result will be based on the input from/output displayed to the user, specify 'CultureInfo.CurrentCulture' as the 'IFormatProvider'. Otherwise, if the result will be stored and accessed by software, such as when it is loaded from disk/database and when it is persisted to disk/database, specify 'CultureInfo.InvariantCulture'."
              },
              "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305",
              "properties": {
                "category": "Globalization",
                "tags": [
                  "PortedFromFxCop",
                  "Telemetry",
                  "EnabledRuleInAggressiveMode"
                ]
              }
            },
            {
              "id": "CA1307",
              "shortDescription": {
                "text": "Specify StringComparison for clarity"
              },
              "fullDescription": {
                "text": "A string comparison operation uses a method overload that does not set a StringComparison parameter. It is recommended to use the overload with StringComparison parameter for clarity of intent. If the result will be displayed to the user, such as when sorting a list of items for display in a list box, specify 'StringComparison.CurrentCulture' or 'StringComparison.CurrentCultureIgnoreCase' as the 'StringComparison' parameter. If comparing case-insensitive identifiers, such as file paths, environment variables, or registry keys and values, specify 'StringComparison.OrdinalIgnoreCase'. Otherwise, if comparing case-sensitive identifiers, specify 'StringComparison.Ordinal'."
              },
              "defaultConfiguration": {
                "enabled": false
              },
              "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1307",
              "properties": {
                "category": "Globalization",
                "tags": [
                  "PortedFromFxCop",
                  "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"
    }
  ]
}

Generated by compiling with .NET SDK 6.0.302:

SarifCategoryDemo.csproj
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <ErrorLog>log.sarif,version=2.1</ErrorLog>
    <AnalysisLevel>6.0-all</AnalysisLevel>
  </PropertyGroup>

</Project>
Class1.cs
namespace SarifCategoryDemo;

public class Class1
{
    void M(string s)
    {
#pragma warning disable CA1847 // Demonstrate suppression in SARIF log
        if (s.Contains("i"))
#pragma warning restore CA1847
        {
            throw new Exception(string.Format("String contains 'i': {0}", s));
        }
    }
}

Incidentally, SARIF Validator does not notice that this log violates the following requirement in SARIF 2.1.0 section 3.27.23:

The suppressions values for all result objects in theRun SHALL be either all null or all non-null.

@KalleOlaviNiemitalo
Copy link
Author

Filed dotnet/roslyn#62894 about fixing the SARIF violation in Roslyn, and microsoft/sarif-sdk#2508 about making the validator detect it.

@tomasbjerre
Copy link
Owner

I am releasing this now. Open issue again if any problems!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants