Skip to content

Commit 3d37e2d

Browse files
committed
feat: add warning for mixed operators without parentheses
1 parent d873dd4 commit 3d37e2d

30 files changed

+930
-233
lines changed

.gitignore

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,7 @@ coverage*.txt
99
.claude/
1010
e2e/newenemy/spicedb
1111
e2e/newenemy/cockroach
12-
e2e/newenemy/chaosd
12+
e2e/newenemy/chaosd
13+
14+
# Auto Claude data directory
15+
.auto-claude/

go.sum

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2492,6 +2492,7 @@ google.golang.org/grpc v1.54.0/go.mod h1:PUSEXI6iWghWaB6lXM4knEgpJNu2qUcKfDtNci3
24922492
google.golang.org/grpc v1.56.3/go.mod h1:I9bI3vqKfayGqPUAwGdOSu7kt6oIJLixfffKrpXqQ9s=
24932493
google.golang.org/grpc v1.77.0 h1:wVVY6/8cGA6vvffn+wWK5ToddbgdU3d8MNENr4evgXM=
24942494
google.golang.org/grpc v1.77.0/go.mod h1:z0BY1iVj0q8E1uSQCjL9cppRj+gnZjzDnzV0dHhrNig=
2495+
google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0 h1:M1YKkFIboKNieVO5DLUEVzQfGwJD30Nv2jfUgzb5UcE=
24952496
google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0/go.mod h1:6Kw0yEErY5E/yWrBtf03jp27GLLJujG4z/JK95pnjjw=
24962497
google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8=
24972498
google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0=

