Fix #5046: Adjacent JSX #5049

zdenko · 2018-05-02T13:10:02Z

Fixes the issue in previous PR (#5046).
In Block::makeReturn, the last two expressions are checked before the main loop starts.
Instead of unwrapAll(), I used unwrap() to avoid false errors with foo <a></a> and when explicit return is used.

<a></a>
foo <b></b>
###
<a></a>;
foo(<b></b>);
###

render = ->
  <a></a>
  return <b></b>
###
render = function() {
  <a></a>;
  return <b></b>;
};
###

GeoffreyBooth · 2018-05-02T16:29:55Z

What about this case?

render = -> (
  <Row>ABC</Row>
  <Row>DEF</Row>
)

The parentheses get removed as redundant, and then <Row>DEF</Row> gets returned. That’s clearly not the user’s intent, and Babel throws an error on what I would think is the equivalent of what the user had in mind:

var render = function() {
  return (
    <Row>ABC</Row>;
    <Row>DEF</Row>;
  )
}

Curiously though, this is the error:

repl: Unexpected token, expected , (3:18)
  1 | var render = () => {
  2 |   return (
> 3 |     <Row>ABC</Row>;
    |                   ^
  4 |     <Row>DEF</Row>;

GeoffreyBooth · 2018-05-02T16:31:40Z

Also, this:

render = ->
  <Row>ABC</Row>
  <Row>DEF</Row>

should compile, as it would become this:

var render = function() {
  <Row>ABC</Row>;
  return <Row>DEF</Row>;
}

which is valid JSX.

zdenko · 2018-05-02T16:44:59Z

User from #5034 wants this

JSXFunction = -> (
  <Row>ABC</Row>
  <Row>DEF</Row>
)

to compile into this

JSXFunction = function() {
  return <Row>ABC</Row>
     <Row>DEF</Row>;
};

But, this is an error. Your PR solved this, but was to greedy and was throwing and error even if return was explicit, e.g.

render = -> (
  <Row>ABC</Row>
  return <Row>DEF</Row>
)

My PR only corrects greediness.

zdenko · 2018-05-02T16:48:05Z

Besides, this

render = ->
  <Row>ABC</Row>
  <Row>DEF</Row>

already compiles on the master

render = function() {
  <Row>ABC</Row>;
  return <Row>DEF</Row>;
};

GeoffreyBooth · 2018-05-02T16:52:45Z

The non-parentheses version compiles today in 2.3.0, but my PR made it break. It should compile, because it’s valid JSX.

zdenko · 2018-05-02T17:02:31Z

OK, So only JSX in parentheses should throw an error?

render = -> (
  <Row>ABC</Row>
  <Row>DEF</Row>
)

Since CS removes parentheses in any case, JSX will get special treatment.

And, what about this case?

render = -> (
  a += 1
  <Row>ABC</Row>
  <Row>DEF</Row>
)

I guess we could inspect expressions in the parentheses and only if JSX is present we would throw an error.

GeoffreyBooth · 2018-05-02T17:12:08Z

I’m not sure what to do. I just want to make sure we at least don’t throw errors on valid JSX.

zdenko · 2018-05-02T17:29:51Z

The only way I see is checking parentheses.
And, we can treat any expression inside the same way as Babel does.

zdenko · 2018-05-02T20:49:08Z

The last commit fixes cases discussed above.

Valid CSX:

render = ->
  <Row>ABC</Row>
  <Row>DEF</Row>

###
render = function() {
  <Row>ABC</Row>;
  return <Row>DEF</Row>;
};
###

<a></a>
foo <b></b>

###
<a></a>;
foo(<b></b>);
###

Compiler throws an error when adjacent CSX in parentheses is not wrapped in an enclosing tag.

render = -> (
  <Row>ABC</Row>
  <Row>DEF</Row>
)

###
error: Adjacent JSX elements must be wrapped in an enclosing tag
  <Row>DEF</Row>
   ^^^^^^^^^^^^^
###

Only last two expressions are checked.

render = -> (
  a = 2
  <Row>ABC</Row>
  <Row>DEF</Row>
)

###
error: Adjacent JSX elements must be wrapped in an enclosing tag
  <Row>DEF</Row>
   ^^^^^^^^^^^^^
###

This will compile.

render = -> (
  <Row>ABC</Row>
  a = 2
  <Row>DEF</Row>
)

###
render = function() {
  var a;
  <Row>ABC</Row>;
  a = 2;
  return <Row>DEF</Row>;
};
###

GeoffreyBooth

LTGM. @vendethiel?

vendethiel · 2018-05-02T21:48:00Z

I think even the non-parenthesized version should break. It will never be what the user meant, Afaik. The way CoffeeScript works makes this a bit ambiguous maybe... I think we had a similar thing with

->
  a: 1
  b: 2

zdenko · 2018-05-02T22:15:53Z

It will never be what the user mean

Agree. Even the user from #5034 expected something else.
The question is how to deal with mixed expressions:

render = ->
   x = 1
   y = 2
  <Row>ABC</Row>
  <Row>DEF</Row>

I think in such cases we should return the last expression, e.g., no error.

GeoffreyBooth · 2018-05-07T05:47:21Z

So what else needs to be done to wrap up this PR?

zdenko · 2018-05-07T11:46:14Z

As per @vendethiel's suggestion, I would treat

render = ->
  <Row>ABC</Row>
  <Row>DEF</Row>

in the same way as

render = -> (
  <Row>ABC</Row>
  <Row>DEF</Row>
)

However, this case would return the last expressions

render = ->
   x = 1
   y = 2
   <Row>ABC</Row>
   <Row>DEF</Row>

###
render = function() {
  var x, y;
  x = 1;
  y = 2;
  <Row>ABC</Row>;
  return <Row>DEF</Row>;
};
###

GeoffreyBooth · 2018-05-07T17:01:54Z

Isn’t that the current behavior before we started changing things?

zdenko · 2018-05-07T19:13:59Z

@GeoffreyBooth it was, but then your PR changed it and in this PR you commented that non-parenthesized JSX should compile.

GeoffreyBooth · 2018-05-07T22:03:47Z

Right, so what's the difference between this PR and reverting mine?

vendethiel · 2018-05-07T22:15:55Z

I think in such cases we should return the last expression, e.g., no error.

Why? I don't think that makes sense.

Inve1951 · 2018-05-08T12:51:15Z

@vendethiel It's valid coffee/js/jsx code and coffee implicitely returns the last statement of a function body. JSX tags are still only js objects. <Row>ABC</Row> is same as React.createElement(Row, null, "ABC").

I don't see the need for special treatment either but it's true that it's unlikely that the user ment for the code to turn out that way.
I see 2 solutions:

Ignore it and have ESlint deal with it.
Implicitely wrap the JSX tags into Fragments.

zdenko · 2018-05-08T13:35:56Z

@GeoffreyBooth

->
  <a></a>
  foo
  <b></b>

Your PR:

[stdin]:5:4: error: Adjacent JSX elements must be wrapped in an enclosing tag
  <b></b>
   ^^^^^^

This PR:

(function() {
  <a></a>;
  foo;
  return <b></b>;
});

GeoffreyBooth · 2018-05-08T14:00:48Z

@zdenko I get that, what I mean is, what's the difference between current 2.3.0 (a.k.a. reverting my PR to the prior state of master) and merging this PR in?

vendethiel · 2018-05-08T14:52:17Z

@vendethiel It's valid coffee/js/jsx code and coffee implicitely returns the last statement of a function body. JSX tags are still only js objects. ABC is same as React.createElement(Row, null, "ABC").

In general I'd agree -- that's the role of a linter. But here we're just copying an error that's reported by babel; because the error is lost when compiling.

GeoffreyBooth · 2018-05-08T14:57:15Z

The error isn’t lost; there’s a difference between

var render = function() {
  return (
    <Row>ABC</Row>
    <Row>DEF</Row>
  )
}

and

var render = function() {
  return (
    <Row>ABC</Row>;
    <Row>DEF</Row>;
  )
}

The version without the semicolons throws the Babel error, while the other doesn’t. Since CoffeeScript always adds the semicolons for each new line, we shouldn’t ever get that error.

zdenko · 2018-05-08T16:09:17Z

I get an error on Babel for

var render = function() {
  return (
    <Row>ABC</Row>;
    <Row>DEF</Row>;
  )
}

This works

var render = function() {
  return (
    <Row>ABC</Row>,
    <Row>DEF</Row>
  )
}

If we revert your PR and merge this one, we'll (currently) get an error only if JSX in enclosed in parentheses.

-> (
  <a></a>
  <b></b>
)
###
[stdin]:4:4: error: Adjacent JSX elements must be wrapped in an enclosing tag
  <b></b>
   ^^^^^^
###

Non-parenthesized JSX will compile:

->
  <a></a>
  <b></b>
###
(function() {
  <a></a>;
  return <b></b>;
});
###

GeoffreyBooth · 2018-05-13T00:50:45Z

Okay, so regarding what would trigger an error in this PR:

-> (
  <a></a>
  <b></b>
)

That’s equivalent to this JS:

(function() {
  return (
    <a></a>;
    <b></b>;
  );
});

Which throws this error in Babel:

repl: Unexpected token, expected , (3:11)
  1 | (function() {
  2 |   return (
> 3 |     <a></a>;
    |            ^

Which isn’t really equivalent to “Adjacent JSX elements must be wrapped in an enclosing tag.” Do we care?

vendethiel · 2018-05-13T00:55:29Z

It’s not equivalent because of our everything-is-an-expression semantics. I think we do care.

GeoffreyBooth · 2018-05-13T01:16:24Z

Maybe “do we care” is a bit harsh 😄 I guess the question is what we consider

(
  <a />
  <b />
)

to be equivalent to, since it is an expression.

Some more ways of putting it, in JS, with the errors from Babel:

var someJsx = (
  <a />
  <b />
);

repl: Adjacent JSX elements must be wrapped in an enclosing tag (3:2)
  1 | var someJsx = (
  2 |   <a />
> 3 |   <b />
    |   ^

and

var someJsx = (
  <a />;
  <b />;
);

repl: Unexpected token, expected , (2:7)
  1 | var someJsx = (
> 2 |   <a />;
    |        ^

and finally,

var someJsx = (
  <a />,
  <b />
);

compiles successfully, into

var someJsx = (React.createElement("a", null), React.createElement("b", null));

So in CoffeeScript, each newline triggers a semicolon; and there doesn’t seem to be an equivalent for the comma operator or a way to trigger its output. (This is worth reading.)

I think Babel’s expected , error, asking for the comma operator, doesn’t make sense for CoffeeScript to output since we don’t have that operator (I think). And I think Babel is only asking for that because asking for a semicolon is pointless here, since the semicolon version throws a different error. Really though I think Babel asking for a comma is misguided, since I don’t see the sense in using that operator here. All it means in this case, to use the last example, is that someJsx gets the value of React.createElement("b", null), which doesn’t seem terribly useful. I think the Adjacent JSX elements must be wrapped error is actually more informative, as it’s closer to the source of the problem. @vendethiel what do you think?

vendethiel · 2018-05-13T10:24:27Z

We do have the comma operator, just spelled ;: return (1;2).
Babel doesn’t have such an ambiguity because it only has ASI, no conflating of statements and expressions.

For us, (1; 2) is like a block, we allow pretty much all our constructions there (if, while, for, ... everything that’s not a jump). Which is why i think we should treat it as adjacent JSX and warn about it, even wrapped in parens.

GeoffreyBooth · 2018-05-13T18:21:19Z

So @vendethiel what’s the bottom line here? Can we merge in this PR as is, or does it need any further changes?

vendethiel · 2018-05-13T18:48:55Z

test/error_messages.coffee

@@ -1664,25 +1664,26 @@ test 'CSX error: invalid attributes', ->

 test '#5034: CSX error: Adjacent JSX elements must be wrapped in an enclosing tag', ->
  assertErrorFormat '''
-    render = ->
+    render = -> (


I think we should test that it errors out both with and without parens

But the without-parens version is valid JSX:

-> <a /> 

(function() { <a />; return ; });

(function () { React.createElement("a", null); return React.createElement("b", null); });

I agree it’s likely a mistake on the user’s part, but catching “most of the time this is wrong” errors is the job of a linter, not the compiler.

I talked about this a bit earlier.
You could say that, but you could also say that

-> <a /> 

really is

-> return ( <a /> )

just like

-> a: 1 b: 2

is

-> return { a: 1 b: 2 }

instead of

-> {a: 1} return {b: 2}

I don’t think you could say that parentheses are implied. JSX tags on subsequent lines aren’t assumed to be part of a unified construct the way an object is. They don’t compile into a single entity, for one thing, even with parentheses:

jsx = ( <a /> )

jsx = (<a />, );

That’s not the way objects behave. The separate keys of an object are unified by virtue of being all part of the same block, as defined by indentation. Adjacent JSX tags are just that, adjacent JSX tags, unless they’re enclosed by a parent tag or fragment.

OK. I also want to point out that this:

I agree it’s likely a mistake on the user’s part, but catching “most of the time this is wrong” errors is the job of a linter, not the compiler.

Could very well apply to the original warning. Babel could compile it to two createElement doing nothing.

Could very well apply to the original warning. Babel could compile it to two createElement doing nothing.

It could, but the JSX spec tells it not to. I think that's the distinction.

GeoffreyBooth · 2018-05-14T02:53:01Z

test/error_messages.coffee

      <Row>a</Row>
      <Row>b</Row>
    )
  ''', '''
-    [stdin]:3:4: error: Adjacent JSX elements must be wrapped in an enclosing tag
+    [stdin]:4:4: error: Adjacent JSX elements must be wrapped in an enclosing tag
      <Row>b</Row>
       ^^^^^^^^^^^


@zdenko We should also add a test for the non-error case:

-> <a /> 

That would be a good place to put a comment with a link to this PR, and an explanation for why this shouldn’t error.

# Conflicts: # test/csx.coffee

GeoffreyBooth · 2018-05-19T05:38:05Z

@zdenko @vendethiel I added the last test that I had requested. I’m eager to wrap this up. Any more notes?

zdenko · 2018-05-20T15:28:30Z

@GeoffreyBooth LGTM

Fix jashkenas#5046: Adjacent JSX

ba01407

zdenko mentioned this pull request May 2, 2018

Bug: Last JSX tag returned, when compiler should throw error #5034

Closed

check CSX only when wrapped in parentheses

09465f7

GeoffreyBooth approved these changes May 2, 2018

View reviewed changes

vendethiel reviewed May 13, 2018

View reviewed changes

GeoffreyBooth requested changes May 14, 2018

View reviewed changes

GeoffreyBooth added 3 commits May 18, 2018 22:33

Fix indentation

e33f032

Add test for unlikely, but valid, JSX syntax

89405a1

Merge branch 'master' into jsx

22cd4e3

# Conflicts: # test/csx.coffee

GeoffreyBooth approved these changes May 19, 2018

View reviewed changes

GeoffreyBooth merged commit 0e7677a into jashkenas:master May 20, 2018

GeoffreyBooth mentioned this pull request May 20, 2018

2.3.1 #5069

Merged

Fix #5046: Adjacent JSX #5049

Fix #5046: Adjacent JSX #5049

Conversation

zdenko commented May 2, 2018

GeoffreyBooth commented May 2, 2018

GeoffreyBooth commented May 2, 2018

zdenko commented May 2, 2018

zdenko commented May 2, 2018

GeoffreyBooth commented May 2, 2018

zdenko commented May 2, 2018

GeoffreyBooth commented May 2, 2018

zdenko commented May 2, 2018

zdenko commented May 2, 2018

GeoffreyBooth left a comment

Choose a reason for hiding this comment

vendethiel commented May 2, 2018

zdenko commented May 2, 2018

GeoffreyBooth commented May 7, 2018

zdenko commented May 7, 2018

GeoffreyBooth commented May 7, 2018

zdenko commented May 7, 2018

GeoffreyBooth commented May 7, 2018

vendethiel commented May 7, 2018

Inve1951 commented May 8, 2018

zdenko commented May 8, 2018

GeoffreyBooth commented May 8, 2018 • edited Loading

vendethiel commented May 8, 2018 • edited Loading

GeoffreyBooth commented May 8, 2018

zdenko commented May 8, 2018

GeoffreyBooth commented May 13, 2018 • edited Loading

vendethiel commented May 13, 2018

GeoffreyBooth commented May 13, 2018

vendethiel commented May 13, 2018

GeoffreyBooth commented May 13, 2018

vendethiel May 13, 2018

Choose a reason for hiding this comment

GeoffreyBooth May 13, 2018

Choose a reason for hiding this comment

vendethiel May 13, 2018

Choose a reason for hiding this comment

GeoffreyBooth May 13, 2018 • edited Loading

Choose a reason for hiding this comment

vendethiel May 13, 2018

Choose a reason for hiding this comment

GeoffreyBooth May 13, 2018

Choose a reason for hiding this comment

GeoffreyBooth May 14, 2018

Choose a reason for hiding this comment

GeoffreyBooth commented May 19, 2018

zdenko commented May 20, 2018

GeoffreyBooth commented May 8, 2018 •

edited

Loading

vendethiel commented May 8, 2018 •

edited

Loading

GeoffreyBooth commented May 13, 2018 •

edited

Loading

GeoffreyBooth May 13, 2018 •

edited

Loading