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 9 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 (/* node.variableDeclaration && */ isBindingPattern(node.variableDeclaration.name)) {
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

transformFlags |= TransformFlags.AssertES2015;
}

Expand Down
13 changes: 12 additions & 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 Expand Up @@ -20885,6 +20885,17 @@ namespace ts {
}
}
}
else if (/* !catchClause.variableDeclaration && */ languageVersion < ScriptTarget.ESNext) {
const blockLocals = catchClause.block.locals;
if (blockLocals) {
forEachKey(blockLocals, caughtName => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a name? We can generate a name in the transforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this block of change. Since we don't need check and error on a named variable anymore.

if (caughtName === "_ignoredCatchParameter") {
const localSymbol = blockLocals.get(caughtName);
grammarErrorOnNode(localSymbol.valueDeclaration, Diagnostics.Duplicate_identifier_ignoredCatchParameter_Compiler_uses_the_parameter_declaration_ignoredCatchParameter_to_bind_ignored_catched_exceptions);
}
});
}
}

checkBlock(catchClause.block);
}
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2196,6 +2196,10 @@
"category": "Error",
"code": 2713
},
"Duplicate identifier '_ignoredCatchParameter'. Compiler uses the parameter declaration '_ignoredCatchParameter' to bind ignored catched exceptions.": {
Copy link
Member

Choose a reason for hiding this comment

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

If you still need this:

Compiler uses the declaration '_ignoredCatchBinding` to bind ignored 'catch' clause parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

"category": "Error",
"code": 2714
},

"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
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
2 changes: 1 addition & 1 deletion src/compiler/transformers/es2015.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3173,7 +3173,7 @@ namespace ts {
function visitCatchClause(node: CatchClause): CatchClause {
const ancestorFacts = enterSubtree(HierarchyFacts.BlockScopeExcludes, HierarchyFacts.BlockScopeIncludes);
let updated: CatchClause;
if (isBindingPattern(node.variableDeclaration.name)) {
if (node.variableDeclaration && isBindingPattern(node.variableDeclaration.name)) {
Copy link
Member

Choose a reason for hiding this comment

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

By the time we get to the es2015 transform we must already have a variableDeclaration. Filling in the missing binding should be handled in the esnext transform. At best we should have a debug assertion.

Copy link
Member

@DanielRosenwasser DanielRosenwasser 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.

Agreed!

Debug.assert(!!node.variableDeclaration, "Catch clauses should always be present when downleveling ES2015 code.");

const temp = createTempVariable(/*recordTempVariable*/ undefined);
const newVariableDeclaration = createVariableDeclaration(temp);
setTextRange(newVariableDeclaration, node.variableDeclaration);
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("_ignoredCatchParameter"), node.block);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of createVariableDeclaration, we have a createUniqueName that you can call, which relieves you of having to write the error out. I think @RyanCavanaugh was just joking about reserving a variable name (but I'm not sure 😄 ).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you should either use createUniqueName or createTempVariable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added createTempVariable(undefined) here.

}
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 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.

What does this comment mean? This property is present to reduce costs when walking up the AST.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't apparent to me that the parent can be missing in a catch clause.

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 (_ignoredCatchParameter) { }
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 {}
}

10 changes: 9 additions & 1 deletion tests/baselines/reference/invalidTryStatements.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
tests/cases/conformance/statements/tryStatements/invalidTryStatements.ts(8,23): error TS1196: Catch clause variable cannot have a type annotation.
tests/cases/conformance/statements/tryStatements/invalidTryStatements.ts(9,23): error TS1196: Catch clause variable cannot have a type annotation.
tests/cases/conformance/statements/tryStatements/invalidTryStatements.ts(10,23): error TS1196: Catch clause variable cannot have a type annotation.
tests/cases/conformance/statements/tryStatements/invalidTryStatements.ts(14,13): error TS2714: Duplicate identifier '_ignoredCatchParameter'. Compiler uses the parameter declaration '_ignoredCatchParameter' to bind ignored catched exceptions.


==== tests/cases/conformance/statements/tryStatements/invalidTryStatements.ts (3 errors) ====
==== tests/cases/conformance/statements/tryStatements/invalidTryStatements.ts (4 errors) ====
function fn() {
try {
} catch (x) {
Expand All @@ -20,6 +21,13 @@ tests/cases/conformance/statements/tryStatements/invalidTryStatements.ts(10,23):
try { } catch (y: string) { }
~~~~~~
!!! error TS1196: Catch clause variable cannot have a type annotation.


try { } catch {
let _ignoredCatchParameter; // Should error since we downlevel emit this variable.
~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2714: Duplicate identifier '_ignoredCatchParameter'. Compiler uses the parameter declaration '_ignoredCatchParameter' to bind ignored catched exceptions.
}
}


9 changes: 9 additions & 0 deletions tests/baselines/reference/invalidTryStatements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ function fn() {
try { } catch (z: any) { }
try { } catch (a: number) { }
try { } catch (y: string) { }


try { } catch {
let _ignoredCatchParameter; // Should error since we downlevel emit this variable.
}
}


Expand All @@ -27,4 +32,8 @@ function fn() {
catch (a) { }
try { }
catch (y) { }
try { }
catch (_ignoredCatchParameter) {
var _ignoredCatchParameter = void 0; // Should error since we downlevel emit this variable.
}
}
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 (_ignoredCatchParameter) { }
try { }
catch (x) {
var x;
}
try { }
finally { }
try { }
catch (_ignoredCatchParameter) { }
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
}
5 changes: 5 additions & 0 deletions tests/cases/conformance/statements/tryStatements/invalidTryStatements.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,10 @@ function fn() {
try { } catch (z: any) { }
try { } catch (a: number) { }
try { } catch (y: string) { }


try { } catch {
let _ignoredCatchParameter; // Should error since we downlevel emit this variable.
}
}

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
Loading