pkg/composableschemadsl/compiler/compiler_test.go

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -573,16 +573,19 @@ func TestCompile(t *testing.T) {
573573
"",
574574
[]SchemaDefinition{
575575
namespace.Namespace("sometenant/complex",
576-
namespace.MustRelation("foos",
577-
namespace.Exclusion(
578-
namespace.Rewrite(
579-
namespace.Union(
580-
namespace.ComputedUserset("bars"),
581-
namespace.ComputedUserset("bazs"),
576+
namespace.WithMixedOperators(
577+
namespace.MustRelation("foos",
578+
namespace.Exclusion(
579+
namespace.Rewrite(
580+
namespace.Union(
581+
namespace.ComputedUserset("bars"),
582+
namespace.ComputedUserset("bazs"),
583+
),
582584
),
585+
namespace.ComputedUserset("mehs"),
583586
),
584-
namespace.ComputedUserset("mehs"),
585587
),
588+
1, 22, // line 1, column 22 (0-indexed)
586589
),
587590
),
588591
},

pkg/composableschemadsl/compiler/translator.go

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,9 @@ func translatePermission(tctx *translationContext, permissionNode *dslNode) (*co
437437
return nil, permissionNode.Errorf("invalid permission expression: %w", err)
438438
}
439439

440+
// Detect mixed operators without parentheses before translation (which may flatten the AST).
441+
mixedOpsPosition := detectMixedOperatorsWithoutParens(tctx, expressionNode)
442+
440443
rewrite, err := translateExpression(tctx, expressionNode)
441444
if err != nil {
442445
return nil, err
@@ -447,6 +450,14 @@ func translatePermission(tctx *translationContext, permissionNode *dslNode) (*co
447450
return nil, err
448451
}
449452

453+
// Store mixed operators flag in metadata
454+
if mixedOpsPosition != nil {
455+
err = namespace.SetMixedOperatorsWithoutParens(permission, true, mixedOpsPosition)
456+
if err != nil {
457+
return nil, permissionNode.Errorf("error adding mixed operators flag to metadata: %w", err)
458+
}
459+
}
460+
450461
if !tctx.skipValidate {
451462
if err := permission.Validate(); err != nil {
452463
return nil, permissionNode.Errorf("error in permission %s: %w", permissionName, err)
@@ -577,6 +588,14 @@ func translateExpressionOperationDirect(tctx *translationContext, expressionOpNo
577588
case dslshape.NodeTypeNilExpression:
578589
return namespace.Nil(), nil
579590

591+
case dslshape.NodeTypeParenthesizedExpression:
592+
// Unwrap the parenthesized expression and translate its inner expression.
593+
innerExprNode, err := expressionOpNode.Lookup(dslshape.NodeParenthesizedExpressionPredicateInnerExpr)
594+
if err != nil {
595+
return nil, err
596+
}
597+
return translateExpressionOperation(tctx, innerExprNode)
598+
580599
case dslshape.NodeTypeArrowExpression:
581600
leftChild, err := expressionOpNode.Lookup(dslshape.NodeExpressionPredicateLeftExpr)
582601
if err != nil {
@@ -952,3 +971,91 @@ func translateUseFlag(tctx *translationContext, useFlagNode *dslNode) error {
952971
tctx.enabledFlags = append(tctx.enabledFlags, flagName)
953972
return nil
954973
}
974+
975+
// operatorType represents the type of set operator in an expression.
976+
type operatorType int
977+
978+
const (
979+
operatorTypeUnknown operatorType = iota
980+
operatorTypeUnion
981+
operatorTypeIntersection
982+
operatorTypeExclusion
983+
)
984+
985+
// getOperatorType returns the operator type for a given node type, or operatorTypeUnknown if not a set operator.
986+
func getOperatorType(nodeType dslshape.NodeType) operatorType {
987+
switch nodeType {
988+
case dslshape.NodeTypeUnionExpression:
989+
return operatorTypeUnion
990+
case dslshape.NodeTypeIntersectExpression:
991+
return operatorTypeIntersection
992+
case dslshape.NodeTypeExclusionExpression:
993+
return operatorTypeExclusion
994+
default:
995+
return operatorTypeUnknown
996+
}
997+
}
998+
999+
// detectMixedOperatorsWithoutParens walks the expression AST and detects if there are mixed
1000+
// operators (union, intersection, exclusion) at the same scope level without explicit parentheses.
1001+
// Returns the source position of the first mixed operator found, or nil if none.
1002+
// Parenthesized expressions act as boundaries - mixing inside parens does not trigger a warning
1003+
// since the user explicitly grouped the expression. However, top-level parentheses are unwrapped
1004+
// since they don't clarify internal operator precedence.
1005+
func detectMixedOperatorsWithoutParens(tctx *translationContext, node *dslNode) *core.SourcePosition {
1006+
// Unwrap top-level parenthesized expressions - they don't clarify internal precedence.
1007+
// e.g., (a + b - c) should still warn about mixed operators.
1008+
for node.GetType() == dslshape.NodeTypeParenthesizedExpression {
1009+
innerNode, err := node.Lookup(dslshape.NodeParenthesizedExpressionPredicateInnerExpr)
1010+
if err != nil {
1011+
break
1012+
}
1013+
node = innerNode
1014+
}
1015+
return detectMixedOperatorsInScope(tctx, node, operatorTypeUnknown)
1016+
}
1017+
1018+
// detectMixedOperatorsInScope recursively checks for mixed operators within a scope.
1019+
// parentOp is the operator type seen so far at this scope level.
1020+
func detectMixedOperatorsInScope(tctx *translationContext, node *dslNode, parentOp operatorType) *core.SourcePosition {
1021+
nodeType := node.GetType()
1022+
1023+
// Parenthesized expressions act as a boundary - don't propagate operator checking into them.
1024+
// The user explicitly grouped the expression, so we don't warn about mixing inside.
1025+
if nodeType == dslshape.NodeTypeParenthesizedExpression {
1026+
return nil
1027+
}
1028+
1029+
currentOp := getOperatorType(nodeType)
1030+
1031+
// If this is a set operator and we've seen a different operator at this scope, it's mixed.
1032+
if currentOp != operatorTypeUnknown && parentOp != operatorTypeUnknown && currentOp != parentOp {
1033+
return getSourcePosition(node, tctx.mapper)
1034+
}
1035+
1036+
// If this is a set operator, check children with this operator as the scope's operator.
1037+
if currentOp != operatorTypeUnknown {
1038+
effectiveOp := currentOp
1039+
if parentOp != operatorTypeUnknown {
1040+
effectiveOp = parentOp // Keep the first operator seen at this scope
1041+
}
1042+
1043+
// Check left child
1044+
leftChild, err := node.Lookup(dslshape.NodeExpressionPredicateLeftExpr)
1045+
if err == nil {
1046+
if pos := detectMixedOperatorsInScope(tctx, leftChild, effectiveOp); pos != nil {
1047+
return pos
1048+
}
1049+
}
1050+
1051+
// Check right child
1052+
rightChild, err := node.Lookup(dslshape.NodeExpressionPredicateRightExpr)
1053+
if err == nil {
1054+
if pos := detectMixedOperatorsInScope(tctx, rightChild, effectiveOp); pos != nil {
1055+
return pos
1056+
}
1057+
}
1058+
}
1059+
1060+
return nil
1061+
}

pkg/composableschemadsl/dslshape/dslshape.go

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,9 @@ const (
3232

3333
NodeTypeArrowExpression // A TTU in arrow form.
3434

35-
NodeTypeIdentifier // An identifier under an expression.
36-
NodeTypeNilExpression // A nil keyword
35+
NodeTypeIdentifier // An identifier under an expression.
36+
NodeTypeNilExpression // A nil keyword
37+
NodeTypeParenthesizedExpression // A parenthesized expression wrapper.
3738

3839
NodeTypeCaveatTypeReference // A type reference for a caveat parameter.
3940

@@ -211,6 +212,13 @@ const (
211212
NodeExpressionPredicateLeftExpr = "left-expr"
212213
NodeExpressionPredicateRightExpr = "right-expr"
213214

215+
//
216+
// NodeTypeParenthesizedExpression
217+
//
218+
219+
// The inner expression wrapped by parentheses.
220+
NodeParenthesizedExpressionPredicateInnerExpr = "inner-expr"
221+
214222
//
215223
// NodeTypeImport
216224
//

pkg/composableschemadsl/dslshape/zz_generated.nodetype_string.go

Lines changed: 7 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/composableschemadsl/parser/parser.go

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -652,18 +652,23 @@ func (p *sourceParser) tryConsumeArrowExpression() (AstNode, bool) {
652652
// ```nil```
653653
func (p *sourceParser) tryConsumeBaseExpression() (AstNode, bool) {
654654
switch {
655-
// Nested expression.
655+
// Nested expression - wrap in NodeTypeParenthesizedExpression to preserve parentheses info.
656656
case p.isToken(lexer.TokenTypeLeftParen):
657657
comments := p.currentToken.comments
658658

659+
wrapperNode := p.startNode(dslshape.NodeTypeParenthesizedExpression)
660+
659661
p.consume(lexer.TokenTypeLeftParen)
660-
exprNode := p.consumeComputeExpression()
662+
innerExprNode := p.consumeComputeExpression()
661663
p.consume(lexer.TokenTypeRightParen)
662664

663-
// Attach any comments found to the consumed expression.
664-
p.decorateComments(exprNode, comments)
665+
wrapperNode.Connect(dslshape.NodeParenthesizedExpressionPredicateInnerExpr, innerExprNode)
666+
p.mustFinishNode()
665667

666-
return exprNode, true
668+
// Attach any comments found to the wrapper node.
669+
p.decorateComments(wrapperNode, comments)
670+
671+
return wrapperNode, true
667672

668673
// Nil expression.
669674
case p.isKeyword("nil"):

pkg/composableschemadsl/parser/tests/basic.zed.expected

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -84,22 +84,27 @@ NodeTypeFile
8484
input-source = basic definition test
8585
start-rune = 208
8686
left-expr =>
87-
NodeTypeExclusionExpression
88-
end-rune = 217
87+
NodeTypeParenthesizedExpression
88+
end-rune = 218
8989
input-source = basic definition test
90-
start-rune = 209
91-
left-expr =>
92-
NodeTypeIdentifier
93-
end-rune = 211
94-
identifier-value = foo
95-
input-source = basic definition test
96-
start-rune = 209
97-
right-expr =>
98-
NodeTypeIdentifier
90+
start-rune = 208
91+
inner-expr =>
92+
NodeTypeExclusionExpression
9993
end-rune = 217
100-
identifier-value = meh
10194
input-source = basic definition test
102-
start-rune = 215
95+
start-rune = 209
96+
left-expr =>
97+
NodeTypeIdentifier
98+
end-rune = 211
99+
identifier-value = foo
100+
input-source = basic definition test
101+
start-rune = 209
102+
right-expr =>
103+
NodeTypeIdentifier
104+
end-rune = 217
105+
identifier-value = meh
106+
input-source = basic definition test
107+
start-rune = 215
103108
right-expr =>
104109
NodeTypeIdentifier
105110
end-rune = 224

0 commit comments

Comments
 (0)