Skip to content

Flag middle comma SEF operands #33648

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

Closed
wants to merge 3 commits into from
Closed

Flag middle comma SEF operands #33648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79037c6
Add a test
RyanCavanaugh Sep 27, 2019
52240dd
Update unrelated files
RyanCavanaugh Sep 27, 2019
d851fd2
A comma operator is SEF if *either* of its operands are
RyanCavanaugh Sep 27, 2019
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
8 changes: 7 additions & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24378,7 +24378,7 @@ namespace ts {
}

/**
* This is a *shallow* check: An expression is side-effect-free if the
* This is a *shallow* check: An expression is side-effect-free (SEF) if the
* evaluation of the expression *itself* cannot produce side effects.
* For example, x++ / 3 is side-effect free because the / operator
* does not have side effects.
Expand Down Expand Up @@ -24419,6 +24419,12 @@ namespace ts {
if (isAssignmentOperator((node as BinaryExpression).operatorToken.kind)) {
return false;
}
if ((node as BinaryExpression).operatorToken.kind === SyntaxKind.CommaToken) {
// A comma operator is SEF if either operand is SEF, e.g. the template argument in
// `The coordinates are ${x.toString(), y, z}`
// contains an illegally SEF expression at 'y' (the left side of the outer comma whose right operand is 'z')
return isSideEffectFree((node as BinaryExpression).left) || isSideEffectFree((node as BinaryExpression).right);
Copy link
Member

Choose a reason for hiding this comment

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

So this is less isSideEffectFree and more containsSideEffectFree now, right? Since technically the comma operator itself would only actually be side-effect free if everything it executed was free of side effects?

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh Sep 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so. f() + 4 contains a non-side-effect-free expression, but is side-effect-free due to the addition operator itself not being something that can produce side effects*

Upon thinking about this more, I think the correct fix is just to return true if the operator in a binary expression isn't an assignment operator. The expression [(f(), 4, 2)] is equally wrong as [(f() + 4, 2)]; the comma is a red herring. I'll try this on Monday.

* let's continue to pretend valueOf doesn't exist, per convention

}
return isSideEffectFree((node as BinaryExpression).left) &&
isSideEffectFree((node as BinaryExpression).right);

Expand Down
9 changes: 9 additions & 0 deletions tests/baselines/reference/comma-sef-operand.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
tests/cases/compiler/comma-sef-operand.ts(2,16): error TS2695: Left side of comma operator is unused and has no side effects.


==== tests/cases/compiler/comma-sef-operand.ts (1 errors) ====
declare const obj: any;
console.log(`${JSON.stringify(obj), undefined, 2}`);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2695: Left side of comma operator is unused and has no side effects.

7 changes: 7 additions & 0 deletions tests/baselines/reference/comma-sef-operand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//// [comma-sef-operand.ts]
declare const obj: any;
console.log(`${JSON.stringify(obj), undefined, 2}`);


//// [comma-sef-operand.js]
console.log("" + (JSON.stringify(obj), undefined, 2));
14 changes: 14 additions & 0 deletions tests/baselines/reference/comma-sef-operand.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
=== tests/cases/compiler/comma-sef-operand.ts ===
declare const obj: any;
>obj : Symbol(obj, Decl(comma-sef-operand.ts, 0, 13))

console.log(`${JSON.stringify(obj), undefined, 2}`);
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>console : Symbol(console, Decl(lib.dom.d.ts, --, --))
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>JSON.stringify : Symbol(JSON.stringify, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>JSON : Symbol(JSON, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>stringify : Symbol(JSON.stringify, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>obj : Symbol(obj, Decl(comma-sef-operand.ts, 0, 13))
>undefined : Symbol(undefined)

20 changes: 20 additions & 0 deletions tests/baselines/reference/comma-sef-operand.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
=== tests/cases/compiler/comma-sef-operand.ts ===
declare const obj: any;
>obj : any

console.log(`${JSON.stringify(obj), undefined, 2}`);
>console.log(`${JSON.stringify(obj), undefined, 2}`) : void
>console.log : (message?: any, ...optionalParams: any[]) => void
>console : Console
>log : (message?: any, ...optionalParams: any[]) => void
>`${JSON.stringify(obj), undefined, 2}` : string
>JSON.stringify(obj), undefined, 2 : 2
>JSON.stringify(obj), undefined : undefined
>JSON.stringify(obj) : string
>JSON.stringify : { (value: any, replacer?: (this: any, key: string, value: any) => any, space?: string | number): string; (value: any, replacer?: (string | number)[], space?: string | number): string; }
>JSON : JSON
>stringify : { (value: any, replacer?: (this: any, key: string, value: any) => any, space?: string | number): string; (value: any, replacer?: (string | number)[], space?: string | number): string; }
>obj : any
>undefined : undefined
>2 : 2

4 changes: 2 additions & 2 deletions tests/baselines/reference/importExportInternalComments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ declare module "foo";
/*1*/ import /*2*/ D /*3*/, /*4*/ { /*5*/ A /*6*/, /*7*/ B /*8*/ as /*9*/ C /*10*/ } /*11*/ from /*12*/ "foo";
/*1*/ import /*2*/ * /*3*/ as /*4*/ foo /*5*/ from /*6*/ "foo";

