Ignored catch parameter by tinganho · Pull Request #17517 · microsoft/TypeScript

tinganho · 2017-07-30T09:06:46Z

DanielRosenwasser · 2017-07-31T23:39:55Z

        "category": "Error",
        "code": 2713
    },
+    "Duplicate identifier '_ignoredCatchParameter'. Compiler uses the parameter declaration '_ignoredCatchParameter' to bind ignored catched exceptions.": {


If you still need this:

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

DanielRosenwasser · 2017-07-31T23:52:34Z


+        function visitCatchClause(node: CatchClause): CatchClause {
+            if (!node.variableDeclaration) {
+                return updateCatchClause(node, createVariableDeclaration("_ignoredCatchParameter"), node.block);


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 😄 ).

Yes, you should either use createUniqueName or createTempVariable.

Added createTempVariable(undefined) here.

DanielRosenwasser · 2017-07-31T23:54:15Z

Looks great other than the unique identifier change I suggested (which may lead you to remove some tests relating to reserving these identifiers).

I think @rbuckton may want to review the changes.

DanielRosenwasser · 2017-07-31T23:56:27Z

(Also, thanks for the PR @tinganho, long time no see! 😄)

rbuckton · 2017-08-01T00:15:37Z

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


The comment is unnecessary

rbuckton · 2017-08-01T00:17:21Z

+                else if (/* !catchClause.variableDeclaration && */ languageVersion < ScriptTarget.ESNext) {
+                    const blockLocals = catchClause.block.locals;
+                    if (blockLocals) {
+                        forEachKey(blockLocals, caughtName => {


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

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

rbuckton · 2017-08-01T00:20:38Z

            const ancestorFacts = enterSubtree(HierarchyFacts.BlockScopeExcludes, HierarchyFacts.BlockScopeIncludes);
            let updated: CatchClause;
-            if (isBindingPattern(node.variableDeclaration.name)) {
+            if (node.variableDeclaration && isBindingPattern(node.variableDeclaration.name)) {


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.

Agreed!

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

rbuckton · 2017-08-01T00:21:46Z


+        function visitCatchClause(node: CatchClause): CatchClause {
+            if (!node.variableDeclaration) {
+                return updateCatchClause(node, createVariableDeclaration("_ignoredCatchParameter"), node.block);


Yes, you should either use createUniqueName or createTempVariable.

rbuckton · 2017-08-01T00:22:50Z

        kind: SyntaxKind.CatchClause;
-        parent?: TryStatement;
-        variableDeclaration: VariableDeclaration;
+        parent?: TryStatement; // We parse missing try statements


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

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

DanielRosenwasser · 2017-08-01T00:42:54Z

Sounds like you want createTempVariable, not createUniqueName (unless you have useful text in mind, but I'd keep it as short as possible). Is that accurate @rbuckton?

rbuckton · 2017-08-01T01:11:42Z

Yes, specifically createTempVariable(undefined).

tinganho · 2017-08-01T20:20:43Z

(Thanks @DanielRosenwasser , I have been quite busy for a while, so I haven't have time to participate so much here. Though, working with TS projects all day 😄 )

tinganho · 2017-08-01T20:24:08Z

I now, removed the named variable ignoredCatchParameter, it made more sense to make the suggested temporary unique variable. So I cleaned up diagnostics, tests and some other unnecessary stuff.

weswigham

I also bemoan apparently having no coverage in the conformance suite for

try {} catch() {}

which is an error both before and after this change.

weswigham · 2017-08-01T23:06:37Z


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


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

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

tinganho · 2017-08-02T09:27:27Z

@weswigham I think I know why, it messes up checking with the lines after it:

tinganho · 2017-08-02T10:06:35Z

I updated the code with recursive visits.

rbuckton · 2017-08-02T17:13:43Z

+            if (node.variableDeclaration) {
+                writeToken(SyntaxKind.OpenParenToken, openParenPos);
+                emit(node.variableDeclaration);
+                writeToken(SyntaxKind.CloseParenToken, node.variableDeclaration ? node.variableDeclaration.end : openParenPos);


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

rbuckton · 2017-08-02T17:17:46Z

-            if (parseExpected(SyntaxKind.OpenParenToken)) {
+
+            if (parseOptional(SyntaxKind.OpenParenToken)) {
                result.variableDeclaration = parseVariableDeclaration();


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.

rbuckton · 2017-08-02T17:19:48Z

        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.");


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

rbuckton · 2017-08-02T17:21:39Z

        kind: SyntaxKind.CatchClause;
-        parent?: TryStatement;
-        variableDeclaration: VariableDeclaration;
+        parent?: TryStatement; // We make this optional to parse missing try statements


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.

rbuckton · 2017-08-02T17:23:30Z

-        parent?: TryStatement;
-        variableDeclaration: VariableDeclaration;
+        parent?: TryStatement; // We make this optional to parse missing try statements
+        variableDeclaration?: VariableDeclaration;


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.

rbuckton · 2017-08-02T17:31:05Z

@weswigham I think I know why, it messes up checking with the lines after it:

invalidTryStatements.ts covers grammar errors handled in the checker. invalidTryStatements2.ts covers parse errors. I think invalidTryStatements2.ts is a better place for try {} catch () {}.

tinganho · 2017-08-02T18:49:58Z

invalidTryStatements.ts covers grammar errors handled in the checker. invalidTryStatements2.ts covers parse errors. I think invalidTryStatements2.ts is a better place for try {} catch () {}.

I added both the test cases try { } and try { } catch() { } in invalidTryStatements2.ts.

…efined'.

rbuckton · 2017-08-08T23:14:48Z

Looks great! Thanks for the contribution!

tinganho · 2017-08-09T11:44:57Z

Thanks for the great review!

msftclas added the cla-already-signed label Jul 30, 2017

tinganho added 9 commits July 30, 2017 23:26

Clean up

b17bd97

Add diagnostics

99c662b

Adds optional catch parameter into the compiler

b917eb0

Adds transformers for ignored catch parameter

148a494

Add tests

e349943

Accept baselines

90ea10e

Removes valid test cases from invalid test case file

c65e5a1

remove debugger

de38ef7

adds test for try-catch-finally

58c8a2c

tinganho force-pushed the IgnoredCatchParameter branch from 4d29920 to 58c8a2c Compare July 30, 2017 21:27

DanielRosenwasser requested changes Jul 31, 2017

View reviewed changes

DanielRosenwasser requested review from rbuckton and weswigham July 31, 2017 23:54

rbuckton suggested changes Aug 1, 2017

View reviewed changes

Addresses CR feedback

81fb3f7

Accept baselines

dbdbb05

DanielRosenwasser approved these changes Aug 1, 2017

View reviewed changes

weswigham requested changes Aug 1, 2017

View reviewed changes

Fixes CR comments

1926285

Accept baseline

053b708

rbuckton suggested changes Aug 2, 2017

View reviewed changes

tinganho added 2 commits August 2, 2017 20:13

Addresses CR comment

d5c24f3

Adds missing test cases

f9e85ec

tinganho and others added 2 commits August 2, 2017 20:55

Typo

4f3e13a

Amend comment about explicitly setting catch clause variables to 'und…

3da1a53

…efined'.

weswigham approved these changes Aug 3, 2017

View reviewed changes

rbuckton approved these changes Aug 8, 2017

View reviewed changes

rbuckton merged commit 75c8ecb into microsoft:master Aug 8, 2017

microsoft locked and limited conversation to collaborators Jun 14, 2018

Conversation

tinganho commented Jul 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielRosenwasser commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Aug 1, 2017

Uh oh!

tinganho commented Aug 1, 2017

Uh oh!

tinganho commented Aug 1, 2017

Uh oh!

weswigham left a comment

Choose a reason for hiding this comment

Uh oh!

weswigham Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinganho commented Aug 2, 2017

Uh oh!

tinganho commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DanielRosenwasser commented Jul 31, 2017 •

edited

Loading

DanielRosenwasser commented Jul 31, 2017 •

edited

Loading

DanielRosenwasser Aug 1, 2017 •

edited

Loading

DanielRosenwasser commented Aug 1, 2017 •

edited

Loading

weswigham Aug 1, 2017 •

edited

Loading

tinganho commented Aug 2, 2017 •

edited

Loading