noUnusedLocals: Destructuring assignment is a write (#26365)

Andy · web-flow · commit 3931b72118e8 · 2018-08-28T11:43:45.000-07:00
* noUnusedLocals: Destructuring assignment is a write

* Code review

* Clarify test
diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -2331,6 +2331,13 @@ namespace ts {
         return node;
     }
 
+    function skipParenthesesUp(node: Node): Node {
+        while (node.kind === SyntaxKind.ParenthesizedExpression) {
+            node = node.parent;
+        }
+        return node;
+    }
+
     // a node is delete target iff. it is PropertyAccessExpression/ElementAccessExpression with parentheses skipped
     export function isDeleteTarget(node: Node): boolean {
         if (node.kind !== SyntaxKind.PropertyAccessExpression && node.kind !== SyntaxKind.ElementAccessExpression) {
@@ -4206,6 +4213,8 @@ namespace ts {
         if (!parent) return AccessKind.Read;
 
         switch (parent.kind) {
+            case SyntaxKind.ParenthesizedExpression:
+                return accessKind(parent);
             case SyntaxKind.PostfixUnaryExpression:
             case SyntaxKind.PrefixUnaryExpression:
                 const { operator } = parent as PrefixUnaryExpression | PostfixUnaryExpression;
@@ -4217,13 +4226,35 @@ namespace ts {
                     : AccessKind.Read;
             case SyntaxKind.PropertyAccessExpression:
                 return (parent as PropertyAccessExpression).name !== node ? AccessKind.Read : accessKind(parent);
+            case SyntaxKind.PropertyAssignment: {
+                const parentAccess = accessKind(parent.parent);
+                // In `({ x: varname }) = { x: 1 }`, the left `x` is a read, the right `x` is a write.
+                return node === (parent as PropertyAssignment).name ? reverseAccessKind(parentAccess) : parentAccess;
+            }
+            case SyntaxKind.ShorthandPropertyAssignment:
+                // Assume it's the local variable being accessed, since we don't check public properties for --noUnusedLocals.
+                return node === (parent as ShorthandPropertyAssignment).objectAssignmentInitializer ? AccessKind.Read : accessKind(parent.parent);
+            case SyntaxKind.ArrayLiteralExpression:
+                return accessKind(parent);
             default:
                 return AccessKind.Read;
         }
 
         function writeOrReadWrite(): AccessKind {
             // If grandparent is not an ExpressionStatement, this is used as an expression in addition to having a side effect.
-            return parent.parent && parent.parent.kind === SyntaxKind.ExpressionStatement ? AccessKind.Write : AccessKind.ReadWrite;
+            return parent.parent && skipParenthesesUp(parent.parent).kind === SyntaxKind.ExpressionStatement ? AccessKind.Write : AccessKind.ReadWrite;
+        }
+    }
+    function reverseAccessKind(a: AccessKind): AccessKind {
+        switch (a) {
+            case AccessKind.Read:
+                return AccessKind.Write;
+            case AccessKind.Write:
+                return AccessKind.Read;
+            case AccessKind.ReadWrite:
+                return AccessKind.ReadWrite;
+            default:
+                return Debug.assertNever(a);
         }
     }
 
diff --git a/tests/baselines/reference/noUnusedLocals_writeOnly.errors.txt b/tests/baselines/reference/noUnusedLocals_writeOnly.errors.txt
@@ -1,14 +1,22 @@
 tests/cases/compiler/noUnusedLocals_writeOnly.ts(1,12): error TS6133: 'x' is declared but its value is never read.
-tests/cases/compiler/noUnusedLocals_writeOnly.ts(10,9): error TS6133: 'z' is declared but its value is never read.
+tests/cases/compiler/noUnusedLocals_writeOnly.ts(18,9): error TS6133: 'z' is declared but its value is never read.
 
 
 ==== tests/cases/compiler/noUnusedLocals_writeOnly.ts (2 errors) ====
-    function f(x = 0) {
+    function f(x = 0, b = false) {
                ~
 !!! error TS6133: 'x' is declared but its value is never read.
+        // None of these statements read from 'x', so it will be marked unused.
         x = 1;
         x++;
         x /= 2;
+        ([x] = [1]);
+        ({ x } = { x: 1 });
+        ({ x: x } = { x: 1 });
+        ({ a: [{ b: x }] } = { a: [{ b: 1 }] });
+        ({ x = 2 } = { x: b ? 1 : undefined });
+        let used = 1;
+        ({ x = used } = { x: b ? 1 : undefined });
     
         let y = 0;
         // This is a write access to y, but not a write-*only* access.
@@ -19,4 +27,5 @@ tests/cases/compiler/noUnusedLocals_writeOnly.ts(10,9): error TS6133: 'z' is dec
 !!! error TS6133: 'z' is declared but its value is never read.
         f(z = 1); // This effectively doesn't use `z`, values just pass through it.
     }
+    function f2(_: ReadonlyArray<number>): void {}
     
diff --git a/tests/baselines/reference/noUnusedLocals_writeOnly.js b/tests/baselines/reference/noUnusedLocals_writeOnly.js
@@ -1,8 +1,16 @@
 //// [noUnusedLocals_writeOnly.ts]
-function f(x = 0) {
+function f(x = 0, b = false) {
+    // None of these statements read from 'x', so it will be marked unused.
     x = 1;
     x++;
     x /= 2;
+    ([x] = [1]);
+    ({ x } = { x: 1 });
+    ({ x: x } = { x: 1 });
+    ({ a: [{ b: x }] } = { a: [{ b: 1 }] });
+    ({ x = 2 } = { x: b ? 1 : undefined });
+    let used = 1;
+    ({ x = used } = { x: b ? 1 : undefined });
 
     let y = 0;
     // This is a write access to y, but not a write-*only* access.
@@ -11,17 +19,30 @@ function f(x = 0) {
     let z = 0;
     f(z = 1); // This effectively doesn't use `z`, values just pass through it.
 }
+function f2(_: ReadonlyArray<number>): void {}
 
 
 //// [noUnusedLocals_writeOnly.js]
-function f(x) {
+"use strict";
+function f(x, b) {
     if (x === void 0) { x = 0; }
+    if (b === void 0) { b = false; }
+    var _a, _b;
+    // None of these statements read from 'x', so it will be marked unused.
     x = 1;
     x++;
     x /= 2;
+    (x = [1][0]);
+    (x = { x: 1 }.x);
+    (x = { x: 1 }.x);
+    (x = { a: [{ b: 1 }] }.a[0].b);
+    (_a = { x: b ? 1 : undefined }.x, x = _a === void 0 ? 2 : _a);
+    var used = 1;
+    (_b = { x: b ? 1 : undefined }.x, x = _b === void 0 ? used : _b);
     var y = 0;
     // This is a write access to y, but not a write-*only* access.
     f(y++);
     var z = 0;
     f(z = 1); // This effectively doesn't use `z`, values just pass through it.
 }
+function f2(_) { }
diff --git a/tests/baselines/reference/noUnusedLocals_writeOnly.symbols b/tests/baselines/reference/noUnusedLocals_writeOnly.symbols
@@ -1,8 +1,10 @@
 === tests/cases/compiler/noUnusedLocals_writeOnly.ts ===
-function f(x = 0) {
+function f(x = 0, b = false) {
 >f : Symbol(f, Decl(noUnusedLocals_writeOnly.ts, 0, 0))
 >x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 0, 11))
+>b : Symbol(b, Decl(noUnusedLocals_writeOnly.ts, 0, 17))
 
+    // None of these statements read from 'x', so it will be marked unused.
     x = 1;
 >x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 0, 11))
 
@@ -12,19 +14,58 @@ function f(x = 0) {
     x /= 2;
 >x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 0, 11))
 
+    ([x] = [1]);
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 0, 11))
+
+    ({ x } = { x: 1 });
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 6, 6))
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 6, 14))
+
+    ({ x: x } = { x: 1 });
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 7, 6))
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 0, 11))
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 7, 17))
+
+    ({ a: [{ b: x }] } = { a: [{ b: 1 }] });
+>a : Symbol(a, Decl(noUnusedLocals_writeOnly.ts, 8, 6))
+>b : Symbol(b, Decl(noUnusedLocals_writeOnly.ts, 8, 12))
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 0, 11))
+>a : Symbol(a, Decl(noUnusedLocals_writeOnly.ts, 8, 26))
+>b : Symbol(b, Decl(noUnusedLocals_writeOnly.ts, 8, 32))
+
+    ({ x = 2 } = { x: b ? 1 : undefined });
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 9, 6))
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 9, 18))
+>b : Symbol(b, Decl(noUnusedLocals_writeOnly.ts, 0, 17))
+>undefined : Symbol(undefined)
+
+    let used = 1;
+>used : Symbol(used, Decl(noUnusedLocals_writeOnly.ts, 10, 7))
+
+    ({ x = used } = { x: b ? 1 : undefined });
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 11, 6))
+>used : Symbol(used, Decl(noUnusedLocals_writeOnly.ts, 10, 7))
+>x : Symbol(x, Decl(noUnusedLocals_writeOnly.ts, 11, 21))
+>b : Symbol(b, Decl(noUnusedLocals_writeOnly.ts, 0, 17))
+>undefined : Symbol(undefined)
+
     let y = 0;
