expr-lang / expr

Expression language and expression evaluation for Go
https://expr-lang.org
MIT License
5.85k stars 378 forks source link

Separate patching phases for patchers that require multiple passes. #659

Open rrb3942 opened 1 month ago

rrb3942 commented 1 month ago

See issue #637

This contains the changes from pull request #658 as well. If this pull is accepted that one may be closed.

Commit https://github.com/expr-lang/expr/commit/3da85278439a5e8ef5dc0d73f321f76742e2cecc introduced repeating the patch phase for operator overloading, but also reruns all patchers any time an operator overload is patched.

This can break patchers that don't expect to be re-run over the statement multiple times.

This patch set splits patching into two phases. First phase only patchers that don't require multiple passes are run. Second phase only patchers that require multiple passes are run.

As a side effect, it also allows any ordering between other patchers and operator overloads, where as previously if a patcher changed a nodes type it would have to be supplied as an option before the operator.

Passes all tests.

ok      github.com/expr-lang/expr       0.138s
ok      github.com/expr-lang/expr/ast   (cached)
ok      github.com/expr-lang/expr/builtin       0.015s
ok      github.com/expr-lang/expr/checker       0.013s
?       github.com/expr-lang/expr/conf  [no test files]
ok      github.com/expr-lang/expr/compiler      0.007s
ok      github.com/expr-lang/expr/docgen        (cached)
ok      github.com/expr-lang/expr/file  (cached)
ok      github.com/expr-lang/expr/internal/deref        (cached)
ok      github.com/expr-lang/expr/internal/difflib      (cached)
ok      github.com/expr-lang/expr/internal/spew (cached)
ok      github.com/expr-lang/expr/internal/testify/assert       (cached)
ok      github.com/expr-lang/expr/internal/testify/assert/internal/unsafetests  (cached)
ok      github.com/expr-lang/expr/internal/testify/require      (cached)
?       github.com/expr-lang/expr/parser/operator       [no test files]
?       github.com/expr-lang/expr/parser/utils  [no test files]
ok      github.com/expr-lang/expr/optimizer     0.009s
ok      github.com/expr-lang/expr/parser        (cached)
ok      github.com/expr-lang/expr/parser/lexer  (cached)
ok      github.com/expr-lang/expr/patcher       0.004s
ok      github.com/expr-lang/expr/patcher/value 0.004s
ok      github.com/expr-lang/expr/test/coredns  0.004s
ok      github.com/expr-lang/expr/test/crowdsec 0.052s
ok      github.com/expr-lang/expr/test/deref    0.005s
?       github.com/expr-lang/expr/test/mock     [no test files]
?       github.com/expr-lang/expr/test/playground       [no test files]
?       github.com/expr-lang/expr/vm/func_types [no test files]
?       github.com/expr-lang/expr/vm/runtime/helpers    [no test files]
ok      github.com/expr-lang/expr/test/fuzz     0.540s
ok      github.com/expr-lang/expr/test/gen      0.619s
ok      github.com/expr-lang/expr/test/interface_method 0.003s
ok      github.com/expr-lang/expr/test/operator 0.005s
ok      github.com/expr-lang/expr/test/operator/issues584       0.004s
ok      github.com/expr-lang/expr/test/patch    0.004s
ok      github.com/expr-lang/expr/test/patch/set_type   0.004s
ok      github.com/expr-lang/expr/test/pipes    0.004s
ok      github.com/expr-lang/expr/test/time     0.003s
ok      github.com/expr-lang/expr/vm    0.016s
ok      github.com/expr-lang/expr/vm/runtime    (cached)
antonmedv commented 3 weeks ago

This can break patchers that don't expect to be re-run over the statement multiple times.

Maybe it makes sense to add a test for this use case.

rrb3942 commented 3 weeks ago

Rebased and added a test to catch if patchers are run more times than expected. I verified that the test fails without the other fixes.