Clean up convert-to-async refactor #36858
Conversation
…ntly fixed some unnoticed bugs
| @@ -32,7 +32,8 @@ namespace ts.codefix { | |||
| readonly kind: SynthBindingNameKind.Identifier; | |||
| readonly identifier: Identifier; | |||
| readonly types: Type[]; | |||
| numberOfAssignmentsOriginal: number; // number of times the variable should be assigned in the refactor | |||
| /** A declaration for this identifier has already been generated */ | |||
| hasBeenDeclared: boolean; | |||
There was a problem hiding this comment.
This was used for determining whether to declare variables with let or const, but I determined the only time let ever occurred was after generating code like
let x: string | number;
try {
x = await getString();
} catch {
x = await getNumber();
}and numberOfAssignmentsOriginal was being explicitly set to 2. In every other code path, the number was always 0. So, the property became a more descriptively-named boolean.
| function getConstIdentifiers(synthNamesMap: ReadonlyMap<SynthIdentifier>): Identifier[] { | ||
| const constIdentifiers: Identifier[] = []; | ||
| synthNamesMap.forEach((val) => { | ||
| if (val.numberOfAssignmentsOriginal === 0) { |
There was a problem hiding this comment.
As mentioned earlier, at the time this function was called, this condition was always true.
| Returns true if node is a promise returning expression | ||
| If name is not undefined, node is a promise returning call of name | ||
| */ | ||
| function isPromiseReturningExpression(node: Node, checker: TypeChecker, name?: string): boolean { |
There was a problem hiding this comment.
This function had a totally different meaning based on whether name was provided, so I split it up
| function deepCloneCallback(node: Node, clone: Node) { | ||
| if (isIdentifier(node)) { | ||
| const symbol = checker.getSymbolAtLocation(node); | ||
| const symboldIdString = symbol && getSymbolId(symbol).toString(); | ||
| const renameInfo = symbol && synthNamesMap.get(symboldIdString!); | ||
|
|
||
| if (renameInfo) { | ||
| const type = checker.getTypeAtLocation(node); | ||
| originalType.set(getNodeId(clone).toString(), type); | ||
| } | ||
| } | ||
|
|
||
| const val = setOfAllExpressionsToReturn.get(getNodeId(node).toString()); | ||
| if (val !== undefined) { | ||
| setOfAllExpressionsToReturn.delete(getNodeId(node).toString()); | ||
| setOfAllExpressionsToReturn.set(getNodeId(clone).toString(), val); | ||
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
This function runs for every (original, clone) pair of nodes during getSynthesizedDeepClone, and it was essentially just eagerly doing a lot of work that could be done on demand. I think the root reason it exists is because getDeepSynthesizedClone was failing to set node.original on identifiers, and so this was previously the last chance to get a reference to any identifiers in the original source. After fixing cloning identifiers, this function became useless, because you can always count on clonedNode.original to exist.
| } | ||
| else { | ||
| const identifier = getSynthesizedDeepClone(node); | ||
| identsToRenameMap.set(symbolIdString, identifier); | ||
| synthNamesMap.set(symbolIdString, createSynthIdentifier(identifier, [], allVarNames.filter(elem => elem.identifier.text === node.text).length/*, numberOfAssignmentsSynthesized: 0*/)); |
There was a problem hiding this comment.
This filter expression always evaluated to 0, and I couldn’t figure out a test case to make it not 0, but I’m not 100% convinced one doesn’t exist.
There was a problem hiding this comment.
It should be noted that I did find an additional case where let should be used instead of const, but the code before this PR produces the wrong result (uses const):
There was a problem hiding this comment.
Can you add a test for this if it's not already covered?
There was a problem hiding this comment.
Sorry, I meant that both master and my PR produce an incorrect result on this pattern. I planned to fix it and add a test for it in a later PR. This PR attempts not to change the existing tested behavior at all (but two bugs were fixed accidentally by cloning identifiers correctly).
| Debug.assertNode(node.original!.parent, isPropertyAccessExpression); | ||
| return transformPromiseExpressionOfPropertyAccess(node, transformer, prevArgName); |
There was a problem hiding this comment.
This condition on node.original.parent always holds in the existing tests, but I’m not 100% sure why.
| function getFlagOfBindingName(bindingName: SynthBindingName, constIdentifiers: readonly Identifier[]): NodeFlags { | ||
| const identifiers = getIdentifierTextsFromBindingName(getNode(bindingName)); | ||
| const inArr: boolean = constIdentifiers.some(elem => contains(identifiers, elem.text)); | ||
| return inArr ? NodeFlags.Const : NodeFlags.Let; |
There was a problem hiding this comment.
It was always NodeFlags.Const.
| function transformPromiseCall(node: Expression, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { | ||
| const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(node).toString()); | ||
| // the identifier is empty when the handler (.then()) ignores the argument - In this situation we do not need to save the result of the promise returning call | ||
| const originalNodeParent = node.original ? node.original.parent : node.parent; |
There was a problem hiding this comment.
This shows the confusion caused by node.original being missing for identifiers. There seems to be an assumption that if originalNodeParent was missing, it was probably a property access expression. What I found after ensuring that node.original.parent always exists is that it is always a property access expression, so parts of the following condition were always true.
There was a problem hiding this comment.
Also, the original name was confusing—what is a “promise call”? The node passed here wasn’t even a call expression much of the time.
| getSynthesizedDeepClone(getNode(variableName)), | ||
| /*type*/ undefined, | ||
| rightHandSide)], | ||
| NodeFlags.Const))]; |
There was a problem hiding this comment.
I guess I'm a little surprised this always comes out const, but if nothing breaks...
|
Thanks @uniqueiniquity! Looking forward to seeing the future bug reports that demonstrate what all this deleted code was supposed to do in the first place 😄 |
I have several convert-to-async bugs assigned to me, and as I started to work on them, I was frustrated by vague names, and was also confused about what some blocks of code were doing. To figure it out, I set breakpoints in the confusing code and debugged all the tests, and the breakpoints were never hit. So, as I was unable to determine the utility of large chunks of logic, and they had zero test coverage (and yet the refactor on the whole has pretty good test coverage), I decided to delete those chunks of logic. I think that much of it was predicated on impossible conditions, but I can’t be 100% certain.
There was also a bit of logical branching that I determined was only necessary because identifiers weren’t being properly cloned in
getSynthesizedDeepClone. After fixing that, two test baselines changed, revealing that they actually contained bugs before. (Remarkably, one was missing anawaitand seemed to be fundamentally broken; the other was just a comment being deleted when it should have been preserved. But otherwise, this PR does not change the behavior of the refactor as observable in any existing test.