Skip to content

Conversation

@ivanauth
Copy link
Contributor

When writing permission expressions like viewer - blocked & editor, it's easy to assume arithmetic-style precedence. But SpiceDB's actual precedence (exclusion binds loosest, then intersection, then union) can lead to unexpected behavior.

This adds a lint warning that flags mixed operators at the same scope, nudging users to add parentheses and make their intent explicit.

Example:

permission view = viewer - blocked & editor  // warns
permission view = (viewer - blocked) & editor  // ok
permission view = viewer + editor + blocked  // ok (same operator)

The warning can be suppressed with:

// spicedb-ignore-warning: mixed-operators-without-parentheses
permission view = viewer - blocked & editor

Fixes authzed/zed#598

@ivanauth ivanauth requested a review from a team as a code owner December 19, 2025 22:39
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Dec 19, 2025
@ivanauth ivanauth force-pushed the fix/issue-598-mixed-operators-warning branch from aa2be07 to 6a03a3d Compare December 19, 2025 22:40
@ivanauth ivanauth changed the title Warn when permission expressions mix operators without parentheses feat: add warning for mixed operators without parentheses Dec 19, 2025
@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 87.12446% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.52%. Comparing base (7a18b39) to head (ebb6aa6).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
pkg/composableschemadsl/compiler/translator.go 77.78% 10 Missing and 4 partials ⚠️
pkg/schemadsl/compiler/translator.go 87.31% 5 Missing and 3 partials ⚠️
pkg/namespace/metadata.go 89.48% 4 Missing and 2 partials ⚠️
pkg/namespace/builder.go 75.00% 1 Missing and 1 partial ⚠️

❌ Your project check has failed because the head coverage (74.52%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2786      +/-   ##
==========================================
+ Coverage   74.40%   74.52%   +0.12%     
==========================================
  Files         484      484              
  Lines       57406    57852     +446     
==========================================
+ Hits        42707    43106     +399     
- Misses      11709    11735      +26     
- Partials     2990     3011      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@josephschorr
Copy link
Member

@ivanauth this won't actually work in all cases, because expressions can cross multiple lines. Instead, this should likely be done by marking the expression during compilation with a piece of metadata, and having the warnings generator read that metadata

@github-actions github-actions bot added area/schema Affects the Schema Language area/dependencies Affects dependencies labels Jan 7, 2026
@ivanauth
Copy link
Contributor Author

ivanauth commented Jan 7, 2026

Switched from source-scanning to compile-time metadata detection per review feedback. Added NodeTypeParenthesizedExpression wrapper in parser to track explicit grouping, then detect mixed operators during AST translation. Also fixed edge case where top-level parentheses like (a + b - c) would skip the warning.

@ivanauth ivanauth force-pushed the fix/issue-598-mixed-operators-warning branch from 88ff553 to 6952dbd Compare January 8, 2026 21:59
Satisfies panic check linter which allows panics in Must* functions.
Also regenerates proto files after import order fix.
Add TestMixedOperatorsMetadata to pkg/namespace/metadata_test.go covering
all code paths in HasMixedOperatorsWithoutParens, GetMixedOperatorsPosition,
and SetMixedOperatorsWithoutParens (nil metadata, empty metadata, non-permission
relation, permission with/without flag, set/clear operations, creation of new
metadata entries).

Add additional warning test cases to pkg/development/warnings_test.go for
mixed intersection+exclusion, intersection+union operator combinations,
same-operators-repeated (no warning), and relation-referencing-parent for
a non-permission relation.
@ivanauth ivanauth force-pushed the fix/issue-598-mixed-operators-warning branch from e292e69 to 5c2b616 Compare January 23, 2026 21:48
ivanauth and others added 2 commits January 29, 2026 11:19
Update the expected AST output for the self_test to reflect the new
NodeTypeParenthesizedExpression wrapper that is now emitted when
parsing parenthesized expressions.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/dependencies Affects dependencies area/schema Affects the Schema Language area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add warning to use parenthesis for precedence

2 participants