Skip to content

Commit

Permalink
lsp: Ensure parse errors are saved as diagnostics correctly (#1352)
Browse files Browse the repository at this point in the history 
* lsp: Correctly extract parse errors

This also updates the server to use versioned parsing.

Signed-off-by: Charlie Egan <[email protected]>

* lsp: No code lenses if there are parse errors

Fixes #1123

Signed-off-by: Charlie Egan <[email protected]>

* lsp: use regoVersionForURI in a few more places

Signed-off-by: Charlie Egan <[email protected]>

* lsp: Add test for unknown version parsing

Signed-off-by: Charlie Egan <[email protected]>

* lsp: Use noCodeLenses const for no-code-lenses resp

Signed-off-by: Charlie Egan <[email protected]>

---------

Signed-off-by: Charlie Egan <[email protected]>
  • Loading branch information
@charlieegan3
charlieegan3 authored Jan 20, 2025
1 parent 7b693ee commit d9749f4
Show file tree
Hide file tree
Showing 3 changed files with 186 additions and 55 deletions.
61 changes: 29 additions & 32 deletions internal/lsp/lint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package lsp

import (
"context"
"encoding/json"
"errors"
"fmt"
"strings"
Expand Down Expand Up @@ -30,6 +29,7 @@ func updateParse(
store storage.Store,
fileURI string,
builtins map[string]*ast.Builtin,
version ast.RegoVersion,
) (bool, error) {
content, ok := cache.GetFileContents(fileURI)
if !ok {
Expand All @@ -43,8 +43,7 @@ func updateParse(
module *ast.Module
)

// TODO: IF not set in config, or from .manifest
module, err = rparse.ModuleUnknownVersionWithOpts(fileURI, content, rparse.ParserOptions())
module, err = rparse.ModuleWithOpts(fileURI, content, ast.ParserOptions{RegoVersion: version})
if err == nil {
// if the parse was ok, clear the parse errors
cache.SetParseErrors(fileURI, []types.Diagnostic{})
Expand Down Expand Up @@ -76,29 +75,30 @@ func updateParse(
return true, nil
}

unwrappedError := errors.Unwrap(err)

// if the module is empty, then the unwrapped error is a single parse ast.Error
// otherwise it's a slice of ast.Error.
// When a user has an empty file, we still want to show this single error.
var astErrors []astError

var parseError *ast.Error

if errors.As(unwrappedError, &parseError) {
astErrors = append(astErrors, astError{
Code: parseError.Code,
Message: parseError.Message,
Location: parseError.Location,
})
} else {
jsonErrors, err := json.Marshal(unwrappedError)
if err != nil {
return false, fmt.Errorf("failed to marshal parse errors: %w", err)
var astErrors []ast.Error

// Check if err is of type ast.Errors
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,
})
}

if err := json.Unmarshal(jsonErrors, &astErrors); err != nil {
return false, fmt.Errorf("failed to unmarshal parse errors: %w", err)
} 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)
}
}

Expand Down Expand Up @@ -147,6 +147,10 @@ func updateParse(

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
}

Expand Down Expand Up @@ -324,13 +328,6 @@ func convertReportToDiagnostics(
return fileDiags
}

// astError is copied from OPA but drop details as I (charlieegan3) had issues unmarshalling the field.
type astError struct {
Location *ast.Location `json:"location,omitempty"`
Code string `json:"code"`
Message string `json:"message"`
}

//nolint:gosec
func getRangeForViolation(item report.Violation) types.Range {
start := types.Position{
Expand Down
130 changes: 130 additions & 0 deletions internal/lsp/lint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,143 @@ import (
"reflect"
"testing"

"github.com/open-policy-agent/opa/v1/ast"

"github.com/styrainc/regal/internal/lsp/cache"
"github.com/styrainc/regal/internal/lsp/types"
"github.com/styrainc/regal/internal/parse"
"github.com/styrainc/regal/pkg/config"
"github.com/styrainc/regal/pkg/report"
)

func TestUpdateParse(t *testing.T) {
t.Parallel()

tests := map[string]struct {
fileURI string
content string

expectSuccess bool
// ParseErrors are formatted as another type/source of diagnostic
expectedParseErrors []types.Diagnostic
expectModule bool
regoVersion ast.RegoVersion
}{
"valid file": {
fileURI: "file:///valid.rego",
content: `package test
allow if { 1 == 1 }
`,
expectModule: true,
expectSuccess: true,
regoVersion: ast.RegoV1,
},
"parse error": {
fileURI: "file:///broken.rego",
content: `package test
p = true { 1 == }
`,
regoVersion: ast.RegoV1,
expectedParseErrors: []types.Diagnostic{{
Code: "rego-parse-error",
Range: types.Range{Start: types.Position{Line: 2, Character: 13}, End: types.Position{Line: 2, Character: 13}},
}},
expectModule: false,
expectSuccess: false,
},
"empty file": {
fileURI: "file:///empty.rego",
content: "",
regoVersion: ast.RegoV1,
expectedParseErrors: []types.Diagnostic{{
Code: "rego-parse-error",
Range: types.Range{Start: types.Position{Line: 0, Character: 0}, End: types.Position{Line: 0, Character: 0}},
}},
expectModule: false,
expectSuccess: false,
},
"parse error due to version": {
fileURI: "file:///valid.rego",
content: `package test
allow if { 1 == 1 }
`,
expectModule: false,
expectSuccess: false,
expectedParseErrors: []types.Diagnostic{{
Code: "rego-parse-error",
Range: types.Range{Start: types.Position{Line: 1, Character: 0}, End: types.Position{Line: 1, Character: 0}},
}},
regoVersion: ast.RegoV0,
},
"unknown rego version, rego v1 code": {
fileURI: "file:///valid.rego",
content: `package test
allow if { 1 == 1 }
`,
expectModule: true,
expectSuccess: true,
regoVersion: ast.RegoUndefined,
},
"unknown rego version, rego v0 code": {
fileURI: "file:///valid.rego",
content: `package test
allow[msg] { 1 == 1; msg := "hello" }
`,
expectModule: true,
expectSuccess: true,
regoVersion: ast.RegoUndefined,
},
}

for testName, testData := range tests {
t.Run(testName, func(t *testing.T) {
t.Parallel()

ctx := context.Background()

c := cache.NewCache()
c.SetFileContents(testData.fileURI, testData.content)

s := NewRegalStore()

success, err := updateParse(ctx, c, s, testData.fileURI, ast.BuiltinMap, testData.regoVersion)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

if success != testData.expectSuccess {
t.Fatalf("expected success to be %v, got %v", testData.expectSuccess, success)
}

_, ok := c.GetModule(testData.fileURI)
if testData.expectModule && !ok {
t.Fatalf("expected module to be set, but it was not")
}

diags, _ := c.GetParseErrors(testData.fileURI)

if len(testData.expectedParseErrors) != len(diags) {
t.Fatalf("expected %v parse errors, got %v", len(testData.expectedParseErrors), len(diags))
}

for i, diag := range testData.expectedParseErrors {
if diag.Code != diags[i].Code {
t.Errorf("expected diagnostic code to be %v, got %v", diag.Code, diags[i].Code)
}

if diag.Range.Start.Line != diags[i].Range.Start.Line {
t.Errorf("expected diagnostic start line to be %v, got %v", diag.Range.Start.Line, diags[i].Range.Start.Line)
}

if diag.Range.End.Line != diags[i].Range.End.Line {
t.Errorf("expected diagnostic end line to be %v, got %v", diag.Range.End.Line, diags[i].Range.End.Line)
}
}
})
}
}

func TestConvertReportToDiagnostics(t *testing.T) {
t.Parallel()

Expand Down
50 changes: 27 additions & 23 deletions internal/lsp/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const (

var (
noCodeActions = make([]types.CodeAction, 0)
noCodeLenses = make([]types.CodeLens, 0)
noDocumentSymbols = make([]types.DocumentSymbol, 0)
noCompletionItems = make([]types.CompletionItem, 0)
noFoldingRanges = make([]types.FoldingRange, 0)
Expand Down Expand Up @@ -281,7 +282,7 @@ func (l *LanguageServer) StartDiagnosticsWorker(ctx context.Context) {

// updateParse will not return an error when the parsing failed,
// but only when it was impossible
if _, err := updateParse(ctx, l.cache, l.regoStore, job.URI, bis); err != nil {
if _, err := updateParse(ctx, l.cache, l.regoStore, job.URI, bis, l.regoVersionForURI(job.URI)); err != nil {
l.logf(log.LevelMessage, "failed to update module for %s: %s", job.URI, err)

continue
Expand Down Expand Up @@ -450,7 +451,7 @@ func (l *LanguageServer) StartHoverWorker(ctx context.Context) {

bis := l.builtinsForCurrentCapabilities()

success, err := updateParse(ctx, l.cache, l.regoStore, fileURI, bis)
success, err := updateParse(ctx, l.cache, l.regoStore, fileURI, bis, l.regoVersionForURI(fileURI))
if err != nil {
l.logf(log.LevelMessage, "failed to update parse: %s", err)

Expand Down Expand Up @@ -643,7 +644,7 @@ func (l *LanguageServer) StartConfigWorker(ctx context.Context) {
// updating the parse here will enable things like go-to definition
// to start working right away without the need for a file content
// update to run updateParse.
if _, err = updateParse(ctx, l.cache, l.regoStore, k, bis); err != nil {
if _, err = updateParse(ctx, l.cache, l.regoStore, k, bis, l.regoVersionForURI(k)); err != nil {
l.logf(log.LevelMessage, "failed to update parse for previously ignored file %q: %s", k, err)
}
}
Expand Down Expand Up @@ -1161,14 +1162,7 @@ func (l *LanguageServer) templateContentsForFile(fileURI string) (string, error)
pkg += "_test"
}

version := ast.RegoUndefined
if l.loadedConfigAllRegoVersions != nil {
version = rules.RegoVersionFromVersionsMap(
l.loadedConfigAllRegoVersions.Clone(),
strings.TrimPrefix(uri.ToPath(l.clientIdentifier, fileURI), uri.ToPath(l.clientIdentifier, l.workspaceRootURI)),
ast.RegoUndefined,
)
}
version := l.regoVersionForURI(fileURI)

if version == ast.RegoV0 {
return fmt.Sprintf("package %s\n\nimport rego.v1\n", pkg), nil
Expand Down Expand Up @@ -1367,7 +1361,7 @@ func (l *LanguageServer) processHoverContentUpdate(ctx context.Context, fileURI

bis := l.builtinsForCurrentCapabilities()

if success, err := updateParse(ctx, l.cache, l.regoStore, fileURI, bis); err != nil {
if success, err := updateParse(ctx, l.cache, l.regoStore, fileURI, bis, l.regoVersionForURI(fileURI)); err != nil {
return fmt.Errorf("failed to update parse: %w", err)
} else if !success {
return nil
Expand Down Expand Up @@ -1659,6 +1653,11 @@ func (l *LanguageServer) handleTextDocumentInlayHint(params types.TextDocumentIn
}

func (l *LanguageServer) handleTextDocumentCodeLens(ctx context.Context, params types.CodeLensParams) (any, error) {
parseErrors, ok := l.cache.GetParseErrors(params.TextDocument.URI)
if ok && len(parseErrors) > 0 {
return noCodeLenses, nil
}

contents, module, ok := l.cache.GetContentAndModule(params.TextDocument.URI)
if !ok {
return nil, nil // return a null response, as per the spec
Expand Down Expand Up @@ -1700,11 +1699,7 @@ func (l *LanguageServer) handleTextDocumentCompletion(ctx context.Context, param
ClientIdentifier: l.clientIdentifier,
RootURI: l.workspaceRootURI,
Builtins: l.builtinsForCurrentCapabilities(),
RegoVersion: rules.RegoVersionFromVersionsMap(
l.loadedConfigAllRegoVersions.Clone(),
strings.TrimPrefix(params.TextDocument.URI, l.workspaceRootURI),
ast.RegoUndefined,
),
RegoVersion: l.regoVersionForURI(params.TextDocument.URI),
})
if err != nil {
return nil, fmt.Errorf("failed to find completions: %w", err)
Expand Down Expand Up @@ -2004,11 +1999,7 @@ func (l *LanguageServer) handleTextDocumentFormatting(
switch formatter {
case "opa-fmt", "opa-fmt-rego-v1":
opts := format.Opts{
RegoVersion: rules.RegoVersionFromVersionsMap(
l.loadedConfigAllRegoVersions.Clone(),
strings.TrimPrefix(params.TextDocument.URI, l.workspaceRootURI),
ast.RegoUndefined,
),
RegoVersion: l.regoVersionForURI(params.TextDocument.URI),
}

if formatter == "opa-fmt-rego-v1" {
Expand Down Expand Up @@ -2403,7 +2394,7 @@ func (l *LanguageServer) loadWorkspaceContents(ctx context.Context, newOnly bool

bis := l.builtinsForCurrentCapabilities()

if _, err = updateParse(ctx, l.cache, l.regoStore, fileURI, bis); err != nil {
if _, err = updateParse(ctx, l.cache, l.regoStore, fileURI, bis, l.regoVersionForURI(fileURI)); err != nil {
return fmt.Errorf("failed to update parse: %w", err)
}

Expand Down Expand Up @@ -2538,6 +2529,19 @@ func (l *LanguageServer) workspacePath() string {
return uri.ToPath(l.clientIdentifier, l.workspaceRootURI)
}

func (l *LanguageServer) regoVersionForURI(fileURI string) ast.RegoVersion {
version := ast.RegoUndefined
if l.loadedConfigAllRegoVersions != nil {
version = rules.RegoVersionFromVersionsMap(
l.loadedConfigAllRegoVersions.Clone(),
strings.TrimPrefix(uri.ToPath(l.clientIdentifier, fileURI), uri.ToPath(l.clientIdentifier, l.workspaceRootURI)),
ast.RegoUndefined,
)
}

return version
}

// builtinsForCurrentCapabilities returns the map of builtins for use
// in the server based on the currently loaded capabilities. If there is no
// config, then the default for the Regal OPA version is used.
Expand Down

0 comments on commit d9749f4

Please sign in to comment.