void D, A, C, foo; // Use the variables to prevent ellision
void D, void A, void C, void foo; // Use the variables to prevent ellision

/*1*/ export /*2*/ { /*3*/ A /*4*/, /*5*/ B /*6*/ as /*7*/ C /*8*/ } /*9*/ from /*10*/ "foo";
/*1*/ export /*2*/ * /*3*/ from /*4*/ "foo"
Expand All @@ -20,6 +20,6 @@ void D, A, C, foo; // Use the variables to prevent ellision
//// [index.js]
/*1*/ import /*2*/ D /*3*/, /*4*/ { /*5*/ A /*6*/, /*7*/ B /*8*/ as /*9*/ C /*10*/ } /*11*/ from /*12*/ "foo";
/*1*/ import /*2*/ * /*3*/ as /*4*/ foo /*5*/ from /*6*/ "foo";
void D, A, C, foo; // Use the variables to prevent ellision
void D, void A, void C, void foo; // Use the variables to prevent ellision
/*1*/ export /*2*/ { /*3*/ A /*4*/, /*5*/ B /*6*/ as /*7*/ C /*8*/ } /*9*/ from /*10*/ "foo";
/*1*/ export /*2*/ * /*3*/ from /*4*/ "foo";
2 changes: 1 addition & 1 deletion tests/baselines/reference/importExportInternalComments.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ declare module "foo";
/*1*/ import /*2*/ * /*3*/ as /*4*/ foo /*5*/ from /*6*/ "foo";
>foo : Symbol(foo, Decl(index.ts, 1, 12))

void D, A, C, foo; // Use the variables to prevent ellision
void D, void A, void C, void foo; // Use the variables to prevent ellision
>D : Symbol(D, Decl(index.ts, 0, 12))
>A : Symbol(A, Decl(index.ts, 0, 35))
>C : Symbol(C, Decl(index.ts, 0, 50))
Expand Down
11 changes: 7 additions & 4 deletions tests/baselines/reference/importExportInternalComments.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,17 @@ declare module "foo";
/*1*/ import /*2*/ * /*3*/ as /*4*/ foo /*5*/ from /*6*/ "foo";
>foo : any

void D, A, C, foo; // Use the variables to prevent ellision
>void D, A, C, foo : any
>void D, A, C : any
>void D, A : any
void D, void A, void C, void foo; // Use the variables to prevent ellision
>void D, void A, void C, void foo : undefined
>void D, void A, void C : undefined
>void D, void A : undefined
>void D : undefined
>D : any
>void A : undefined
>A : any
>void C : undefined
>C : any
>void foo : undefined
>foo : any

/*1*/ export /*2*/ { /*3*/ A /*4*/, /*5*/ B /*6*/ as /*7*/ C /*8*/ } /*9*/ from /*10*/ "foo";
Expand Down
2 changes: 2 additions & 0 deletions tests/cases/compiler/comma-sef-operand.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
declare const obj: any;
console.log(`${JSON.stringify(obj), undefined, 2}`);
2 changes: 1 addition & 1 deletion tests/cases/compiler/importExportInternalComments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ declare module "foo";
/*1*/ import /*2*/ D /*3*/, /*4*/ { /*5*/ A /*6*/, /*7*/ B /*8*/ as /*9*/ C /*10*/ } /*11*/ from /*12*/ "foo";
/*1*/ import /*2*/ * /*3*/ as /*4*/ foo /*5*/ from /*6*/ "foo";

void D, A, C, foo; // Use the variables to prevent ellision
void D, void A, void C, void foo; // Use the variables to prevent ellision

/*1*/ export /*2*/ { /*3*/ A /*4*/, /*5*/ B /*6*/ as /*7*/ C /*8*/ } /*9*/ from /*10*/ "foo";
/*1*/ export /*2*/ * /*3*/ from /*4*/ "foo"