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 29.36508% with 89 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.05%. Comparing base (21d76e0) to head (6d64184).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
pkg/namespace/metadata.go 0.00% 47 Missing ⚠️
pkg/schemadsl/compiler/translator.go 63.83% 11 Missing and 6 partials ⚠️
pkg/development/warningdefs.go 0.00% 15 Missing ⚠️
pkg/namespace/builder.go 0.00% 7 Missing ⚠️
pkg/development/warnings.go 0.00% 3 Missing ⚠️

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

❗ There is a different number of reports uploaded between BASE (21d76e0) and HEAD (6d64184). Click for more details.

HEAD has 26 uploads less than BASE
Flag BASE (21d76e0) HEAD (6d64184)
49 23
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2786       +/-   ##
===========================================
- Coverage   77.70%   59.05%   -18.65%     
===========================================
  Files         472      425       -47     
  Lines       49707    44857     -4850     
===========================================
- Hits        38620    26485    -12135     
- Misses       8234    15809     +7575     
+ Partials     2853     2563      -290     

☔ 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 and others added 3 commits January 7, 2026 16:00
Satisfies panic check linter which allows panics in Must* functions.
Also regenerates proto files after import order fix.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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