fix[next]: MergeLet guard blind to builtin-named params - #2728
Draft
havogt wants to merge 1 commit into
Draft
Conversation
`MergeLet` refuses to merge nested lets when the inner lambda's arguments
reference a parameter of the outer lambda. The check went through
`CountSymbolRefs.apply`, whose `ignore_builtins` argument defaults to `True`,
so a reference to an outer parameter whose name happens to coincide with a
builtin was never counted and the guard passed:
(λ(s) → (λ(b) → b)(·s))(it) stays as is
(λ(shift) → (λ(b) → b)(·shift))(it) becomes (λ(shift, b) → b)(it, ·shift)
The merged form moves `·shift` out of the lambda that binds `shift`, and a
semantics-preserving rename of the binder changes the result of the pass.
Pass `ignore_builtins=False` for that check and add the first unit tests for
`merge_let`.
Contributor
Author
|
Do we actually disallow shadowing builtins? Then I'll transform this PR in one that gives a proper error message at the different levels... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
MergeLetmerges(λ(a) → (λ(b) → body)(arg1))(arg2)into(λ(a, b) → body)(arg2, arg1). One of its guards checks that the inner lambda's arguments do not reference a parameter of the outer lambda — merging would move those references out of the binder that binds them.That check is defeated whenever the outer parameter's name coincides with a builtin name.
Reproduction
On
main:The two inputs differ only in the name of the outer binder.
Why this is a defect
(λ(shift, b) → b)(it, ·shift)has·shiftas an argument of the very lambda that bindsshift, i.e. the reference has been moved out of the scope of its binder. The occurrence now resolves to theshiftbuiltin (or to whatevershiftis in the enclosing scope) instead of to the parameter bound toit.stoshift— a semantics-preserving change — flips it from "do not merge" to "merge".Cause and fix
The guard uses
and
CountSymbolRefs.applydefaults toignore_builtins=True, soSymRefs whose name is a builtin are never counted. Here the names come fromouter_lambda.params, so a parameter shadowing a builtin makes the guard count zero references and let the merge through.The fix passes
ignore_builtins=Falsefor this check: within this lambda a reference to such a name is an ordinary reference to the parameter, not to the builtin.Tests
merge_lethad no unit tests upstream; this PR adds the first ones intests/next_tests/unit_tests/iterator_tests/transforms_tests/test_merge_let.py, covering the plain merge, the two existing "do not merge" guards, and the A/B pair above (parametrized overs/shift) so the alpha-invariance is visible. Theshiftcase fails before the fix and passes after.tests/next_tests/unit_tests/iterator_tests/is green (502 passed, 1 skipped, 2 xfailed).