lint/output: SARIF output is incorrect when a parse error is encountered

[charlieegan3]

When Regal is requested to format linting results in SARIF format, and there is a parse error, the output returns is not a valid SARIF json:

Desktop $ cat p.rego
package policy

allow if {
    input.request.method == "GET"
    input.request.path == ["users"]
}
Desktop $ regal lint --format=sarif p.rego
{
  "errors": [
    "error(s) encountered while linting: errors encountered when reading files to lint: failed to parse 1 module(s) — first error: 1 error occurred: p.rego:3: rego_parse_error: var cannot be used for rule name"
  ]
}

In such cases, I understand the output should set an invocations field and use that to present errors.

{
  "version": "2.1.0",
  "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/main/sarif-2.1/schema/sarif-schema-2.1.0.json",
  "runs": [
    {
      "tool": {
        "driver": {
          "informationUri": "https://docs.styra.com/regal",
          "name": "Regal",
          "version": "0.24.0",
          "rules": []
        }
      },
      "invocations": [
        {
          "executionSuccessful": false,
          "toolExecutionNotifications": [
            {
              "message": {
                "text": "p.rego:3: rego_parse_error: var cannot be used for rule name"
              },
              "level": "error",
              "properties": {
                "exitCode": 1
              }
            }
          ],
          "startTimeUtc": "2023-07-16T12:34:56Z",
          "endTimeUtc": "2023-07-16T12:35:00Z"
        }
      ],
      "results": []
    }
  ]
}

I think bonus points for adding an item to toolExecutionNotifications for each parse error.

[charlieegan3]

What if parse errors were converted into linter violations at all levels of the stack? We already do that in the language server.

regal/internal/lsp/lint.go

Lines 78 to 151 in 760b484

	var astErrs ast.Errors
	if errors.As(err, &astErrs) {
	for _, e := range astErrs {
	astErrors = append(astErrors, ast.Error{
	Code: e.Code,
	Message: e.Message,
	Location: e.Location,
	})
	}
	} else {
	// Check if err is a single ast.Error
	var singleAstErr *ast.Error
	if errors.As(err, &singleAstErr) {
	astErrors = append(astErrors, ast.Error{
	Code: singleAstErr.Code,
	Message: singleAstErr.Message,
	Location: singleAstErr.Location,
	})
	} else {
	// Unknown error type
	return false, fmt.Errorf("unknown error type: %T", err)
	}
	}
	diags := make([]types.Diagnostic, 0)
	for _, astError := range astErrors {
	line := max(astError.Location.Row-1, 0)
	lineLength := 1
	if line < len(lines) {
	lineLength = len(lines[line])
	}
	key := "regal/parse"
	link := "https://docs.styra.com/opa/category/rego-parse-error"
	hints, _ := hints.GetForError(err)
	if len(hints) > 0 {
	// there should only be one hint, so take the first
	key = hints[0]
	link = "https://docs.styra.com/opa/errors/" + hints[0]
	}
	//nolint:gosec
	diags = append(diags, types.Diagnostic{
	Severity: 1, // parse errors are the only error Diagnostic the server sends
	Range: types.Range{
	Start: types.Position{
	Line: uint(line),
	// we always highlight the whole line for parse errors to make them more visible
	Character: 0,
	},
	End: types.Position{
	Line: uint(line),
	Character: uint(lineLength),
	},
	},
	Message: astError.Message,
	Source: key,
	Code: strings.ReplaceAll(astError.Code, "_", "-"),
	CodeDescription: &types.CodeDescription{
	Href: link,
	},
	})
	}
	cache.SetParseErrors(fileURI, diags)
	if len(diags) == 0 {
	return false, errors.New("failed to parse module, but no errors were set as diagnostics")
	}
	return false, nil

Or is this a bad idea for some reason...

I was just thinking that the way to generate this output is much like the code we already have for parse errors in the lsp/lint functions.

[anderseknert]

Hey, that doesn't sound bad at all! I guess we could add a special category for errors? Having all issues handled in a uniform way sounds compelling to me.

[anderseknert]

Also related: #745

Those could also be dealt with the same way.