->y : Symbol(y, Decl(noUnusedLocals_writeOnly.ts, 5, 7))
+>y : Symbol(y, Decl(noUnusedLocals_writeOnly.ts, 13, 7))
 
     // This is a write access to y, but not a write-*only* access.
     f(y++);
 >f : Symbol(f, Decl(noUnusedLocals_writeOnly.ts, 0, 0))
->y : Symbol(y, Decl(noUnusedLocals_writeOnly.ts, 5, 7))
+>y : Symbol(y, Decl(noUnusedLocals_writeOnly.ts, 13, 7))
 
     let z = 0;
->z : Symbol(z, Decl(noUnusedLocals_writeOnly.ts, 9, 7))
+>z : Symbol(z, Decl(noUnusedLocals_writeOnly.ts, 17, 7))
 
     f(z = 1); // This effectively doesn't use `z`, values just pass through it.
 >f : Symbol(f, Decl(noUnusedLocals_writeOnly.ts, 0, 0))
->z : Symbol(z, Decl(noUnusedLocals_writeOnly.ts, 9, 7))
+>z : Symbol(z, Decl(noUnusedLocals_writeOnly.ts, 17, 7))
 }
+function f2(_: ReadonlyArray<number>): void {}
+>f2 : Symbol(f2, Decl(noUnusedLocals_writeOnly.ts, 19, 1))
+>_ : Symbol(_, Decl(noUnusedLocals_writeOnly.ts, 20, 12))
+>ReadonlyArray : Symbol(ReadonlyArray, Decl(lib.es5.d.ts, --, --))
 
