Merge pull request #1881 from josephschorr/lsp-warnings

Add warnings to the LSP

internal/lsp/handlers.go

@@ -57,14 +57,15 @@ func (s *Server) computeDiagnostics(ctx context.Context, uri lsp.DocumentURI) ([
 			return &jsonrpc2.Error{Code: jsonrpc2.CodeInternalError, Message: "file not found"}
 		}
 
-		_, devErrs, err := development.NewDevContext(ctx, &developerv1.RequestContext{
+		devCtx, devErrs, err := development.NewDevContext(ctx, &developerv1.RequestContext{
 			Schema:        file.contents,
 			Relationships: nil,
 		})
 		if err != nil {
 			return err
 		}
 
+		// Get errors.
 		for _, devErr := range devErrs.GetInputErrors() {
 			diagnostics = append(diagnostics, lsp.Diagnostic{
 				Severity: lsp.Error,
@@ -76,11 +77,31 @@ func (s *Server) computeDiagnostics(ctx context.Context, uri lsp.DocumentURI) ([
 			})
 		}
 
+		// If there are no errors, we can also check for warnings.
+		if len(diagnostics) == 0 {
+			warnings, err := development.GetWarnings(ctx, devCtx)
+			if err != nil {
+				return err
+			}
+
+			for _, devWarning := range warnings {
+				diagnostics = append(diagnostics, lsp.Diagnostic{
+					Severity: lsp.Warning,
+					Range: lsp.Range{
+						Start: lsp.Position{Line: int(devWarning.Line) - 1, Character: int(devWarning.Column) - 1},
+						End:   lsp.Position{Line: int(devWarning.Line) - 1, Character: int(devWarning.Column) - 1},
+					},
+					Message: devWarning.Message,
+				})
+			}
+		}
+
 		return nil
 	}); err != nil {
 		return nil, err
 	}
 
+	log.Info().Int("diagnostics", len(diagnostics)).Str("uri", string(uri)).Msg("computed diagnostics")
 	return diagnostics, nil
 }
 

internal/lsp/lsp_test.go

@@ -51,7 +51,7 @@ func TestDocumentNoDiagnostics(t *testing.T) {
 	require.Len(t, resp.Items, 0)
 }
 
-func TestDocumentDiagnostics(t *testing.T) {
+func TestDocumentErrorDiagnostics(t *testing.T) {
 	tester := newLSPTester(t)
 	tester.initialize()
 
@@ -80,6 +80,33 @@ func TestDocumentDiagnostics(t *testing.T) {
 	require.Len(t, resp.Items, 0)
 }
 
+func TestDocumentWarningDiagnostics(t *testing.T) {
+	tester := newLSPTester(t)
+	tester.initialize()
+
+	tester.setFileContents("file:///test", `
+		definition user {}
+
+		definition resource {
+			relation viewer: user
+			permission view_resource = viewer
+		}
+	`)
+
+	resp, _ := sendAndReceive[FullDocumentDiagnosticReport](tester, "textDocument/diagnostic",
+		TextDocumentDiagnosticParams{
+			TextDocument: TextDocument{URI: "file:///test"},
+		})
+	require.Equal(t, "full", resp.Kind)
+	require.Len(t, resp.Items, 1)
+	require.Equal(t, lsp.DiagnosticSeverity(lsp.Warning), resp.Items[0].Severity)
+	require.Equal(t, `Permission "view_resource" references parent type "resource" in its name; it is recommended to drop the suffix`, resp.Items[0].Message)
+	require.Equal(t, lsp.Range{
+		Start: lsp.Position{Line: 5, Character: 3},
+		End:   lsp.Position{Line: 5, Character: 3},
+	}, resp.Items[0].Range)
+}
+
 func TestDocumentDiagnosticsForTypeError(t *testing.T) {
 	tester := newLSPTester(t)
 	tester.initialize()

pkg/development/warningdefs.go

@@ -37,14 +37,15 @@ var lintRelationReferencesParentType = func(
 var lintPermissionReferencingItself = func(
 	ctx context.Context,
 	computedUserset *corev1.ComputedUserset,
+	sourcePosition *corev1.SourcePosition,
 	ts *typesystem.TypeSystem,
 ) (*devinterface.DeveloperWarning, error) {
 	parentRelation := ctx.Value(relationKey).(*corev1.Relation)
 	permName := parentRelation.Name
 	if computedUserset.Relation == permName {
-		return warningForMetadata(
+		return warningForPosition(
 			fmt.Sprintf("Permission %q references itself, which will cause an error to be raised due to infinite recursion", permName),
-			parentRelation,
+			sourcePosition,
 		), nil
 	}
 
@@ -54,6 +55,7 @@ var lintPermissionReferencingItself = func(
 var lintArrowReferencingUnreachable = func(
 	ctx context.Context,
 	ttu *corev1.TupleToUserset,
+	sourcePosition *corev1.SourcePosition,
 	ts *typesystem.TypeSystem,
 ) (*devinterface.DeveloperWarning, error) {
 	parentRelation := ctx.Value(relationKey).(*corev1.Relation)
@@ -82,7 +84,7 @@ var lintArrowReferencingUnreachable = func(
 	}
 
 	if !wasFound {
-		return warningForMetadata(
+		return warningForPosition(
 			fmt.Sprintf(
 				"Arrow `%s->%s` under permission %q references relation/permission %q that does not exist on any subject types of relation %q",
 				ttu.Tupleset.Relation,
@@ -91,7 +93,7 @@ var lintArrowReferencingUnreachable = func(
 				ttu.ComputedUserset.Relation,
 				ttu.Tupleset.Relation,
 			),
-			parentRelation,
+			sourcePosition,
 		), nil
 	}
 
@@ -101,6 +103,7 @@ var lintArrowReferencingUnreachable = func(
 var lintArrowOverSubRelation = func(
 	ctx context.Context,
 	ttu *corev1.TupleToUserset,
+	sourcePosition *corev1.SourcePosition,
 	ts *typesystem.TypeSystem,
 ) (*devinterface.DeveloperWarning, error) {
 	parentRelation := ctx.Value(relationKey).(*corev1.Relation)
@@ -117,7 +120,7 @@ var lintArrowOverSubRelation = func(
 
 	for _, subjectType := range allowedSubjectTypes {
 		if subjectType.Relation != tuple.Ellipsis {
-			return warningForMetadata(
+			return warningForPosition(
 				fmt.Sprintf(
 					"Arrow `%s->%s` under permission %q references relation %q that has relation %q on subject %q: *the subject relation will be ignored for the arrow*",
 					ttu.Tupleset.Relation,
@@ -127,7 +130,7 @@ var lintArrowOverSubRelation = func(
 					subjectType.Relation,
 					subjectType.Namespace,
 				),
-				parentRelation,
+				sourcePosition,
 			), nil
 		}
 	}
@@ -138,6 +141,7 @@ var lintArrowOverSubRelation = func(
 var lintArrowReferencingRelation = func(
 	ctx context.Context,
 	ttu *corev1.TupleToUserset,
+	sourcePosition *corev1.SourcePosition,
 	ts *typesystem.TypeSystem,
 ) (*devinterface.DeveloperWarning, error) {
 	parentRelation := ctx.Value(relationKey).(*corev1.Relation)
@@ -166,7 +170,7 @@ var lintArrowReferencingRelation = func(
 		}
 
 		if !nts.IsPermission(targetRelation.Name) {
-			return warningForMetadata(
+			return warningForPosition(
 				fmt.Sprintf(
 					"Arrow `%s->%s` under permission %q references relation %q on definition %q; it is recommended to point to a permission",
 					ttu.Tupleset.Relation,
@@ -175,7 +179,7 @@ var lintArrowReferencingRelation = func(
 					targetRelation.Name,
 					subjectType.Namespace,
 				),
-				parentRelation,
+				sourcePosition,
 			), nil
 		}
 	}

pkg/development/warnings.go

@@ -25,14 +25,18 @@ var allChecks = checkers{
 }
 
 func warningForMetadata(message string, metadata namespace.WithSourcePosition) *devinterface.DeveloperWarning {
-	if metadata.GetSourcePosition() == nil {
+	return warningForPosition(message, metadata.GetSourcePosition())
+}
+
+func warningForPosition(message string, sourcePosition *corev1.SourcePosition) *devinterface.DeveloperWarning {
+	if sourcePosition == nil {
 		return &devinterface.DeveloperWarning{
 			Message: message,
 		}
 	}
 
-	lineNumber := metadata.GetSourcePosition().ZeroIndexedLineNumber + 1
-	columnNumber := metadata.GetSourcePosition().ZeroIndexedColumnPosition + 1
+	lineNumber := sourcePosition.ZeroIndexedLineNumber + 1
+	columnNumber := sourcePosition.ZeroIndexedColumnPosition + 1
 
 	return &devinterface.DeveloperWarning{
 		Message: message,
@@ -96,8 +100,8 @@ func addDefinitionWarnings(ctx context.Context, def *corev1.NamespaceDefinition,
 
 type (
 	relationChecker        func(ctx context.Context, relation *corev1.Relation, vts *typesystem.TypeSystem) (*devinterface.DeveloperWarning, error)
-	computedUsersetChecker func(ctx context.Context, computedUserset *corev1.ComputedUserset, vts *typesystem.TypeSystem) (*devinterface.DeveloperWarning, error)
-	ttuChecker             func(ctx context.Context, ttu *corev1.TupleToUserset, vts *typesystem.TypeSystem) (*devinterface.DeveloperWarning, error)
+	computedUsersetChecker func(ctx context.Context, computedUserset *corev1.ComputedUserset, sourcePosition *corev1.SourcePosition, vts *typesystem.TypeSystem) (*devinterface.DeveloperWarning, error)
+	ttuChecker             func(ctx context.Context, ttu *corev1.TupleToUserset, sourcePosition *corev1.SourcePosition, vts *typesystem.TypeSystem) (*devinterface.DeveloperWarning, error)
 )
 
 type checkers struct {
@@ -135,7 +139,7 @@ func walkUsersetOperations(ctx context.Context, ops []*corev1.SetOperation_Child
 
 		case *corev1.SetOperation_Child_ComputedUserset:
 			for _, checker := range checkers.computedUsersetCheckers {
-				checkerWarning, err := checker(ctx, t.ComputedUserset, ts)
+				checkerWarning, err := checker(ctx, t.ComputedUserset, op.SourcePosition, ts)
 				if err != nil {
 					return nil, err
 				}
@@ -155,7 +159,7 @@ func walkUsersetOperations(ctx context.Context, ops []*corev1.SetOperation_Child
 
 		case *corev1.SetOperation_Child_TupleToUserset:
 			for _, checker := range checkers.ttuCheckers {
-				checkerWarning, err := checker(ctx, t.TupleToUserset, ts)
+				checkerWarning, err := checker(ctx, t.TupleToUserset, op.SourcePosition, ts)
 				if err != nil {
 					return nil, err
 				}

pkg/development/warnings_test.go

@@ -38,7 +38,7 @@ func TestWarnings(t *testing.T) {
 			expectedWarning: &developerv1.DeveloperWarning{
 				Message: "Permission \"view\" references itself, which will cause an error to be raised due to infinite recursion",
 				Line:    2,
-				Column:  5,
+				Column:  23,
 			},
 		},
 		{
@@ -51,7 +51,7 @@ func TestWarnings(t *testing.T) {
 			expectedWarning: &developerv1.DeveloperWarning{
 				Message: "Permission \"view\" references itself, which will cause an error to be raised due to infinite recursion",
 				Line:    4,
-				Column:  5,
+				Column:  42,
 			},
 		},
 		{
@@ -70,7 +70,7 @@ func TestWarnings(t *testing.T) {
 			expectedWarning: &developerv1.DeveloperWarning{
 				Message: "Arrow `group->member` under permission \"view\" references relation \"member\" on definition \"group\"; it is recommended to point to a permission",
 				Line:    9,
-				Column:  5,
+				Column:  23,
 			},
 		},
 		{
@@ -88,7 +88,7 @@ func TestWarnings(t *testing.T) {
 			expectedWarning: &developerv1.DeveloperWarning{
 				Message: "Arrow `group->member` under permission \"view\" references relation/permission \"member\" that does not exist on any subject types of relation \"group\"",
 				Line:    8,
-				Column:  5,
+				Column:  23,
 			},
 		},
 		{
@@ -108,7 +108,7 @@ func TestWarnings(t *testing.T) {
 			expectedWarning: &developerv1.DeveloperWarning{
 				Message: "Arrow `parent_group->member` under permission \"view\" references relation \"parent_group\" that has relation \"member\" on subject \"group\": *the subject relation will be ignored for the arrow*",
 				Line:    10,
-				Column:  5,
+				Column:  23,
 			},
 		},
 		{