Skip to content

noUnusedLocals: Destructuring assignment is a write #26365

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.

Sign up for GitHub

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

Merged
4 commits merged into from
Aug 28, 2018
Merged

noUnusedLocals: Destructuring assignment is a write #26365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aad4c9f
noUnusedLocals: Destructuring assignment is a write
Aug 10, 2018
f5cffb5
Code review
Aug 10, 2018
af67dc6
Merge branch 'master' into unusedLocals_destructureAssignment
Aug 27, 2018
33b3ef1
Clarify test
Aug 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
}

Expand Down
13 changes: 11 additions & 2 deletions tests/baselines/reference/noUnusedLocals_writeOnly.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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 {}

25 changes: 23 additions & 2 deletions tests/baselines/reference/noUnusedLocals_writeOnly.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 });
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 you need tests for more levels in destructuring (nested object literal)

Copy link
Member

Choose a reason for hiding this comment

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

shouldnt each test be moved to their own function to guarantee used is getting marked correctly for each expression type usage of x?

Copy link
Author

Choose a reason for hiding this comment

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

Every one of these tests only writes to x, so we're testing that none of these marks x as used. If a single test failed the baseline would change since x was used.

({ 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.
Expand All @@ -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(_) { }
51 changes: 46 additions & 5 deletions tests/baselines/reference/noUnusedLocals_writeOnly.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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))

Expand All @@ -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, --, --))

87 changes: 83 additions & 4 deletions tests/baselines/reference/noUnusedLocals_writeOnly.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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

Expand All @@ -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>

12 changes: 11 additions & 1 deletion tests/cases/compiler/noUnusedLocals_writeOnly.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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 {}