fix(compiler-cli): correctly resolve symbol for SafePropertyRead in c… - #70155
fix(compiler-cli): correctly resolve symbol for SafePropertyRead in c…#70155atscott wants to merge 1 commit into
Conversation
…hained optional navigation When resolving template symbols for SafePropertyRead in TCBs emitted with optional chaining (strictSafeNavigationTypes: true), SymbolBuilder falls back to finding a TS node matching the AST expression's nameSpan. It then traverses up through parent nodes to find the enclosing expression. Previously, the traversal loop checked isAccessExpression(node.parent) without verifying whether node was the accessed member name or the expression receiver. When multiple optional navigation expressions are chained (e.g. route?.data?.['icon']), the parent of ((route)?.data) is an access expression where ((route)?.data) is the receiver. Because isAccessExpression was true, the loop continued ascending into the outer access expression, causing symbol resolution for data to erroneously return the symbol and TCB location of icon. This commit refines the parent traversal condition so that it only climbs into a parent PropertyAccessExpression or ElementAccessExpression if node is the accessed name (node.parent.name === node) or argument expression (node.parent.argumentExpression === node), preventing escape into outer receiver expressions.
| ts.isNonNullExpression(node.parent) || | ||
| isAccessExpression(node.parent)) | ||
| (ts.isPropertyAccessExpression(node.parent) && node.parent.name === node) || | ||
| (ts.isElementAccessExpression(node.parent) && node.parent.argumentExpression === node)) |
There was a problem hiding this comment.
AGENT: Review Feedback
Great catch on the isAccessExpression bug! However, by keeping the isElementAccessExpression check in the refined loop, this introduces a new bug.
Because this fallback logic is wrapped in if (node === null && expression instanceof SafePropertyRead), we are only ever resolving a SafePropertyRead. A SafePropertyRead will only generate a PropertyAccessExpression, never an ElementAccessExpression.
By checking (ts.isElementAccessExpression(node.parent) && node.parent.argumentExpression === node), if a user writes an expression where the SafePropertyRead is an argument (e.g. arr[route?.data!]), this loop will incorrectly ascend out of the SafePropertyRead and into the outer ElementAccessExpression!
We should drop the element access check completely:
| (ts.isElementAccessExpression(node.parent) && node.parent.argumentExpression === node)) | |
| (ts.isPropertyAccessExpression(node.parent) && node.parent.name === node)) |
…hained optional navigation
When resolving template symbols for SafePropertyRead in TCBs emitted with optional chaining (strictSafeNavigationTypes: true), SymbolBuilder falls back to finding a TS node matching the AST expression's nameSpan. It then traverses up through parent nodes to find the enclosing expression.
Previously, the traversal loop checked isAccessExpression(node.parent) without verifying whether node was the accessed member name or the expression receiver. When multiple optional navigation expressions are chained (e.g. route?.data?.['icon']), the parent of ((route)?.data) is an access expression where ((route)?.data) is the receiver. Because isAccessExpression was true, the loop continued ascending into the outer access expression, causing symbol resolution for data to erroneously return the symbol and TCB location of icon.
This commit refines the parent traversal condition so that it only climbs into a parent PropertyAccessExpression or ElementAccessExpression if node is the accessed name (node.parent.name === node) or argument expression (node.parent.argumentExpression === node), preventing escape into outer receiver expressions.