Add warning for mixed operators without parentheses

Author: ivanauth

pkg/development/devcontext.go

@@ -48,6 +48,9 @@ type DevContext struct {
 	Revision       datastore.Revision
 	CompiledSchema *compiler.CompiledSchema
 	Dispatcher     dispatch.Dispatcher
+
+	// SchemaString is the original schema source text, used for source-scanning warnings.
+	SchemaString string
 }
 
 // NewDevContext creates a new DevContext from the specified request context, parsing and populating
@@ -154,6 +157,7 @@ func newDevContextWithDatastore(ctx context.Context, requestContext *devinterfac
 		CompiledSchema: compiled,
 		Revision:       currentRevision,
 		Dispatcher:     graph.MustNewLocalOnlyDispatcher(params),
+		SchemaString:   requestContext.Schema,
 	}, nil, nil
 }
 

pkg/development/warningdefs.go

@@ -230,3 +230,120 @@ var lintArrowReferencingRelation = ttuCheck{
 		return nil, nil
 	},
 }
+
+// CheckExpressionForMixedOperators checks a permission expression for mixed operators
+// at the same parenthetical scope. This is a source-scanning check that detects
+// cases where users mix +, -, and & operators without explicit parentheses.
+//
+// For example:
+//   - "foo + bar" - OK (single operator type)
+//   - "foo - bar & baz" - WARN (mixed - and & at same scope)
+//   - "(foo - bar) & baz" - OK (operators in different scopes)
+//   - "(foo + bar) - (baz & qux)" - OK (operators in different scopes)
+//   - "(foo + bar - baz)" - WARN (mixed inside parentheses)
+func CheckExpressionForMixedOperators(
+	permissionName string,
+	expressionText string,
+	sourcePosition *corev1.SourcePosition,
+) *devinterface.DeveloperWarning {
+	// Use a stack-based approach to track operators in each parenthetical scope.
+	// Each scope is a map of operators seen at that level.
+	type scope struct {
+		operators map[rune]bool
+	}
+
+	// Helper to check if a scope has mixed operators and return warning if so
+	checkScope := func(s scope) *devinterface.DeveloperWarning {
+		if len(s.operators) > 1 {
+			// Build a human-readable list of the mixed operators
+			var opList []string
+			if s.operators['+'] {
+				opList = append(opList, "union (+)")
+			}
+			if s.operators['-'] {
+				opList = append(opList, "exclusion (-)")
+			}
+			if s.operators['&'] {
+				opList = append(opList, "intersection (&)")
+			}
+
+			return warningForPosition(
+				"mixed-operators-without-parentheses",
+				fmt.Sprintf(
+					"Permission %q mixes %s at the same level of nesting; consider adding parentheses to clarify precedence",
+					permissionName,
+					strings.Join(opList, " and "),
+				),
+				permissionName,
+				sourcePosition,
+			)
+		}
+		return nil
+	}
+
+	// Stack of scopes - start with the root scope
+	scopeStack := []scope{{operators: make(map[rune]bool)}}
+
+	runes := []rune(expressionText)
+	for i := 0; i < len(runes); i++ {
+		r := runes[i]
+
+		// Skip whitespace
+		if isWhitespace(r) {
+			continue
+		}
+
+		// Enter a new scope on open parenthesis
+		if r == '(' {
+			scopeStack = append(scopeStack, scope{operators: make(map[rune]bool)})
+			continue
+		}
+
+		// Exit scope on close parenthesis - check for mixed operators before popping
+		if r == ')' {
+			if len(scopeStack) > 1 {
+				// Check the scope we're about to pop for mixed operators
+				if warning := checkScope(scopeStack[len(scopeStack)-1]); warning != nil {
+					return warning
+				}
+				scopeStack = scopeStack[:len(scopeStack)-1]
+			}
+			continue
+		}
+
+		// Skip arrow operator (->)
+		if r == '-' && i+1 < len(runes) && runes[i+1] == '>' {
+			i++ // Skip the '>'
+			continue
+		}
+
+		// Skip dot notation for function calls (e.g., parent.all)
+		if r == '.' {
+			continue
+		}
+
+		// Check for operators: +, -, &
+		// Only count operators that are not part of an identifier
+		if r == '+' || r == '-' || r == '&' {
+			// Look back to see if the previous non-whitespace char was an identifier char
+			// If so, this might be part of a different construct, but in SpiceDB schema
+			// these operators should always be surrounded by whitespace or parens
+			currentScope := &scopeStack[len(scopeStack)-1]
+			currentScope.operators[r] = true
+			continue
+		}
+	}
+
+	// Check remaining scopes (including root) for mixed operators
+	for _, s := range scopeStack {
+		if warning := checkScope(s); warning != nil {
+			return warning
+		}
+	}
+
+	return nil
+}
+
+func isWhitespace(r rune) bool {
+	return r == ' ' || r == '\t' || r == '\n' || r == '\r'
+}

pkg/development/warnings.go

@@ -3,6 +3,7 @@ package development
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/ccoveille/go-safecast/v2"
 
