Allow passing own compiler to oracle #7354
Conversation
| @@ -45,6 +50,14 @@ type DefinitionQueryResult struct { | |||
| Result *ast.Location `json:"result"` | |||
| } | |||
|
|
|||
| // WithCompiler sets the compiler to use for the oracle. If not set, a new ast.Compiler | |||
| // will be created when needed. | |||
| func (o *Oracle) WithCompiler(compiler *ast.Compiler) *Oracle { | |||
There was a problem hiding this comment.
Do we need a setter for the compiler so that we can create an oracle in one place, and update it with a compiler in another? If not, maybe consider adding a vararg option to the New() factory func instead.
There was a problem hiding this comment.
That's what we do in Regal currently. Set the compiler once in a package-level var and then have it updated with the compiler per request. Not sure to what degree the compiler can be reused though 🤔 Are you saying we could have that set only once too?
There was a problem hiding this comment.
I think the internal call to Compiler.Compile(map[string]*Module) will reset the modules on the compiler at least; but maybe there is other state that would need to get wiped that isn't today 🤔 .
But if you're creating a new compiler for each round, then also creating a new oracle doesn't sound like much added overhead 😄.
There was a problem hiding this comment.
For sure, but we'd rather reuse the compiler too if that's an option :) I just assumed it wasn't given that the original implementation had it created per call.
v1/ast/oracle/oracle_test.go
Outdated
There was a problem hiding this comment.
Would be great if we could add a test for injecting a custom compiler. Could simply be one with a simple/no-op compile-stage added that we then declare in the call to compileUpto().
There was a problem hiding this comment.
Added a (trivial) test to assert the custom compiler is used.
Verified that this works in Regal, but also added a trivial test to assert that a custom compiler passed is used. Signed-off-by: Anders Eknert <[email protected]>
anderseknert
force-pushed
the
opa-oracle-2
branch
from
739d908 to
61db479
Compare
Verified that this works in Regal.