Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add refactor convertToOptionalChainExpression #39135

Merged
merged 49 commits into from
Jul 14, 2020

Conversation

jessetrinity
Copy link
Contributor

@jessetrinity jessetrinity commented Jun 18, 2020

closes #35018

Adds a refactor to convert && binary expressions to optional chain expressions.

Supported conversions:

//before
[|a && a.b && a.b.c|];
//after
a?.b?.c;
//before
[|a && a.b && a.b.c()|];
//after
a?.b?.c();

We offer the refactor to convert ternary expressions to nullish coalescing expressions if the type of c is not one of null, unknown, undefined, or any:

//before
[|a.b ? a.b.c : "false"|];
//after
a.b?.c ?? "false";

An explicit request ("invoked") for a 0-length span will check the largest containing binary expression for a refactor:

// before
a && a.b && [||]a.b.c;

// is equivalent to
[|a && a.b && a.b.c|];

// after
a?.b?.c;

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to think through the main binary expression loop in getInfo, here are the comments I've got so far.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of your test cases are expression statements, but expression statements that don’t immediately contain call expressions are somewhat rare. If the idea is to automatically find a suitable expression node from a zero-width span, I think you could try just walking up ancestors until you see something that’s unlikely to be part of an expression you can transform, like a statement. For example, say you have

return foo(a && a.b && a.b/*|*/.c);

You start on an Identifier, and walk up because an Identifier is obviously part of something you could transform. One parent up is a PropertyAccessExpression, which could also be part of a candidate, so you keep walking. Another PropertyAccessExpression, keep going. A BinaryExpression with an && token, keep going, while saving this node as a candidate expression. Now, since you have a candidate BinaryExpression, I think you’d want to continue only if the next parent is also a candidate BinaryExpression. It’s not, so you use the BinaryExpression as the potential node to transform.

I’m probably oversimplifying this in my head, but I think you at least want to make sure the refactor works for

  • return statement expressions
  • call arguments
  • variable initializers

in addition to ExpressionStatements.

src/services/refactors/convertToOptionalChainExpression.ts Outdated Show resolved Hide resolved
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't have enough time to look at the refactor again, but I did review the tests and I think I found a mistake in the emit.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments on the new syntactic code, plus a suggestion for how to handle a && a.b && a.b().

*/
function getLastPropertyAccessChain(node: Expression): PropertyAccessExpression | undefined {
// foo && |foo.bar === 1|; - here the right child of the && binary expression is another binary expression.
// the rightmost member of the && chain should be the leftmost child of that expression.
if (isBinaryExpression(node)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also node = skipParentheses(node) at the top here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be an example we care to test, something like a && (a).b?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, foo && (foo.bar === 1) seems more realistic since the precedence between different binary operators can be hard to remember, but I think if you put skipParentheses in the right places, you can trivially handle foo && (((foo).bar) === 1) 😁

src/services/refactors/convertToOptionalChainExpression.ts Outdated Show resolved Hide resolved
src/services/refactors/convertToOptionalChainExpression.ts Outdated Show resolved Hide resolved
if (fullMatch && !isInJSFile(source)) {
Debug.assert(checker.getSymbolAtLocation(source) === checker.getSymbolAtLocation(target));
}
return fullMatch;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there’s possibly a less efficient but more easily understandable way to write this function by going depth-first, grabbing the left-most name of both the chain and the subchain right away, and walking up their parents. But I couldn’t write an elegant example in a few minutes, so maybe it wouldn’t be as good as I think it would.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the easiest to understand way to break it down, in my mind, would be to have a function that returns the array ["a", "b", "c", "d"] for the PropertyAccessExpression a.b.c.d (it could return strings or Identifiers, either way), then

function chainStartsWith(chain: EntityNameExpression, subchain: EntityNameExpression) {
  const subchainIdentifiers = getIdentifiersInEntityName(subchain);
  const chainIdentifiers = getIdentifiersInEntityName(chain);
  return every(subchain, (identifier, index) => {
    return identifier.getText() === chainIdentifiers[index]?.getText();
  });
}

This would be a bit more eager than what you have, and would allocate some arrays you don’t strictly need, but is easier to follow to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this example I also presuppose the types EntityNameExpression to guarantee that you have a string of dot-separated identifiers, because it makes this function easier to reason about and there are already isEntityNameExpression functions you can use, but if you wanted to nitpick on efficiency, you could check on the fly in getIdentifiersInEntityName, returning undefined if you discover that you don’t actually have an EntityNameExpression.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this make other cases we care about easier to implement (maybe optional element access) in addition to making it easier to read? It does feel quite awkward to go through the entirety of both access chains to build the arrays if we would terminate on the first check we would have made otherwise. On the other hand, really long access chains in binary expressions are probably not common anyway?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think expanding to optional element access should be pretty easy either way.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of tiny style suggestions left.

return undefined;
function getMatchingStart(chain: Expression, subchain: Expression): PropertyAccessExpression | Identifier | undefined {
if (!isIdentifier(subchain) && !isPropertyAccessExpression(subchain)) return undefined;
return chainStartsWith(chain, subchain) ? subchain : undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work as well? Seems easier to read than double !

Suggested change
return chainStartsWith(chain, subchain) ? subchain : undefined;
return (isIdentifier(subchain) || isPropertyAccessExpression(subchain)) && chainStartsWith(chain, subchain) ? subchain : undefined;

@@ -251,10 +234,10 @@ namespace ts.refactor.convertToOptionalChainExpression {
}

function doChange(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void {
const { lastPropertyAccessChain, occurrences, expression } = info;
const { finalExpression: lastPropertyAccessChain, occurrences, expression } = info;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just change the name to finalExpression here too?

@jessetrinity jessetrinity merged commit 1702238 into microsoft:master Jul 14, 2020
@sandersn
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring to convert && chain to optional chain expression
6 participants