Disallow line terminator after arrow function parameters, before => #2283

caitp · 2015-03-10T03:05:19Z

caitp · 2015-03-10T03:05:52Z

quick attempt at a fix, some things are probably missing / test in wrong place / error messages not that helpful

DanielRosenwasser · 2015-03-10T04:24:11Z

src/compiler/parser.ts

@@ -3160,6 +3160,11 @@ module ts {
                return undefined;
            }

+            // Must be no line terminator before token `=>`.
+            if (scanner.hasPrecedingLineBreak()) {
+                return undefined;


Wow, that was quick! I started a bit while I was in the office, but if you'd like to take this task on I wouldn't mind.

I actually had the idea of continuing to parse arrow functions exactly as before, but to report the error as part of our grammatical checking in checker.ts. Since => blah(blah, blahblah) is not a valid expression in any context anyway, this should be acceptable, and it's ideal for language service scenarios to gracefully recover as well as possible. For this reason, I'm not sure if checking for a line break here (and above) is the right thing to do.

However, the problem I ran into when I tried to fix this was that it's a real pain to work on the token-level in the checker. So I wonder if the benefit of the idea is actually worth the cost. When I discussed with @CyrusNajmabadi, he seemed to favor the idea, but I'd like him to weigh in.

Correct. I would not do this here. I would detect and parse out the arrow function, but then report later that it isn't valid. This will help ensure that all the higher layers of the stack properly understand what's going on.

caitp · 2015-03-10T15:08:06Z

Could we get a few more expression-bodied tests?

We should be testing different parsing contexts as well, so kill two birds with one stone:

Certainly

However, the problem I ran into when I tried to fix this was that it's a real pain to work on the token-level in the checker. So I wonder if the benefit of the idea is actually worth the cost.

Maybe it would be fine to flag whether a newline occured before the => token (in the node itself), and emit an error for that in the checker

CyrusNajmabadi · 2015-03-10T19:53:12Z

I agree with @DanielRosenwasser We shoudl continue parsing these, but report an appropriate error (likely in the checker, when we do grammar checks) that the arrow function isn't legal.

caitp · 2015-03-10T21:13:56Z

I'm not sure how to do this "work with tokens" thing in the checker, the approach I've taken is to add a boolean field to a new interface, and flag the error during parsing, but only report it during checking. It seemed like the simplest approach.

CyrusNajmabadi · 2015-03-10T22:22:32Z

Just add the equalsGreaterThanToken to the ArrowFunction interface. You can see how we parse tokens into the tree with helpers like parseExpectedToken. (If we don't have an ArrowFunction interface, then make one). Thanks!

caitp · 2015-03-10T22:45:51Z

@CyrusNajmabadi how does the presence of the node in the interface make it easier to figure out if there was a line terminator or not? It looks like it could produce a better error message though (or at least the position of the error would be better). From a glance, I'd have to use getLineAndCharacterOfPosition(), and compare the computed line positions of the end of the parameters with the beginning of the arrow token. That seems like more work than is really needed. But, why not

CyrusNajmabadi · 2015-03-10T22:49:39Z

it doesn't necessarily make it easier. But it's much more consistent with how we do the rest of our nodes.

From a glance, I'd have to use getLineAndCharacterOfPosition(), and compare the computed line positions of the end of the parameters with the beginning of the arrow token. That seems like more work than is really needed. But, why not

That sounds right to me.

DanielRosenwasser · 2015-03-10T23:49:43Z

tests/baselines/reference/APISample_transform.js

@@ -584,6 +584,9 @@ declare module "typescript" {
        name?: Identifier;
        body: Block | Expression;
    }
+    interface ArrowFunctionExpression extends FunctionExpression {


I think this should just be extending from Expression and FunctionLikeDeclaration (not a FunctionExpression since it's not a PrimaryExpression nor does it have a name).

CyrusNajmabadi · 2015-03-11T02:02:11Z

Sorry. That was a misreading on my part. The change looks great. Feel free to merge at any time.

caitp · 2015-03-11T02:10:21Z

hmm, I think it was slightly wrong though... the bottom branch won't ever be taken because a node is always created. I'll fix that.

CyrusNajmabadi · 2015-03-11T20:02:44Z

src/compiler/parser.ts

            // have an opening brace, just in case we're in an error state.
-            if (parseExpected(SyntaxKind.EqualsGreaterThanToken) || token === SyntaxKind.OpenBraceToken) {
+            arrowFunction.equalsGreaterThanToken = parseExpectedToken(SyntaxKind.EqualsGreaterThanToken, false, Diagnostics._0_expected, "=>");


parseExpectedToken will always return a token of type EqualsGreaterThanToken. So the below check will always be true. I think what you want is:

if (token === SyntaxKind.EqualsGreaterThanToken || token === SyntaxKind.OpenBraceToken)) { node.equalsGreaterThanToken = parseExpectedToken(...); node.body = parseArrowFunctionExpressionBody(); } else { node.equalsGreaterThanToken = parseExpectedToken(...); node.body = parseIdentifier(); }

This will ensure the same behavior as before, and will ensure that we get appropriate error messages in the absence of a => token.

Also, add a /*namedParameter:*/ for the 'false' parameter.

or

var lastToken = token; node.equalsGreaterThanToken = parseExpectedToken(...); node.body = lastToken === SyntaxKind.EqualsGreaterThanToken || lastToken === SyntaxKind.OpenBraceToken ? parseArrowFunctionExpressionBody() : parseIdentifier();

caitp · 2015-03-11T20:47:15Z

done (I preferred the second approach). Sorry about the amended last commit, but it was a very minor change to use ternary instead of an if statement

CyrusNajmabadi · 2015-03-11T20:53:05Z

👍 Awesome job. Thanks!

DanielRosenwasser · 2015-03-13T03:30:04Z

src/compiler/checker.ts

+                var arrowFunction = <ArrowFunction>node;
+                var sourceFile = getSourceFileOfNode(node);
+                var equalsGreaterThanLine = getLineAndCharacterOfPosition(sourceFile, getTokenPosOfNode(arrowFunction.equalsGreaterThanToken, sourceFile)).line;
+                var parametersLine = getLineAndCharacterOfPosition(sourceFile, arrowFunction.parameters.end).line;


Swap parametersLine and equalsGreaterThanLine parameters; also use the ends of arrowFunction.equalsGreaterThanToken, since the full start and end are on the same line.

DanielRosenwasser · 2015-03-13T03:55:48Z

Glad I realized it before signing off, but this still has some issues. We'll inappropriately give you an error in the following (I add the first just to iterate on):

// Should not be valid.
var func = (a: number): number
    => a;

// Should be valid.
var funk = (a: number
    ) => a;

// Should be valid.
var funkier = (a: number)
    : number => a;

// Should be valid.
var funkiest = (a: number):
    number => a;

At least, I think the latter 3 should be valid. @JsonFreeman and @CyrusNajmabadi can weigh in. Let's add that to the tests.

As for a fix, this is a bit of a hack - check whether or not the full start of the arrow (just .start) is on the same line as its end (.end).

caitp · 2015-03-13T05:29:10Z

you're right, I didn't consider the case with a type annotation for the return value at all (although I think the second case should have worked, since I was using the "end" position for the parameters).

Anyways, yeah, the suggested fix solves that, and the extra tests with return value types are good.

JsonFreeman · 2015-03-13T20:01:21Z

Yes, @DanielRosenwasser is correct, those latter 3 should be valid. funk is allowed per the ES6 grammar, and the other two can be allowed by our grammar if we put No LineTerminator Here after the type annotation.

DanielRosenwasser · 2015-03-14T22:38:03Z

src/compiler/checker.ts

+        function checkGrammarArrowFunction(node: FunctionLikeDeclaration): boolean {
+            if (node.kind === SyntaxKind.ArrowFunction) {
+                var arrowFunction = <ArrowFunction>node;
+                var sourceFile = getSourceFileOfNode(node);


I actually recall one of the other functions needing the sourceFile as well. Could we grab the sourceFile once in checkGrammarFunctionLikeDeclaration and pass it in as a parameter to these checkers?

DanielRosenwasser · 2015-03-14T22:40:13Z

I've made one last suggestion. After dealing with that and a merge conflict, I think this should be good to go.

Closes microsoft#2282

@DanielRosenwasser

As suggested by @DanielRosenwasser

Restores functionality broken in previous commit

caitp · 2015-03-15T00:14:15Z

done

DanielRosenwasser · 2015-03-16T01:57:37Z

src/compiler/parser.ts

@@ -95,6 +95,7 @@ module ts {
                    visitNodes(cbNodes, (<FunctionLikeDeclaration>node).typeParameters) ||
                    visitNodes(cbNodes, (<FunctionLikeDeclaration>node).parameters) ||
                    visitNode(cbNode, (<FunctionLikeDeclaration>node).type) ||
+                    visitNode(cbNode, (<ArrowFunction>node).equalsGreaterThanToken) ||


This is weird, but I don't mind it terribly.

DanielRosenwasser · 2015-03-16T01:59:45Z

This looks great, thanks for the PR @caitp!

Disallow line terminator after arrow function parameters, before =>

msftclas added the cla-not-required label Mar 10, 2015

DanielRosenwasser reviewed Mar 10, 2015
View reviewed changes

caitp force-pushed the issue-2282 branch from 320f2f7 to 36bc00d Compare March 10, 2015 15:05

caitp force-pushed the issue-2282 branch 2 times, most recently from 530f8f9 to 447ed96 Compare March 10, 2015 21:24

DanielRosenwasser reviewed Mar 10, 2015
View reviewed changes

CyrusNajmabadi reviewed Mar 11, 2015
View reviewed changes

caitp force-pushed the issue-2282 branch from c109014 to ef20ccb Compare March 11, 2015 20:45

DanielRosenwasser reviewed Mar 13, 2015
View reviewed changes

caitp force-pushed the issue-2282 branch 2 times, most recently from 44766b4 to 25e3a95 Compare March 13, 2015 06:34

DanielRosenwasser reviewed Mar 14, 2015
View reviewed changes

caitp added 10 commits March 14, 2015 19:50

Disallow line terminator after arrow function parameters, before =>

84634ac

Closes microsoft#2282

Perform error reporting in checker

dd16fed

Add additional test-cases for arrow function grammar

231f522

As suggested by @DanielRosenwasser

Check that arrow is on same line as parameters

aa3cefb

Fix line wrapping

fdc673f

Address slew of review comments

5e107e6

Parse arrow function body as identifier if missing => or {

bd828e3

Restores functionality broken in previous commit

Restore earlier behaviour when parsing non-simple arrow function bodies

3dc5faf

Make sure arrow function grammar rules can deal with type annotations

10925c1

Share SourceFile with other grammar checker that needs it

02d3568

caitp force-pushed the issue-2282 branch from 25e3a95 to 02d3568 Compare March 15, 2015 00:13

DanielRosenwasser reviewed Mar 16, 2015
View reviewed changes

DanielRosenwasser added a commit that referenced this pull request Mar 16, 2015

Merge pull request #2283 from caitp/issue-2282

ad98fad

Disallow line terminator after arrow function parameters, before =>

DanielRosenwasser merged commit ad98fad into microsoft:master Mar 16, 2015

microsoft locked and limited conversation to collaborators Jun 18, 2018

Disallow line terminator after arrow function parameters, before => #2283

Disallow line terminator after arrow function parameters, before => #2283

Uh oh!

Conversation

caitp commented Mar 10, 2015

Uh oh!

caitp commented Mar 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caitp commented Mar 10, 2015

Uh oh!

CyrusNajmabadi commented Mar 10, 2015

Uh oh!

caitp commented Mar 10, 2015

Uh oh!

CyrusNajmabadi commented Mar 10, 2015

Uh oh!

caitp commented Mar 10, 2015

Uh oh!

CyrusNajmabadi commented Mar 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Mar 11, 2015

Uh oh!

caitp commented Mar 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caitp commented Mar 11, 2015

Uh oh!

CyrusNajmabadi commented Mar 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Mar 13, 2015

Uh oh!

caitp commented Mar 13, 2015

Uh oh!

JsonFreeman commented Mar 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Mar 14, 2015

Uh oh!

caitp commented Mar 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Mar 16, 2015

Uh oh!

Uh oh!