oracle: Refactor oracle to be more extensible #8105

charlieegan3 · 2025-12-02T16:31:32Z

Oracle is going to get some updates with the upcoming string interpolation work and I'd like to make it easier to work on with tests etc.

I have restructured this into a 'Manager', with a number of 'Locators' for different ast constructs. I have tried to add some basic tests for the locators but pausing here for feedback.

I hope to continue to improve this as I add support for string interp.

Ref #4733

netlify · 2025-12-02T16:34:02Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`f80ef7d`
🔍 Latest deploy log	https://app.netlify.com/projects/openpolicyagent/deploys/693187a388e4f50008622db8
😎 Deploy Preview	https://deploy-preview-8105--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

johanfylling

This approach makes sense to me 👍

johanfylling · 2025-12-03T13:52:49Z

v1/ast/oracle/find/locator.go

+// Locator wraps functionality for finding specific AST contructs
+type Locator interface {
+	// Applicable returns true if this handler can process the given stack
+	Applicable(stack []ast.Node) bool


Is there a need for this extra check if each Locator impl also goes through similar motions in Find()?

Yeah it's almost identical now you mention it. I have removed in c7ccd66.

The idea was that I'd clearly define when each was applicable, and test that, but I am not sure if adds enough value. We can test the Find functions in time and that'll have the same result.

johanfylling · 2025-12-03T13:57:42Z

v1/ast/oracle/oracle.go

-	return &Oracle{}
+	manager := find.NewManager()
+
+	manager.Register(find.NewRefLocator())


I don't think your refactoring actually changes this behaviour, but I find it strange that we lookup refs before vars. E.g. if a user queries the location of a var that happens to be inside a ref (except for the first ref term), then I think they'd expect to find the definition of that var, not the ref it happens to be inside.
Or am I misunderstanding this?

Yeah that makes sense to me too. I had just kept the original order. The tests still seem to pass with the var locator first so I'll update it to that.

johanfylling · 2025-12-03T14:11:41Z

v1/ast/oracle/find/variables.go

+}
+
+func (v *VarLocator) FindVarOccurrence(stack []ast.Node, name ast.Var) *ast.Location {
+	for i := range stack {


Also something I don't think this PR changes, but isn't this backwards? The stack starts from the outer most body, right? So if we start matching from that end, then we won't find shadowed variables in nested bodies, e.g.

p if { x = 5 # 3. but I'll find this every y in [1, 2] { x := 0 # 2. which should be this y > x # 1. I want to find the definition of this } x == 5 }

Hm .. I could imagine this working if we had let the compiler run the RewriteLocalVars stage, but we stop at the SetRuleTree stage just before it.

Yes! this was a bug, I have a fix for it in 2a989c7ca3d7d8705fa436483fa63777a1f9f5a1
with some more tests to check my thinking

Screen.Recording.2025-12-04.at.12.41.15.mov

This is a great example. 👾

johanfylling · 2025-12-03T14:18:53Z

v1/ast/oracle/find/some.go

+		}
+	}
+
+	if sd, ok := stack[len(stack)-1].(*ast.SomeDecl); ok {


This is mutually exclusive to the check above, right? Consider adding an else, or using a type-switch instead.

johanfylling · 2025-12-03T14:21:20Z

v1/ast/oracle/find/some.go

+
+type SomeLocator struct {
+	varLocator *VarLocator
+}


Could we also let the SomeLocator be an instance of VarLocator?

type SomeLocator struct { VarLocator }

johanfylling

Thank you!

Oracle is going to get some updates with the upcoming string interpolation work and I'd like to make it easier to work on with tests etc. I have restructured this into a 'Manager', with a number of 'Locators' for different ast constructs. I have tried to add some basic tests for the locators but pausing here for feedback. I hope to continue to improve this as I add support for string interp. Signed-off-by: Charlie Egan <[email protected]>

Signed-off-by: Charlie Egan <[email protected]>

This was meant to be easy to test and show what each locator was for, but in the end is mostly just duplicated in Find(). Signed-off-by: Charlie Egan <[email protected]>

charlieegan3 · 2025-12-04T13:08:26Z

Thanks Johan

In open-policy-agent#8105, I made some improvements I hoped would help make the oracle code easier to work on. Having spent some time trying to address a bug in the GTD of some/every vars I settled on this implementation instead. 1. Build a list of Nodes, with some special cases to include bodies of some / every. 2. Find what we are looking for, and create a target term (ref or var) 3. walk down the stack of nodes looking for the target once with a generic matcher returning matches from more specific to least. Walking into bodies where needed. 4. select and return the most specific match.

In open-policy-agent#8105, I made some improvements I hoped would help make the oracle code easier to work on. Having spent some time trying to address a bug in the GTD of some/every vars I settled on this implementation instead. 1. Build a list of Nodes, with some special cases to include bodies of some / every. 2. Find what we are looking for, and create a target term (ref or var) 3. walk down the stack of nodes looking for the target once with a generic matcher tracking the var or ref found. 4. return the most specific ref or var. Signed-off-by: Charlie Egan <[email protected]>

In open-policy-agent#8105, I made some improvements I hoped would help make the oracle code easier to work on. Having spent some time trying to address a bug in the GTD of some/every vars I settled on this implementation instead. 1. Build a list of Nodes, with some special cases to include bodies of some / every. 2. Find what we are looking for, and create a target term (ref or var) 3. walk down the stack of nodes looking for the target once with a generic matcher tracking the var or ref found. 4. return the most specific ref or var. This was originally done to make sure we could support string interpolation, but actually that worked almost out of the box. Review the oracle tests I've added here to see the new functionality. Signed-off-by: Charlie Egan <[email protected]>

In #8105, I made some improvements I hoped would help make the oracle code easier to work on. Having spent some time trying to address a bug in the GTD of some/every vars I settled on this implementation instead. 1. Build a list of Nodes, with some special cases to include bodies of some / every. 2. Find what we are looking for, and create a target term (ref or var) 3. walk down the stack of nodes looking for the target once with a generic matcher tracking the var or ref found. 4. return the most specific ref or var. This was originally done to make sure we could support string interpolation, but actually that worked almost out of the box. Review the oracle tests I've added here to see the new functionality. Signed-off-by: Charlie Egan <[email protected]>

charlieegan3 added the oracle label Dec 2, 2025

charlieegan3 force-pushed the oracle-refactor branch from 5c0c667 to 61828c6 Compare December 2, 2025 17:00

johanfylling approved these changes Dec 3, 2025

View reviewed changes

charlieegan3 force-pushed the oracle-refactor branch from 2a989c7 to 48b9976 Compare December 4, 2025 12:47

johanfylling approved these changes Dec 4, 2025

View reviewed changes

charlieegan3 added 3 commits December 4, 2025 13:07

Correctly find vars when there is shadowing

ec58808

Signed-off-by: Charlie Egan <[email protected]>

Remove Applicable(), duplicated in Find

f80ef7d

This was meant to be easy to test and show what each locator was for, but in the end is mostly just duplicated in Find(). Signed-off-by: Charlie Egan <[email protected]>

charlieegan3 force-pushed the oracle-refactor branch from c7ccd66 to f80ef7d Compare December 4, 2025 13:07

charlieegan3 enabled auto-merge (squash) December 4, 2025 13:07

charlieegan3 merged commit 82e4469 into open-policy-agent:main Dec 4, 2025
31 checks passed

charlieegan3 deleted the oracle-refactor branch December 4, 2025 13:18

charlieegan3 mentioned this pull request Dec 16, 2025

oracle: Refactor Oracle again to fix Some/Every #8131

Merged

BrewTestBot mentioned this pull request Dec 18, 2025

opa 1.12.0 Homebrew/homebrew-core#259206

Merged

oracle: Refactor oracle to be more extensible #8105

oracle: Refactor oracle to be more extensible #8105

Uh oh!

Conversation

charlieegan3 commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for openpolicyagent ready!

Uh oh!

johanfylling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charlieegan3 Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanfylling left a comment

Choose a reason for hiding this comment

Uh oh!

charlieegan3 commented Dec 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

charlieegan3 commented Dec 2, 2025 •

edited

Loading

netlify bot commented Dec 2, 2025 •

edited

Loading

charlieegan3 Dec 4, 2025 •

edited

Loading