fix: make sure deleted items aren't added back to the cache (#685)

Fixes #679

unrelated: disabled the gocognit linter as it's annoying

Signed-off-by: Anders Eknert <anders@styra.com>

.golangci.yaml

@@ -18,6 +18,7 @@ linters:
     - rowserrcheck
     - wastedassign
     # annoying
+    - gocognit
     - goerr113
     - varnamelen
     - nonamedreturns

cmd/lint.go

@@ -183,7 +183,6 @@ func init() {
 	RootCommand.AddCommand(lintCommand)
 }
 
-//nolint:gocognit
 func lint(args []string, params *lintCommandParams) (report.Report, error) {
 	var err error
 

internal/lsp/documentsymbol_test.go

@@ -83,7 +83,7 @@ func TestRefToString(t *testing.T) {
 	}
 }
 
-//nolint:gocognit,nestif
+//nolint:nestif
 func TestDocumentSymbols(t *testing.T) {
 	t.Parallel()
 

internal/lsp/server.go

@@ -84,6 +84,7 @@ type LanguageServer struct {
 type fileUpdateEvent struct {
 	Reason  string
 	URI     string
+	OldURI  string
 	Content string
 }
 
@@ -173,6 +174,25 @@ func (l *LanguageServer) StartDiagnosticsWorker(ctx context.Context) {
 		case <-ctx.Done():
 			return
 		case evt := <-l.diagnosticRequestFile:
+			// if file has been deleted, clear diagnostics in the client
+			if evt.Reason == "textDocument/didDelete" {
+				err := l.sendFileDiagnostics(ctx, evt.URI)
+				if err != nil {
+					l.logError(fmt.Errorf("failed to send diagnostic: %w", err))
+				}
+
+				continue
+			}
+
+			if evt.Reason == "textDocument/didRename" && evt.OldURI != "" {
+				err := l.sendFileDiagnostics(ctx, evt.OldURI)
+				if err != nil {
+					l.logError(fmt.Errorf("failed to send diagnostic: %w", err))
+				}
+
+				continue
+			}
+
 			// if there is new content, we need to update the parse errors or module first
 			success, err := l.processTextContentUpdate(ctx, evt.URI, evt.Content)
 			if err != nil {
@@ -226,7 +246,7 @@ func (l *LanguageServer) StartHoverWorker(ctx context.Context) {
 		case <-ctx.Done():
 			return
 		case evt := <-l.builtinsPositionFile:
-			_, err := l.processBuiltinsUpdate(ctx, evt.URI, evt.Content)
+			err := l.processBuiltinsUpdate(ctx, evt.URI, evt.Content)
 			if err != nil {
 				l.logError(fmt.Errorf("failed to process builtin positions update: %w", err))
 			}
@@ -429,25 +449,26 @@ func (l *LanguageServer) processTextContentUpdate(
 	return false, nil
 }
 
-func (l *LanguageServer) processBuiltinsUpdate(
-	_ context.Context,
-	uri string,
-	content string,
-) (bool, error) {
+func (l *LanguageServer) processBuiltinsUpdate(_ context.Context, uri string, content string) error {
+	if _, ok := l.cache.GetFileContents(uri); !ok {
+		// If the file is not in the cache, exit early or else
+		// we might accidentally put it in the cache after it's been
+		// deleted: https://github.com/StyraInc/regal/issues/679
+		return nil
+	}
+
 	l.cache.SetFileContents(uri, content)
 
 	success, err := updateParse(l.cache, uri)
 	if err != nil {
-		return false, fmt.Errorf("failed to update parse: %w", err)
+		return fmt.Errorf("failed to update parse: %w", err)
 	}
 
 	if !success {
-		return false, nil
+		return nil
 	}
 
-	err = updateBuiltinPositions(l.cache, uri)
-
-	return err == nil, err
+	return updateBuiltinPositions(l.cache, uri)
 }
 
 func (l *LanguageServer) logError(err error) {
@@ -873,7 +894,6 @@ func (l *LanguageServer) handleWorkspaceDidDeleteFiles(
 		}
 
 		l.diagnosticRequestFile <- evt
-		l.builtinsPositionFile <- evt
 	}
 
 	return struct{}{}, nil
@@ -904,6 +924,7 @@ func (l *LanguageServer) handleWorkspaceDidRenameFiles(
 		evt := fileUpdateEvent{
 			Reason:  "textDocument/didRename",
 			URI:     renameOp.NewURI,
+			OldURI:  renameOp.OldURI,
 			Content: content,
 		}
 

internal/lsp/server_test.go

@@ -48,7 +48,7 @@ const fileURIScheme = "file://"
 // TestLanguageServerSingleFile tests that changes to a single file and Regal config are handled correctly by the
 // language server my making updates to both and validating that the correct diagnostics are sent to the client.
 //
-//nolint:gocognit,gocyclo,maintidx
+//nolint:gocyclo,maintidx
 func TestLanguageServerSingleFile(t *testing.T) {
 	t.Parallel()
 
@@ -268,7 +268,7 @@ rules:
 // workspace diagnostics are run, this test validates that the correct diagnostics are sent to the client in this
 // scenario.
 //
-//nolint:gocognit,maintidx,gocyclo
+// nolint:maintidx,gocyclo
 func TestLanguageServerMultipleFiles(t *testing.T) {
 	t.Parallel()
 
@@ -509,6 +509,33 @@ allow if input.user in admins.users
 	}
 }
 
+// https://github.com/StyraInc/regal/issues/679
+func TestProcessBuiltinUpdateExitsOnMissingFile(t *testing.T) {
+	t.Parallel()
+
+	ls := LanguageServer{
+		cache: NewCache(),
+	}
+
+	err := ls.processBuiltinsUpdate(context.Background(), "file://missing.rego", "foo")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(ls.cache.builtinPositionsFile) != 0 {
+		t.Errorf("expected builtin positions to be empty, got %v", ls.cache.builtinPositionsFile)
+	}
+
+	contents, ok := ls.cache.GetFileContents("file://missing.rego")
+	if ok {
+		t.Errorf("expected file contents to be empty, got %s", contents)
+	}
+
+	if len(ls.cache.GetAllFiles()) != 0 {
+		t.Errorf("expected files to be empty, got %v", ls.cache.GetAllFiles())
+	}
+}
+
 // TestLanguageServerParentDirConfig tests that regal config is loaded as it is for the
 // Regal CLI, and that config files in a parent directory are loaded correctly
 // even when the workspace is a child directory.

pkg/linter/linter.go

@@ -655,7 +655,6 @@ func loadModulesFromCustomRuleFS(customRuleFS fs.FS, rootPath string) (map[strin
 	return files, nil
 }
 
-//nolint:gocognit
 func (l Linter) lintWithRegoRules(ctx context.Context, input rules.Input) (report.Report, error) {
 	l.startTimer(regalmetrics.RegalLintRego)
 	defer l.stopTimer(regalmetrics.RegalLintRego)