Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix(typescript-estree): correct ChainExpression interaction with pare…
…ntheses and non-nulls (#2380)
  • Loading branch information
bradzacher committed Aug 29, 2020
1 parent d738fa4 commit 762bc99
Show file tree
Hide file tree
Showing 12 changed files with 348 additions and 401 deletions.
Expand Up @@ -7,8 +7,6 @@ import * as ts from 'typescript';
import * as semver from 'semver';
import * as util from '../util';

type MessageIds = 'noNonNullOptionalChain' | 'suggestRemovingNonNull';

const is3dot9 = semver.satisfies(
ts.version,
`>= 3.9.0 || >= 3.9.1-rc || >= 3.9.0-beta`,
Expand All @@ -17,7 +15,7 @@ const is3dot9 = semver.satisfies(
},
);

export default util.createRule<[], MessageIds>({
export default util.createRule({
name: 'no-non-null-asserted-optional-chain',
meta: {
type: 'problem',
Expand All @@ -37,64 +35,22 @@ export default util.createRule<[], MessageIds>({
},
defaultOptions: [],
create(context) {
const sourceCode = context.getSourceCode();

function isValidFor3dot9(node: TSESTree.ChainExpression): boolean {
if (!is3dot9) {
return false;
}

// TS3.9 made a breaking change to how non-null works with optional chains.
// Pre-3.9, `x?.y!.z` means `(x?.y).z` - i.e. it essentially scrubbed the optionality from the chain
// Post-3.9, `x?.y!.z` means `x?.y!.z` - i.e. it just asserts that the property `y` is non-null, not the result of `x?.y`.
// This means that for > 3.9, x?.y!.z is valid!
// NOTE: these cases are still invalid:
// - x?.y.z!
// - (x?.y)!.z

const parent = util.nullThrows(
node.parent,
util.NullThrowsReasons.MissingParent,
);
const grandparent = util.nullThrows(
parent.parent,
util.NullThrowsReasons.MissingParent,
);

if (
grandparent.type !== AST_NODE_TYPES.MemberExpression &&
grandparent.type !== AST_NODE_TYPES.CallExpression
) {
return false;
}

const lastChildToken = util.nullThrows(
sourceCode.getLastToken(parent, util.isNotNonNullAssertionPunctuator),
util.NullThrowsReasons.MissingToken('last token', node.type),
);
if (util.isClosingParenToken(lastChildToken)) {
return false;
}

const tokenAfterNonNull = sourceCode.getTokenAfter(parent);
if (
tokenAfterNonNull != null &&
util.isClosingParenToken(tokenAfterNonNull)
) {
return false;
}
// TS3.9 made a breaking change to how non-null works with optional chains.
// Pre-3.9, `x?.y!.z` means `(x?.y).z` - i.e. it essentially scrubbed the optionality from the chain
// Post-3.9, `x?.y!.z` means `x?.y!.z` - i.e. it just asserts that the property `y` is non-null, not the result of `x?.y`.
// This means that for > 3.9, x?.y!.z is valid!
//
// NOTE: these cases are still invalid for 3.9:
// - x?.y.z!
// - (x?.y)!.z

return true;
}

return {
const baseSelectors = {
// non-nulling a wrapped chain will scrub all nulls introduced by the chain
// (x?.y)!
// (x?.())!
'TSNonNullExpression > ChainExpression'(
node: TSESTree.ChainExpression,
): void {
if (isValidFor3dot9(node)) {
return;
}

// selector guarantees this assertion
const parent = node.parent as TSESTree.TSNonNullExpression;
context.report({
Expand All @@ -114,6 +70,84 @@ export default util.createRule<[], MessageIds>({
],
});
},

// non-nulling at the end of a chain will scrub all nulls introduced by the chain
// x?.y!
// x?.()!
'ChainExpression > TSNonNullExpression'(
node: TSESTree.TSNonNullExpression,
): void {
context.report({
node,
messageId: 'noNonNullOptionalChain',
// use a suggestion instead of a fixer, because this can obviously break type checks
suggest: [
{
messageId: 'suggestRemovingNonNull',
fix(fixer): TSESLint.RuleFix {
return fixer.removeRange([node.range[1] - 1, node.range[1]]);
},
},
],
});
},
};

if (is3dot9) {
return baseSelectors;
}

return {
...baseSelectors,
[[
// > :not(ChainExpression) because that case is handled by a previous selector
'MemberExpression > TSNonNullExpression.object > :not(ChainExpression)',
'CallExpression > TSNonNullExpression.callee > :not(ChainExpression)',
].join(', ')](child: TSESTree.Node): void {
// selector guarantees this assertion
const node = child.parent as TSESTree.TSNonNullExpression;

let current = child;
while (current) {
switch (current.type) {
case AST_NODE_TYPES.MemberExpression:
if (current.optional) {
// found an optional chain! stop traversing
break;
}

current = current.object;
continue;

case AST_NODE_TYPES.CallExpression:
if (current.optional) {
// found an optional chain! stop traversing
break;
}

current = current.callee;
continue;

default:
// something that's not a ChainElement, which means this is not an optional chain we want to check
return;
}
}

context.report({
node,
messageId: 'noNonNullOptionalChain',
// use a suggestion instead of a fixer, because this can obviously break type checks
suggest: [
{
messageId: 'suggestRemovingNonNull',
fix(fixer): TSESLint.RuleFix {
return fixer.removeRange([node.range[1] - 1, node.range[1]]);
},
},
],
});
},
};
},
});
6 changes: 6 additions & 0 deletions packages/eslint-plugin/src/rules/prefer-optional-chain.ts
Expand Up @@ -234,6 +234,12 @@ export default util.createRule({
}

if (node.type === AST_NODE_TYPES.ChainExpression) {
/* istanbul ignore if */ if (
node.expression.type === AST_NODE_TYPES.TSNonNullExpression
) {
// this shouldn't happen
return '';
}
return getText(node.expression);
}

Expand Down
Expand Up @@ -8,7 +8,11 @@ const ruleTester = new RuleTester({
ruleTester.run('no-non-null-asserted-optional-chain', rule, {
valid: [
'foo.bar!;',
'foo.bar!.baz;',
'foo.bar!.baz();',
'foo.bar()!;',
'foo.bar()!();',
'foo.bar()!.baz;',
'foo?.bar;',
'foo?.bar();',
'(foo?.bar).baz!;',
Expand Down
5 changes: 4 additions & 1 deletion packages/types/src/ts-estree.ts
Expand Up @@ -308,7 +308,10 @@ export type Node =
export type Accessibility = 'public' | 'protected' | 'private';
export type BindingPattern = ArrayPattern | ObjectPattern;
export type BindingName = BindingPattern | Identifier;
export type ChainElement = CallExpression | MemberExpression;
export type ChainElement =
| CallExpression
| MemberExpression
| TSNonNullExpression;
export type ClassElement =
| ClassProperty
| MethodDefinition
Expand Down
38 changes: 26 additions & 12 deletions packages/typescript-estree/src/convert.ts
Expand Up @@ -14,7 +14,7 @@ import {
getTextForTokenKind,
getTSNodeAccessibility,
hasModifier,
isChildOptionalChain,
isChildUnwrappableOptionalChain,
isComma,
isComputedProperty,
isESTreeClassMember,
Expand Down Expand Up @@ -406,24 +406,36 @@ export class Converter {
tsNode:
| ts.PropertyAccessExpression
| ts.ElementAccessExpression
| ts.CallExpression,
| ts.CallExpression
| ts.NonNullExpression,
): TSESTree.ChainExpression | TSESTree.ChainElement {
let child = (node.type === AST_NODE_TYPES.MemberExpression
? node.object
: node.callee) as TSESTree.Node;
const isChildOptional = isChildOptionalChain(tsNode, child);
const { child, isOptional } = ((): {
child: TSESTree.Node;
isOptional: boolean;
} => {
if (node.type === AST_NODE_TYPES.MemberExpression) {
return { child: node.object, isOptional: node.optional };
}
if (node.type === AST_NODE_TYPES.CallExpression) {
return { child: node.callee, isOptional: node.optional };
}
return { child: node.expression, isOptional: false };
})();
const isChildUnwrappable = isChildUnwrappableOptionalChain(tsNode, child);

if (!isChildOptional && !node.optional) {
if (!isChildUnwrappable && !isOptional) {
return node;
}

if (isChainExpression(child)) {
if (isChildUnwrappable && isChainExpression(child)) {
// unwrap the chain expression child
child = child.expression;
const newChild = child.expression;
if (node.type === AST_NODE_TYPES.MemberExpression) {
node.object = child;
node.object = newChild;
} else if (node.type === AST_NODE_TYPES.CallExpression) {
node.callee = newChild;
} else {
node.callee = child;
node.expression = newChild;
}
}

Expand Down Expand Up @@ -2209,10 +2221,12 @@ export class Converter {
}

case SyntaxKind.NonNullExpression: {
return this.createNode<TSESTree.TSNonNullExpression>(node, {
const nnExpr = this.createNode<TSESTree.TSNonNullExpression>(node, {
type: AST_NODE_TYPES.TSNonNullExpression,
expression: this.convertChild(node.expression),
});

return this.convertChainExpression(nnExpr, node);
}

case SyntaxKind.TypeLiteral: {
Expand Down
30 changes: 3 additions & 27 deletions packages/typescript-estree/src/node-utils.ts
@@ -1,7 +1,6 @@
import unescape from 'lodash/unescape';
import * as ts from 'typescript';
import { AST_NODE_TYPES, AST_TOKEN_TYPES, TSESTree } from './ts-estree';
import { typescriptVersionIsAtLeast } from './version-check';

const SyntaxKind = ts.SyntaxKind;

Expand Down Expand Up @@ -452,11 +451,12 @@ export function isChainExpression(
/**
* Returns true of the child of property access expression is an optional chain
*/
export function isChildOptionalChain(
export function isChildUnwrappableOptionalChain(
node:
| ts.PropertyAccessExpression
| ts.ElementAccessExpression
| ts.CallExpression,
| ts.CallExpression
| ts.NonNullExpression,
child: TSESTree.Node,
): boolean {
if (
Expand All @@ -467,30 +467,6 @@ export function isChildOptionalChain(
return true;
}

if (!typescriptVersionIsAtLeast['3.9']) {
return false;
}

// TS3.9 made a breaking change to how non-null works with optional chains.
// Pre-3.9, `x?.y!.z` means `(x?.y).z` - i.e. it essentially scrubbed the optionality from the chain
// Post-3.9, `x?.y!.z` means `x?.y!.z` - i.e. it just asserts that the property `y` is non-null, not the result of `x?.y`

if (
child.type !== AST_NODE_TYPES.TSNonNullExpression ||
!isChainExpression(child.expression)
) {
return false;
}

if (
// make sure it's not (x.y)!.z
node.expression.kind === ts.SyntaxKind.NonNullExpression &&
(node.expression as ts.NonNullExpression).expression.kind !==
ts.SyntaxKind.ParenthesizedExpression
) {
return true;
}

return false;
}

Expand Down
Expand Up @@ -23,7 +23,8 @@ export interface EstreeToTsNodeTypes {
[AST_NODE_TYPES.ChainExpression]:
| ts.CallExpression
| ts.PropertyAccessExpression
| ts.ElementAccessExpression;
| ts.ElementAccessExpression
| ts.NonNullExpression;
[AST_NODE_TYPES.ClassBody]: ts.ClassDeclaration | ts.ClassExpression;
[AST_NODE_TYPES.ClassDeclaration]: ts.ClassDeclaration;
[AST_NODE_TYPES.ClassExpression]: ts.ClassExpression;
Expand Down
23 changes: 0 additions & 23 deletions packages/typescript-estree/tests/ast-alignment/fixtures-to-test.ts
Expand Up @@ -411,21 +411,6 @@ tester.addFixturePatternConfig('typescript/basics', {
* https://github.com/babel/babel/issues/11939
*/
'catch-clause-with-annotation',

/**
* Optional chaining
* Babel has updated to ESTree's representation, and we haven't yet
* TODO: remove this with the v4 release
*/
'optional-chain-call-with-non-null-assertion',
'optional-chain-call-with-parens',
'optional-chain-call',
'optional-chain-element-access-with-non-null-assertion',
'optional-chain-element-access-with-parens',
'optional-chain-element-access',
'optional-chain-with-non-null-assertion',
'optional-chain-with-parens',
'optional-chain',
],
ignoreSourceType: [
/**
Expand Down Expand Up @@ -479,14 +464,6 @@ tester.addFixturePatternConfig('typescript/decorators/property-decorators', {

tester.addFixturePatternConfig('typescript/expressions', {
fileType: 'ts',
ignore: [
/**
* Optional chaining
* Babel has updated to ESTree's representation, and we haven't yet
* TODO: remove this with the v4 release
*/
'optional-call-expression-type-arguments',
],
});

tester.addFixturePatternConfig('typescript/errorRecovery', {
Expand Down

0 comments on commit 762bc99

Please sign in to comment.