Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rrb3942
Copy link
Contributor

@rrb3942 rrb3942 commented May 22, 2024

See issue #637

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

Commit 3da8527 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
Copy link
Member

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
Copy link
Contributor Author

rrb3942 commented Jun 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants