fix(chaining): preserve optional chaining in chained bindings#382
Open
ashley-hunter wants to merge 1 commit into
Open
fix(chaining): preserve optional chaining in chained bindings#382ashley-hunter wants to merge 1 commit into
ashley-hunter wants to merge 1 commit into
Conversation
When an element has multiple bindings, the chaining phase merges the
consecutive instructions into a single chained call, e.g.
`ɵɵproperty("a", x)("b", y)`. To do this it clones the arguments of
every binding after the first. That clone hard-coded `optional: false`
for property reads, keyed reads, and function invocations, which
silently dropped the `?.` optional-chaining flag on every binding
except the first.
Under native optional chaining (Angular v22+) this emitted `x.y`
instead of `x?.y`, `x[k]` instead of `x?.[k]`, and `fn()` instead of
`fn?.()`, throwing at runtime whenever the receiver was nullish. The
first binding was unaffected because it is the chain root and is never
cloned.
Copy the real `optional` flag from the source node instead of forcing
`false`. Legacy (pre-v22) output is unaffected: there the guard is a
`== null ? null` ternary rather than the `optional` flag, so the clone
was already correct.
Also documents why the two-way binding write-back clone
(`clone_output_expression`) must keep its assignment target
non-optional, since `a?.b = value` is not a valid assignment target.
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
1 participant
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.
Summary
Fixes a bug where the
?.safe-navigation operator was dropped from property, attribute, class, style, and host bindings when an element had more than one binding.When an element has multiple bindings, the chaining phase merges the consecutive instructions into a single chained call:
To build that chain it clones the arguments of every binding after the first. The clone hard-coded
optional: falsefor property reads, keyed reads, and function invocations, so the optional-chaining flag was silently dropped on every binding except the first (the chain root, which is never cloned).Impact
Under native optional chaining (Angular v22+), a template like:
compiled to:
The 2nd and 3rd bindings lost their guard (
ctx.c.dinstead ofctx.c?.d), throwingCannot read properties of null/undefinedat runtime whenever the receiver was nullish. Single-binding elements and the first binding of any chain were unaffected, which made it look intermittent.The same drop applied to
?.[k](keyed reads),?.()(safe calls), and to host property bindings, which go through the same chaining/clone path.Fix
Copy the real
optionalflag from the source node instead of forcingfalsein the chaining clone.Notes
(x == null ? null : x.y)ternary (a structural node cloned faithfully), not theoptionalflag, so the clone was already correct. Verified and pinned with a test.a?.b = valueis not a valid assignment target, so preserving the flag there would emit invalid JavaScript).Tests
Adds regression tests covering:
?.on every binding (not just the first)?.[k]and?.()in chained bindings?.nested inside larger expressions (!x?.y, chainedx?.y?.z, ternary branches)Each new v22 test was confirmed to fail before the fix and pass after. Full test suite and conformance (1264/1264) pass with no snapshot changes.