diff --git a/tests/baselines/reference/noUnusedLocals_writeOnly.types b/tests/baselines/reference/noUnusedLocals_writeOnly.types
@@ -1,9 +1,12 @@
 === tests/cases/compiler/noUnusedLocals_writeOnly.ts ===
-function f(x = 0) {
->f : (x?: number) => void
+function f(x = 0, b = false) {
+>f : (x?: number, b?: boolean) => void
 >x : number
 >0 : 0
+>b : boolean
+>false : false
 
+    // None of these statements read from 'x', so it will be marked unused.
     x = 1;
 >x = 1 : 1
 >x : number
@@ -18,14 +21,87 @@ function f(x = 0) {
 >x : number
 >2 : 2
 
+    ([x] = [1]);
+>([x] = [1]) : [number]
+>[x] = [1] : [number]
+>[x] : [number]
+>x : number
+>[1] : [number]
+>1 : 1
+
+    ({ x } = { x: 1 });
+>({ x } = { x: 1 }) : { x: number; }
+>{ x } = { x: 1 } : { x: number; }
+>{ x } : { x: number; }
+>x : number
+>{ x: 1 } : { x: number; }
+>x : number
+>1 : 1
+
+    ({ x: x } = { x: 1 });
+>({ x: x } = { x: 1 }) : { x: number; }
+>{ x: x } = { x: 1 } : { x: number; }
+>{ x: x } : { x: number; }
+>x : number
+>x : number
+>{ x: 1 } : { x: number; }
+>x : number
+>1 : 1
+
+    ({ a: [{ b: x }] } = { a: [{ b: 1 }] });
+>({ a: [{ b: x }] } = { a: [{ b: 1 }] }) : { a: [{ b: number; }]; }
+>{ a: [{ b: x }] } = { a: [{ b: 1 }] } : { a: [{ b: number; }]; }
+>{ a: [{ b: x }] } : { a: [{ b: number; }]; }
+>a : [{ b: number; }]
+>[{ b: x }] : [{ b: number; }]
+>{ b: x } : { b: number; }
+>b : number
+>x : number
+>{ a: [{ b: 1 }] } : { a: [{ b: number; }]; }
+>a : [{ b: number; }]
+>[{ b: 1 }] : [{ b: number; }]
+>{ b: 1 } : { b: number; }
+>b : number
+>1 : 1
+
+    ({ x = 2 } = { x: b ? 1 : undefined });
+>({ x = 2 } = { x: b ? 1 : undefined }) : { x?: number | undefined; }
+>{ x = 2 } = { x: b ? 1 : undefined } : { x?: number | undefined; }
+>{ x = 2 } : { x?: number; }
+>x : number
+>2 : 2
+>{ x: b ? 1 : undefined } : { x?: number | undefined; }
+>x : number | undefined
+>b ? 1 : undefined : 1 | undefined
+>b : boolean
+>1 : 1
+>undefined : undefined
+
+    let used = 1;
+>used : number
+>1 : 1
+
+    ({ x = used } = { x: b ? 1 : undefined });
+>({ x = used } = { x: b ? 1 : undefined }) : { x?: number | undefined; }
+>{ x = used } = { x: b ? 1 : undefined } : { x?: number | undefined; }
+>{ x = used } : { x?: number; }
+>x : number
+>used : number
+>{ x: b ? 1 : undefined } : { x?: number | undefined; }
+>x : number | undefined
+>b ? 1 : undefined : 1 | undefined
+>b : boolean
+>1 : 1
+>undefined : undefined
+
     let y = 0;
 >y : number
 >0 : 0
 
     // This is a write access to y, but not a write-*only* access.
     f(y++);
 >f(y++) : void
->f : (x?: number) => void
+>f : (x?: number, b?: boolean) => void
 >y++ : number
 >y : number
 
@@ -35,9 +111,12 @@ function f(x = 0) {
 
     f(z = 1); // This effectively doesn't use `z`, values just pass through it.
 >f(z = 1) : void
->f : (x?: number) => void
+>f : (x?: number, b?: boolean) => void
 >z = 1 : 1
 >z : number
 >1 : 1
 }
+function f2(_: ReadonlyArray<number>): void {}
+>f2 : (_: ReadonlyArray<number>) => void
+>_ : ReadonlyArray<number>
 
diff --git a/tests/cases/compiler/noUnusedLocals_writeOnly.ts b/tests/cases/compiler/noUnusedLocals_writeOnly.ts
@@ -1,10 +1,19 @@
+// @strict: true
 // @noUnusedLocals: true
 // @noUnusedParameters: true
 
-function f(x = 0) {
+function f(x = 0, b = false) {
+    // None of these statements read from 'x', so it will be marked unused.
     x = 1;
     x++;
     x /= 2;
+    ([x] = [1]);
+    ({ x } = { x: 1 });
+    ({ x: x } = { x: 1 });
+    ({ a: [{ b: x }] } = { a: [{ b: 1 }] });
+    ({ x = 2 } = { x: b ? 1 : undefined });
+    let used = 1;
+    ({ x = used } = { x: b ? 1 : undefined });
 
     let y = 0;
     // This is a write access to y, but not a write-*only* access.
@@ -13,3 +22,4 @@ function f(x = 0) {
     let z = 0;
     f(z = 1); // This effectively doesn't use `z`, values just pass through it.
 }
+function f2(_: ReadonlyArray<number>): void {}
