SARIF report missing physical location information

[ghost]

A SARIF report from SpotBugs 4.1.2 contains SARIF results with no physical location information, only logical location information. For example:

      "results": [
        {
          "ruleIndex": 0,
          "level": "note",
          "locations": [
            {
              "logicalLocations": [
                {
                  "kind": "type",
                  "name": "ClassReader",
                  "fullyQualifiedName": "aj/org/objectweb/asm/ClassReader.java:[line 3072]"
                }
              ]
            }
          ],
          "ruleId": "UC_USELESS_CONDITION",
          "message": {
            "arguments": [
              "frameType >= 248 (0xf8)"
            ],
            "id": "default"
          }
        },

When I was working with @KengoTODA on validating the SARIF report, the sample I saw did have physical location information, for example:

      "results": [
        {
          "ruleIndex": 0,
          "level": "note",
          "locations": [
            {
              "physicalLocation": {
                "region": {
                  "endLine": 166,
                  "startLine": 166
                },
                "artifactLocation": {
                  "uri": "edu/umd/cs/findbugs/AnalysisError.java",
                  "uriBaseId": "-176137563"
                }
              },
              "logicalLocation": [
                {
                  "kind": "type",
                  "name": "AnalysisError",
                  "fullyQualifiedName": "edu/umd/cs/findbugs/AnalysisError.java:[line 166]"
                }
              ]
            }
          ],

Is this a regression? Is full SARIF support scheduled for a later version than 4.1.2?

Thanks,
Larry

@boAndron @michaelcfanning @lukadlet

[welcome]

Thanks for opening your first issue here! 😃
Please check our contributing guideline. Especially when you report a problem, make sure you share a Minimal, Complete, and Verifiable example to reproduce it in this issue.

[ghost]

@KengoTODA, we have a team that is blocked on this issue. Could you please let me know if there is a workaround, or an ETA to address it? Can we help in any way?

Thanks,
Larry

@boAndron @michaelcfanning @lukadlet

[michaelcfanning]

Definitely, there should be a physical location to express the startLine. Note that it isn't required (but it also doesn't hurt) to include an endLine value when it matches the startLine. Fixing this would only help with log size.

More of a concern is that you shouldn't be encoding the start line in a logical name. The identify of this thing, logically, is a type but if you burn the line location into its name, you may break SARIF result matchers that are operating against the fully qualified name for matches. i.e., one of the core ways the baseline works is to lower the priority of line locations (because these can change run over over) and to raise the priority of logical names (because they tend to be more resilient/stable as code churns).