@@ -64,8 +65,11 @@ func GetWarnings(ctx context.Context, devCtx *DevContext) ([]*devinterface.Devel
 	res := schema.ResolverForCompiledSchema(*devCtx.CompiledSchema)
 	ts := schema.NewTypeSystem(res)
 
+	// Pre-split schema string once for performance when checking multiple permissions
+	schemaLines := strings.Split(devCtx.SchemaString, "\n")
+
 	for _, def := range devCtx.CompiledSchema.ObjectDefinitions {
-		found, err := addDefinitionWarnings(ctx, def, ts)
+		found, err := addDefinitionWarnings(ctx, def, ts, schemaLines)
 		if err != nil {
 			return nil, err
 		}
@@ -79,7 +83,7 @@ type contextKey string
 
 var relationKey = contextKey("relation")
 
-func addDefinitionWarnings(ctx context.Context, nsDef *corev1.NamespaceDefinition, ts *schema.TypeSystem) ([]*devinterface.DeveloperWarning, error) {
+func addDefinitionWarnings(ctx context.Context, nsDef *corev1.NamespaceDefinition, ts *schema.TypeSystem, schemaLines []string) ([]*devinterface.DeveloperWarning, error) {
 	def, err := schema.NewDefinition(ts, nsDef)
 	if err != nil {
 		return nil, err
@@ -111,12 +115,125 @@ func addDefinitionWarnings(ctx context.Context, nsDef *corev1.NamespaceDefinitio
 			}
 
 			warnings = append(warnings, found...)
+
+			// Check for mixed operators without parentheses using source scanning
+			if !shouldSkipCheck(rel.Metadata, "mixed-operators-without-parentheses") {
+				expressionText := extractPermissionExpression(schemaLines, rel.Name, rel.GetSourcePosition())
+				if expressionText != "" {
+					if warning := CheckExpressionForMixedOperators(rel.Name, expressionText, rel.GetSourcePosition()); warning != nil {
+						warnings = append(warnings, warning)
+					}
+				}
+			}
 		}
 	}
 
 	return warnings, nil
 }
 
+// extractPermissionExpression extracts the expression text for a permission from the schema source.
+// It uses the source position to locate the permission and extracts the text after the '=' sign.
+// The schemaLines parameter should be pre-split for performance when checking multiple permissions.
+func extractPermissionExpression(schemaLines []string, _ string, sourcePosition *corev1.SourcePosition) string {
+	if sourcePosition == nil || len(schemaLines) == 0 {
+		return ""
+	}
+
+	lineIdx, err := safecast.Convert[int](sourcePosition.ZeroIndexedLineNumber)
+	if err != nil || lineIdx < 0 || lineIdx >= len(schemaLines) {
+		return ""
+	}
+
+	// Start from the permission's line and look for the expression
+	// The expression is everything after '=' until end of statement
+	var expressionBuilder strings.Builder
+	foundEquals := false
+	parenDepth := 0
+	inBlockComment := false
+	lastNonWhitespaceWasOperator := false
+
+	for i := lineIdx; i < len(schemaLines); i++ {
+		line := schemaLines[i]
+
+		// If this is the first line, start from the column position
+		startCol := 0
+		if i == lineIdx {
+			colPos, err := safecast.Convert[int](sourcePosition.ZeroIndexedColumnPosition)
+			if err == nil && colPos < len(line) {
+				startCol = colPos
+			}
+		}
+
+		lineHasContent := false
+		for j := startCol; j < len(line); j++ {
+			ch := line[j]
+
+			// Handle block comment state
+			if inBlockComment {
+				if ch == '*' && j+1 < len(line) && line[j+1] == '/' {
+					inBlockComment = false
+					j++ // Skip the '/'
+				}
+				continue
+			}
+
+			// Check for start of block comment
+			if ch == '/' && j+1 < len(line) && line[j+1] == '*' {
+				inBlockComment = true
+				j++ // Skip the '*'
+				continue
+			}
+
+			// Check for line comment - skip rest of line
+			if ch == '/' && j+1 < len(line) && line[j+1] == '/' {
+				break
+			}
+
+			if !foundEquals {
+				if ch == '=' {
+					foundEquals = true
+				}
+				continue
+			}
+
+			// Track parenthesis depth for multi-line expressions
+			switch ch {
+			case '(':
+				parenDepth++
+			case ')':
+				parenDepth--
+			}
+
+			// Track if this is an operator (for multi-line continuation detection)
+			isWhitespaceChar := ch == ' ' || ch == '\t'
+			if !isWhitespaceChar {
+				lineHasContent = true
+				lastNonWhitespaceWasOperator = (ch == '+' || ch == '-' || ch == '&')
+			}
+
+			expressionBuilder.WriteByte(ch)
+		}
+
+		// Determine if expression continues on next line:
+		// 1. We're inside parentheses (parenDepth > 0)
+		// 2. We're inside a block comment
+		// 3. The line ended with an operator (suggesting continuation)
+		// 4. The line had no content yet after '=' (e.g., '= \n foo + bar')
+		if foundEquals {
+			continueToNextLine := parenDepth > 0 || inBlockComment || lastNonWhitespaceWasOperator || !lineHasContent
+			if !continueToNextLine {
+				break
+			}
+			// Add space for multi-line expressions
+			if i < len(schemaLines)-1 {
+				expressionBuilder.WriteByte(' ')
+			}
+		}
+	}
+
+	return strings.TrimSpace(expressionBuilder.String())
+}
+
 func shouldSkipCheck(metadata *corev1.Metadata, name string) bool {
 	if metadata == nil {
 		return false