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.

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

Ignored catch parameter #17517

Merged
merged 17 commits into from
Aug 8, 2017
Merged

Ignored catch parameter #17517

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b17bd97
Clean up
tinganho Jul 30, 2017
99c662b
Add diagnostics
tinganho Jul 30, 2017
b917eb0
Adds optional catch parameter into the compiler
tinganho Jul 30, 2017
148a494
Adds transformers for ignored catch parameter
tinganho Jul 30, 2017
e349943
Add tests
tinganho Jul 30, 2017
90ea10e
Accept baselines
tinganho Jul 30, 2017
c65e5a1
Removes valid test cases from invalid test case file
tinganho Jul 30, 2017
de38ef7
remove debugger
tinganho Jul 30, 2017
58c8a2c
adds test for try-catch-finally
tinganho Jul 30, 2017
81fb3f7
Addresses CR feedback
tinganho Aug 1, 2017
dbdbb05
Accept baselines
tinganho Aug 1, 2017
1926285
Fixes CR comments
tinganho Aug 2, 2017
053b708
Accept baseline
tinganho Aug 2, 2017
d5c24f3
Addresses CR comment
tinganho Aug 2, 2017
f9e85ec
Adds missing test cases
tinganho Aug 2, 2017
4f3e13a
Typo
tinganho Aug 2, 2017
3da1a53
Amend comment about explicitly setting catch clause variables to 'und…
DanielRosenwasser Aug 2, 2017
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
5 changes: 4 additions & 1 deletion src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2921,7 +2921,10 @@ namespace ts {
function computeCatchClause(node: CatchClause, subtreeFlags: TransformFlags) {
let transformFlags = subtreeFlags;

if (node.variableDeclaration && isBindingPattern(node.variableDeclaration.name)) {
if (!node.variableDeclaration) {
transformFlags |= TransformFlags.AssertESNext;
}
else if (isBindingPattern(node.variableDeclaration.name)) {
transformFlags |= TransformFlags.AssertES2015;
}

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ namespace ts {
Type,
ResolvedBaseConstructorType,
DeclaredType,
ResolvedReturnType
ResolvedReturnType,
}

const enum CheckMode {
Expand Down
10 changes: 6 additions & 4 deletions src/compiler/emitter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2131,10 +2131,12 @@ namespace ts {
function emitCatchClause(node: CatchClause) {
const openParenPos = writeToken(SyntaxKind.CatchKeyword, node.pos);
write(" ");
writeToken(SyntaxKind.OpenParenToken, openParenPos);
emit(node.variableDeclaration);
writeToken(SyntaxKind.CloseParenToken, node.variableDeclaration ? node.variableDeclaration.end : openParenPos);
write(" ");
if (node.variableDeclaration) {
writeToken(SyntaxKind.OpenParenToken, openParenPos);
emit(node.variableDeclaration);
writeToken(SyntaxKind.CloseParenToken, node.variableDeclaration ? node.variableDeclaration.end : openParenPos);
Copy link
Member

Choose a reason for hiding this comment

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

The conditional on this line is no longer required since you are checking node.variableDeclaration on 2134.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

write(" ");
}
emit(node.block);
}

Expand Down
5 changes: 3 additions & 2 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4778,11 +4778,12 @@ namespace ts {
function parseCatchClause(): CatchClause {
const result = <CatchClause>createNode(SyntaxKind.CatchClause);
parseExpected(SyntaxKind.CatchKeyword);
if (parseExpected(SyntaxKind.OpenParenToken)) {

if (parseOptional(SyntaxKind.OpenParenToken)) {
result.variableDeclaration = parseVariableDeclaration();
Copy link
Member

Choose a reason for hiding this comment

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

Conditionally setting result.variableDeclaration introduces polymorphism as we now have two CatchClause shapes, one with the property and one without. This can cause performance degradation in V8 (NodeJS). I would recommend you add an else clause that sets result.variableDeclaration = undefined to keep the shape of CatchClause the same regardless of which form the user writes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

parseExpected(SyntaxKind.CloseParenToken);
}

parseExpected(SyntaxKind.CloseParenToken);
result.block = parseBlock(/*ignoreMissingOpenBrace*/ false);
return finishNode(result);
}
Expand Down
1 change: 1 addition & 0 deletions src/compiler/transformers/es2015.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3173,6 +3173,7 @@ namespace ts {
function visitCatchClause(node: CatchClause): CatchClause {
const ancestorFacts = enterSubtree(HierarchyFacts.BlockScopeExcludes, HierarchyFacts.BlockScopeIncludes);
let updated: CatchClause;
Debug.assert(!!node.variableDeclaration, "Catch clauses should always be present when downleveling ES2015 code.");
Copy link
Member

Choose a reason for hiding this comment

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

How about "Catch clause variable should always be present when downleveling ES2015"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

if (isBindingPattern(node.variableDeclaration.name)) {
const temp = createTempVariable(/*recordTempVariable*/ undefined);
const newVariableDeclaration = createVariableDeclaration(temp);
Expand Down
9 changes: 9 additions & 0 deletions src/compiler/transformers/esnext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ namespace ts {
return visitExpressionStatement(node as ExpressionStatement);
case SyntaxKind.ParenthesizedExpression:
return visitParenthesizedExpression(node as ParenthesizedExpression, noDestructuringValue);
case SyntaxKind.CatchClause:
return visitCatchClause(node as CatchClause);
default:
return visitEachChild(node, visitor, context);
}
Expand Down Expand Up @@ -212,6 +214,13 @@ namespace ts {
return visitEachChild(node, noDestructuringValue ? visitorNoDestructuringValue : visitor, context);
}

function visitCatchClause(node: CatchClause): CatchClause {
if (!node.variableDeclaration) {
return updateCatchClause(node, createVariableDeclaration(createTempVariable(/*recordTempVariable*/ undefined)), node.block);
Copy link
Member

@weswigham weswigham Aug 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

node.block still needs to be recursively visited here, no?

function* doFoo(foo: any) {
  try {
     throw "whatever";
  }
  catch {
    try {
       throw "again"
    }
    catch {
      for await (const c of foo) {
        c;
      }
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

@weswigham is correct, you should recursively visit the block of the catch clause.

}
return visitEachChild(node, visitor, context);
}

/**
* Visits a BinaryExpression that contains a destructuring assignment.
*
Expand Down
5 changes: 2 additions & 3 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1807,8 +1807,8 @@ namespace ts {

export interface CatchClause extends Node {
kind: SyntaxKind.CatchClause;
parent?: TryStatement;
variableDeclaration: VariableDeclaration;
parent?: TryStatement; // We make this optional to parse missing try statements
Copy link
Member

Choose a reason for hiding this comment

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

The comment is unnecessary. We only re-introduce parent here to refine its type for methods in the checker. parent is optional because it parent pointers are not always set, especially during tree transformations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

variableDeclaration?: VariableDeclaration;
Copy link
Member

Choose a reason for hiding this comment

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

Can you update createCatchClause and updateCatchClause in factory.ts and add | undefined to the variableDeclaration parameters in both cases? This is needed for those using the Compiler API with -strictNullChecks enabled for their project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

block: Block;
}

Expand Down Expand Up @@ -4001,7 +4001,6 @@ namespace ts {
ContainsBindingPattern = 1 << 23,
ContainsYield = 1 << 24,
ContainsHoistedDeclarationOrCompletion = 1 << 25,

ContainsDynamicImport = 1 << 26,

// Please leave this as 1 << 29.
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/constructorWithIncompleteTypeAnnotation.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(138,13): error T
tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(141,32): error TS1005: '{' expected.
tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(143,13): error TS1005: 'try' expected.
tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(159,24): error TS1109: Expression expected.
tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(159,30): error TS1005: '(' expected.
tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(159,30): error TS1005: '{' expected.
tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(159,31): error TS2304: Cannot find name 'Property'.
tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(166,13): error TS2365: Operator '+=' cannot be applied to types 'number' and 'void'.
tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(180,40): error TS2447: The '^' operator is not allowed for boolean types. Consider using '!==' instead.
Expand Down Expand Up @@ -323,7 +323,7 @@ tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(261,1): error TS
~~~~~
!!! error TS1109: Expression expected.
~
!!! error TS1005: '(' expected.
!!! error TS1005: '{' expected.
~~~~~~~~
!!! error TS2304: Cannot find name 'Property'.
retVal += c.Member();
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/constructorWithIncompleteTypeAnnotation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ var BasicFeatures = (function () {
var xx = c;
retVal += ;
try { }
catch () { }
catch (_a) { }
Property;
retVal += c.Member();
retVal += xx.Foo() ? 0 : 1;
Expand Down
11 changes: 11 additions & 0 deletions tests/baselines/reference/emitter.ignoredCatchParameter.esnext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//// [emitter.ignoredCatchParameter.esnext.ts]
function fn() {
try {} catch {}
}


//// [emitter.ignoredCatchParameter.esnext.js]
function fn() {
try { }
catch { }
}
7 changes: 7 additions & 0 deletions tests/baselines/reference/emitter.ignoredCatchParameter.esnext.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
=== tests/cases/conformance/emitter/esnext/noCatchParameter/emitter.ignoredCatchParameter.esnext.ts ===
function fn() {
>fn : Symbol(fn, Decl(emitter.ignoredCatchParameter.esnext.ts, 0, 0))

try {} catch {}
}

7 changes: 7 additions & 0 deletions tests/baselines/reference/emitter.ignoredCatchParameter.esnext.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
=== tests/cases/conformance/emitter/esnext/noCatchParameter/emitter.ignoredCatchParameter.esnext.ts ===
function fn() {
>fn : () => void

try {} catch {}
}

17 changes: 5 additions & 12 deletions tests/baselines/reference/invalidTryStatements2.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,12 @@
tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts(3,13): error TS1005: '(' expected.
tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts(6,5): error TS1005: 'try' expected.
tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts(12,5): error TS1005: 'try' expected.
tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts(13,5): error TS1005: 'try' expected.
tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts(2,5): error TS1005: 'try' expected.
tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts(8,5): error TS1005: 'try' expected.
tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts(9,5): error TS1005: 'try' expected.
tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts(18,5): error TS1005: 'try' expected.
tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts(22,5): error TS1005: 'try' expected.
tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts(26,5): error TS1005: 'try' expected.


==== tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts (6 errors) ====
==== tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts (5 errors) ====
function fn() {
try {
} catch { // syntax error, missing '(x)'
~
!!! error TS1005: '(' expected.
}

catch(x) { } // error missing try
~~~~~
!!! error TS1005: 'try' expected.
Expand Down
8 changes: 0 additions & 8 deletions tests/baselines/reference/invalidTryStatements2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
//// [invalidTryStatements2.ts]
function fn() {
try {
} catch { // syntax error, missing '(x)'
}

catch(x) { } // error missing try

finally{ } // potential error; can be absorbed by the 'catch'
Expand All @@ -30,10 +26,6 @@ function fn2() {

//// [invalidTryStatements2.js]
function fn() {
try {
}
catch () {
}
try {
}
catch (x) { } // error missing try
Expand Down
18 changes: 11 additions & 7 deletions tests/baselines/reference/tryStatements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,30 @@
//// [tryStatements.ts]
function fn() {
try {
try { } catch { }

} catch (x) {
var x: any;
}
try { } catch (x) { var x: any; }

try { } finally { }

try { }catch(z){ } finally { }
try { } catch { } finally { }

try { } catch(z) { } finally { }
}

//// [tryStatements.js]
function fn() {
try {
}
try { }
catch (_a) { }
try { }
catch (x) {
var x;
}
try { }
finally { }
try { }
catch (_b) { }
finally { }
try { }
catch (z) { }
finally { }
}
17 changes: 8 additions & 9 deletions tests/baselines/reference/tryStatements.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,16 @@
function fn() {
>fn : Symbol(fn, Decl(tryStatements.ts, 0, 0))

try {
try { } catch { }

} catch (x) {
>x : Symbol(x, Decl(tryStatements.ts, 3, 13))

var x: any;
>x : Symbol(x, Decl(tryStatements.ts, 4, 11))
}
try { } catch (x) { var x: any; }
>x : Symbol(x, Decl(tryStatements.ts, 3, 19))
>x : Symbol(x, Decl(tryStatements.ts, 3, 27))

try { } finally { }

try { }catch(z){ } finally { }
>z : Symbol(z, Decl(tryStatements.ts, 9, 17))
try { } catch { } finally { }

try { } catch(z) { } finally { }
>z : Symbol(z, Decl(tryStatements.ts, 9, 18))
}
11 changes: 5 additions & 6 deletions tests/baselines/reference/tryStatements.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,16 @@
function fn() {
>fn : () => void

try {
try { } catch { }

} catch (x) {
try { } catch (x) { var x: any; }
>x : any

var x: any;
>x : any
}

try { } finally { }

try { }catch(z){ } finally { }
try { } catch { } finally { }

try { } catch(z) { } finally { }
>z : any
}
4 changes: 0 additions & 4 deletions tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
function fn() {
try {
} catch { // syntax error, missing '(x)'
}

catch(x) { } // error missing try

finally{ } // potential error; can be absorbed by the 'catch'
Expand Down
11 changes: 6 additions & 5 deletions tests/cases/conformance/statements/tryStatements/tryStatements.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@

function fn() {
try {
try { } catch { }

} catch (x) {
var x: any;
}
try { } catch (x) { var x: any; }

try { } finally { }

try { }catch(z){ } finally { }
try { } catch { } finally { }

try { } catch(z) { } finally { }
}