Fixed detection of optional chains containing a reference#59144
Fixed detection of optional chains containing a reference#59144Andarist wants to merge 3 commits intomicrosoft:mainfrom
Conversation
| while (isOptionalChain(source)) { | ||
| source = source.expression; | ||
| if (isMatchingReference(source, target)) { | ||
| if (isMatchingReference(target, source)) { |
There was a problem hiding this comment.
I think an extra refactor should be made here.
isMatchingReference accepts reference as the first parameter but optionalChainContainsReference accepts it as the second one (!). That caused the issue here as both of them refer to both of the parameters as source and target - so it wasn't apparent that the reference (called a target here) is passed to isMatchingReference at the position that is used for candidate expressions (also called a target by that function).
I think it would make sense to:
- rename those parameters to something like
expressionandreferenceor maybecandidateandreference.isMatchingReferencesometimes calls recursively itself with adjustedsource(what I propose to call areferencehere) but I think that doesn't impact the proposed naming - use consistent order of parameters. I think what
optionalChainContainsReferenceuses is better (referencepassed as the second argument to it) butisMatchingReferenceis likely an older function so maybe the team would prefer that.
There was a problem hiding this comment.
This function is nearly identical to containsMatchingReference aside from only supporting optional chaining. I'm not sure argument order is the issue here.
There was a problem hiding this comment.
isMatchingReference walks both source and target to different extents. It may just be that some check on one side or the other is not exhaustive enough.
There was a problem hiding this comment.
This function is nearly identical to containsMatchingReference aside from only supporting optional chaining.
Yeah, it is almost identical - and notice that source and target are passed down to isMatchingReference by them at different parameter positions. That only makes me believe more that the argument order is the issue here.
isMatchingReference walks both source and target to different extents. It may just be that some check on one side or the other is not exhaustive enough.
It may be that. But I'm not sure to what extent that function should be walking both so I'm hesitant to make changes there as previous authors haven't embed that logic related to optional chains there.
I re-read the code now and analyzed this again and I'm only more convinced that the argument order is incorrect here. Fixing this in isMatchingReference (without changing the adjusted argument order) would require fixing this at its end (in the switch/case that checks source):
case SyntaxKind.BinaryExpression:
return (isBinaryExpression(source) && source.operatorToken.kind === SyntaxKind.CommaToken && isMatchingReference(source.right, target));The case here doesn't fit what is handled here at all, it does fit what is handled on the target side though:
case SyntaxKind.BinaryExpression:
return (isAssignmentExpression(target) && isMatchingReference(source, target.left)) ||
(isBinaryExpression(target) && target.operatorToken.kind === SyntaxKind.CommaToken && isMatchingReference(source, target.right));So given that and the fact that arguments really seem to be semantically flipped when judging by other calls to isMatchingReference, I'm inclined to argue the current fix is the correct one.
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
rbuckton
left a comment
rbuckton
left a comment
There was a problem hiding this comment.
Argument order is likely not the culprit here.
5 participants
fixes #56998
fixes #60855