no-semi #1129

Merged
merged 1 commit into from Apr 11, 2017

Conversation

Projects
None yet
7 participants
@rattrayalex
Collaborator

rattrayalex commented Apr 5, 2017

Addresses #736

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 5, 2017

Collaborator

cc @bakkot

Collaborator

vjeux commented Apr 5, 2017

cc @bakkot

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Apr 5, 2017

Collaborator

More interesting cases:

while (false)
  (function(){}()) // definitely do not start this line with a semicolon
aReallyLongLine012345678901234567890123456789012345678901234567890123456789 *
  (b + c) // or this one
a;
/b/g.test('b') // you have a test with a regex, but in yours omitting the semicolon would be a parse error instead of a different AST. Having the flag makes a difference.
a; // Necessary semicolon
::b.c
class A {
  async; // The semicolon is *not* necessary, because of a '[no LineTerminator here]' restriction
  x(){}
}
class A {
  static; // The semicolon *is* necessary
  x(){}
}

There's probably some involving flow and JSX too, but I'm not as familiar with their grammars.

Collaborator

bakkot commented Apr 5, 2017

More interesting cases:

while (false)
  (function(){}()) // definitely do not start this line with a semicolon
aReallyLongLine012345678901234567890123456789012345678901234567890123456789 *
  (b + c) // or this one
a;
/b/g.test('b') // you have a test with a regex, but in yours omitting the semicolon would be a parse error instead of a different AST. Having the flag makes a difference.
a; // Necessary semicolon
::b.c
class A {
  async; // The semicolon is *not* necessary, because of a '[no LineTerminator here]' restriction
  x(){}
}
class A {
  static; // The semicolon *is* necessary
  x(){}
}

There's probably some involving flow and JSX too, but I'm not as familiar with their grammars.

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 5, 2017

Collaborator

Nice, thanks @bakkot !

Collaborator

rattrayalex commented Apr 5, 2017

Nice, thanks @bakkot !

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 5, 2017

Collaborator

@bakkot – FYI, this one doesn't parse, so not including it

class A {
  static; 
  x(){}
}
Collaborator

rattrayalex commented Apr 5, 2017

@bakkot – FYI, this one doesn't parse, so not including it

class A {
  static; 
  x(){}
}
@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Apr 5, 2017

Collaborator

@rattrayalex Does too :)

You're just using a version of babylon which is behind master.

Collaborator

bakkot commented Apr 5, 2017

@rattrayalex Does too :)

You're just using a version of babylon which is behind master.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 5, 2017

Collaborator

Doesn't seem to parse on Chrome

image

Collaborator

vjeux commented Apr 5, 2017

Doesn't seem to parse on Chrome

image

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Apr 5, 2017

Collaborator

@vjeux Class properties aren't yet part of JS; it's an experimental feature. You'll notice that class Y { foo; } doesn't parse either.

Collaborator

bakkot commented Apr 5, 2017

@vjeux Class properties aren't yet part of JS; it's an experimental feature. You'll notice that class Y { foo; } doesn't parse either.

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

Hah! Nice 😄
I'll add a commented-out version with a TODO for now then.

Collaborator

rattrayalex commented Apr 6, 2017

Hah! Nice 😄
I'll add a commented-out version with a TODO for now then.

+
+exports[`no-semi.js 1`] = `
+"
+// the \`if (true)\` isn't necessary; just wanted to use a block statement

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

you can use class x {} if you want something that fits in one line btw.

@vjeux

vjeux Apr 6, 2017

Collaborator

you can use class x {} if you want something that fits in one line btw.

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

@bakkot my hunch is that the best way to handle

class A {
  static; 
  x(){}
}

will be by preserving the ; after the static – does that sound right to you? Would be a break from how we do the other stuff.

(I'll probably kick the can down the road on that particular issue, unless you sneak in a babylon upgrade before I merge 😉 )

Collaborator

rattrayalex commented Apr 6, 2017

@bakkot my hunch is that the best way to handle

class A {
  static; 
  x(){}
}

will be by preserving the ; after the static – does that sound right to you? Would be a break from how we do the other stuff.

(I'll probably kick the can down the road on that particular issue, unless you sneak in a babylon upgrade before I merge 😉 )

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 6, 2017

Collaborator

The static/get is such an edge case that it's totally fine to keep it at the end of the line. I doubt anyone will ever write code like this, and if that person does, then they'll be positively surprised that we didn't break their code :p

Collaborator

vjeux commented Apr 6, 2017

The static/get is such an edge case that it's totally fine to keep it at the end of the line. I doubt anyone will ever write code like this, and if that person does, then they'll be positively surprised that we didn't break their code :p

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

@bakkot care to lend some 👀 on test cases again? Apologies if I missed something you previously mentioned

Collaborator

rattrayalex commented Apr 6, 2017

@bakkot care to lend some 👀 on test cases again? Apologies if I missed something you previously mentioned

src/printer.js
+ return exprNeedsASIProtection(node.expression);
+}
+
+function classStmtNeedsASIProtection(path) {

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

you can kill it now

@vjeux

vjeux Apr 6, 2017

Collaborator

you can kill it now

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 6, 2017

Collaborator

@lydell would you have time to run your fuzzer on this PR (or let us know how to do it)?

Collaborator

vjeux commented Apr 6, 2017

@lydell would you have time to run your fuzzer on this PR (or let us know how to do it)?

src/printer.js
@@ -1941,7 +1948,7 @@ function genericPrintNoParens(path, options, print) {
}
}
-function printStatementSequence(path, options, print) {
+function printStatementSequence(path, options, print, isClass) {

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

While it's technically "more correct", I think I'm going to get rid of this isClass thing because it's not necessary and stinks a bit

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

While it's technically "more correct", I think I'm going to get rid of this isClass thing because it's not necessary and stinks a bit

src/printer.js
+}
+
+function exprNeedsASIProtection(node) {
+ const maybeASIProblem = node.needsParens ||

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

This is where needsParens is used.

exprNeedsASIProtection is recursive, and recurses on the basis of the ast nodes via getLeftSide, just above. So it'll be a bit of a pain to move to using path.needsParens(), in addition to being slower.

But this is pretty ugly.

Thoughts?

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

This is where needsParens is used.

exprNeedsASIProtection is recursive, and recurses on the basis of the ast nodes via getLeftSide, just above. So it'll be a bit of a pain to move to using path.needsParens(), in addition to being slower.

But this is pretty ugly.

Thoughts?

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

@vjeux any thoughts on this?

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

@vjeux any thoughts on this?

@rattrayalex rattrayalex changed the title from WIP - no-semi tests to no-semi Apr 6, 2017

README.md
+ parser: 'babylon',
+
+ // Whether to use a semicolon-free style, similar to eslint's semi: never
+ noSemi: false

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

can you name this option semi and default to true. The option parser library will make --no-semi work

@vjeux

vjeux Apr 6, 2017

Collaborator

can you name this option semi and default to true. The option parser library will make --no-semi work

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

sure!

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

sure!

src/printer.js
@@ -2342,6 +2354,7 @@ function typeIsFunction(type) {
function printExportDeclaration(path, options, print) {
const decl = path.getValue();
+ const semi = options.noSemi ? "" : ";"

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

nit: two spaces, and missing semi :p But feel free to leave it as is, it'll go away when we reformat it again :)

@vjeux

vjeux Apr 6, 2017

Collaborator

nit: two spaces, and missing semi :p But feel free to leave it as is, it'll go away when we reformat it again :)

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

yeah, looks like the codebase is overdue for a run of npm run format:all, I see loads of changes (not all of them look great? what's with the removal of all the newlines?)

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

yeah, looks like the codebase is overdue for a run of npm run format:all, I see loads of changes (not all of them look great? what's with the removal of all the newlines?)

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

We used to do format:all after each release since the number of PRs was near 0 but we skipped doing that during the last one and had a bunch of changes. It would be greatly needed indeed!

@vjeux

vjeux Apr 6, 2017

Collaborator

We used to do format:all after each release since the number of PRs was near 0 but we skipped doing that during the last one and had a bunch of changes. It would be greatly needed indeed!

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

This looks very weird indeed for the removal of newlines. May be related to #1136

@vjeux

vjeux Apr 6, 2017

Collaborator

This looks very weird indeed for the removal of newlines. May be related to #1136

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

yeah, that makes sense. Want me to open an issue?

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

yeah, that makes sense. Want me to open an issue?

+ ;[\\"m\\"]() {
+ 1
+ }
+ ;*g() {}

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

this one is unfortunate, but I guess generator functions are not widely used

@vjeux

vjeux Apr 6, 2017

Collaborator

this one is unfortunate, but I guess generator functions are not widely used

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

yeah, it's ugly, but needed;

class X {
  a = 1
  *g() {}
}

throws a syntax error.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

yeah, it's ugly, but needed;

class X {
  a = 1
  *g() {}
}

throws a syntax error.

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator
class X {
  f() {}
  *g() {}
}

doesn't though, which is pretty common. But yeah, given where we put semi, I think it's okay to put it there.

@vjeux

vjeux Apr 6, 2017

Collaborator
class X {
  f() {}
  *g() {}
}

doesn't though, which is pretty common. But yeah, given where we put semi, I think it's okay to put it there.

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

Hmm, I kind of think it'd be worthwhile to break the "just put a ; at the start of every line" rule for class methods (especially since the universe of possibilities is far smaller).

It wouldn't be too hard to check the preceding node, and only print ; if it's not a method.

@bakkot @lydell what do you think?

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

Hmm, I kind of think it'd be worthwhile to break the "just put a ; at the start of every line" rule for class methods (especially since the universe of possibilities is far smaller).

It wouldn't be too hard to check the preceding node, and only print ; if it's not a method.

@bakkot @lydell what do you think?

This comment has been minimized.

@lydell

lydell Apr 6, 2017

Collaborator

I didn't even know semicolons where allowed there! So 👍 for "breaking the rule" (which was only very loosely defined in the first place anyway) whenever it makes sense.

@lydell

lydell Apr 6, 2017

Collaborator

I didn't even know semicolons where allowed there! So 👍 for "breaking the rule" (which was only very loosely defined in the first place anyway) whenever it makes sense.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Apr 6, 2017

Collaborator

I continue to find the no-semi style abhorrent, so you shouldn't ask my opinion on where it should put semicolons. Maybe ask someone who uses the style?

In particular, I think this snippet captures almost of the oddities of using no-semi with class properties; suggest collecting opinions from no-semi people on how it should be formatted.

class A {
  a = 0
  ;[b](){}

  c = 0
  ;*d(){}

  static
  ;e(){}

  get
  ;f(){}

  // none of the semicolons above this comment can be omitted. none of the semicolons below it are necessary.

  ;[h](){}

  ;*i(){}

  async
  ;j(){}
}

cc @kentcdodds in case he has opinions.

Collaborator

bakkot commented Apr 6, 2017

I continue to find the no-semi style abhorrent, so you shouldn't ask my opinion on where it should put semicolons. Maybe ask someone who uses the style?

In particular, I think this snippet captures almost of the oddities of using no-semi with class properties; suggest collecting opinions from no-semi people on how it should be formatted.

class A {
  a = 0
  ;[b](){}

  c = 0
  ;*d(){}

  static
  ;e(){}

  get
  ;f(){}

  // none of the semicolons above this comment can be omitted. none of the semicolons below it are necessary.

  ;[h](){}

  ;*i(){}

  async
  ;j(){}
}

cc @kentcdodds in case he has opinions.

@lydell

This comment has been minimized.

Show comment
Hide comment
@lydell

lydell Apr 6, 2017

Collaborator

For this one:

class A {
  static; 
  x(){}
}

... I think it is totally fine put the semicolon at the end of the static line instead of at the start of the x(){} line. It's an edge case that I wouldn't expect to find anywhere in real code bases, so, to me, the printing doesn't really matter as long as the program is correct.


Regarding the fuzzer. I might get the time to run it in a few days. But I'll tell you how I run it anyway!

  1. Clone https://github.com/lydell/eslump.

  2. Edit examples/prettier.js:

    • I've been changing it to only use babylon lately (instead of randomly picking between flow and babylon), because the fuzzer likes to generate the edge cases that flow doesn't support. Not sure if newer flow versions work better.
    • Make sure that the version of prettier that you want to test is require()d.
  3. Run ./cli.js ./examples/prettier.js output/. You can also pass the --comments and --whitespace flags to also randomize comments and whitespace. I'd start without them, then add only --comments, then add only --whitespace and finally add both.

  4. When the fuzzer finds an error, I press the Up arrow in the terminal, to edit the last command and add the -r flag (short for --reproduce) and run again. This should produce the same error, using the files in output/.

  5. Edit output/random.js (that's the randomly generated JS that caused the error). Remove a small chunk of it, run ./cli.js ./examples/prettier.js output/ -r again and repeat until you've narrowed down a small repro.


Finally, here's my attempt at defining a high-level rule for semicolons. Might be totally wrong :)

  1. Print as few semicolons as possible. Put them at the end of lines.
  2. If the needed semicolon is because of a following ExpressionStatement, move the semicolon to the start of the ExpressionStatement (that is, at the start of the next line).
  3. If an ExpressionStatement is the first thing on a new line and starts with (, [, ` (or possibly others), add an "unnecessary" semicolon at the start if possible (for example, not after if, else, for, while and do).
Collaborator

lydell commented Apr 6, 2017

For this one:

class A {
  static; 
  x(){}
}

... I think it is totally fine put the semicolon at the end of the static line instead of at the start of the x(){} line. It's an edge case that I wouldn't expect to find anywhere in real code bases, so, to me, the printing doesn't really matter as long as the program is correct.


Regarding the fuzzer. I might get the time to run it in a few days. But I'll tell you how I run it anyway!

  1. Clone https://github.com/lydell/eslump.

  2. Edit examples/prettier.js:

    • I've been changing it to only use babylon lately (instead of randomly picking between flow and babylon), because the fuzzer likes to generate the edge cases that flow doesn't support. Not sure if newer flow versions work better.
    • Make sure that the version of prettier that you want to test is require()d.
  3. Run ./cli.js ./examples/prettier.js output/. You can also pass the --comments and --whitespace flags to also randomize comments and whitespace. I'd start without them, then add only --comments, then add only --whitespace and finally add both.

  4. When the fuzzer finds an error, I press the Up arrow in the terminal, to edit the last command and add the -r flag (short for --reproduce) and run again. This should produce the same error, using the files in output/.

  5. Edit output/random.js (that's the randomly generated JS that caused the error). Remove a small chunk of it, run ./cli.js ./examples/prettier.js output/ -r again and repeat until you've narrowed down a small repro.


Finally, here's my attempt at defining a high-level rule for semicolons. Might be totally wrong :)

  1. Print as few semicolons as possible. Put them at the end of lines.
  2. If the needed semicolon is because of a following ExpressionStatement, move the semicolon to the start of the ExpressionStatement (that is, at the start of the next line).
  3. If an ExpressionStatement is the first thing on a new line and starts with (, [, ` (or possibly others), add an "unnecessary" semicolon at the start if possible (for example, not after if, else, for, while and do).
@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Apr 6, 2017

Collaborator

@rattrayalex Test cases seem reasonably complete at first glance.

Collaborator

bakkot commented Apr 6, 2017

@rattrayalex Test cases seem reasonably complete at first glance.

src/printer.js
@@ -3260,6 +3273,84 @@ function hasLeadingOwnLineComment(text, node) {
return res;
}
+function hasNakedLeftSide(node) {
+ return (
+ node.type === "BinaryExpression" ||

This comment has been minimized.

@sunjay

sunjay Apr 6, 2017

Idea: instead of the many many many === statements, what if you were to move all the constants you're testing out into a global const Set? The time to check a set is very performant and this change would make the code smaller, less repetitive and easier to read. If you give the set a nice, easy to understand name, it'll be even better!

Since this is a function that runs many times, it's worth measuring the actual difference that makes in performance. There's going to have to be a trade off here between performance and readability. I'm suspicious of how the readable version will perform.

@sunjay

sunjay Apr 6, 2017

Idea: instead of the many many many === statements, what if you were to move all the constants you're testing out into a global const Set? The time to check a set is very performant and this change would make the code smaller, less repetitive and easier to read. If you give the set a nice, easy to understand name, it'll be even better!

Since this is a function that runs many times, it's worth measuring the actual difference that makes in performance. There's going to have to be a trade off here between performance and readability. I'm suspicious of how the readable version will perform.

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

This is the style used pretty consistently throughout the prettier codebase; I moved to a fake-set once (dictionary with values of true) and was asked to move back to this style.

It might be possible that a Set-based style could be adopted in the future, but that'd probably best be done as a separate refactoring PR (with a preceding issue verifying that it's desired).

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

This is the style used pretty consistently throughout the prettier codebase; I moved to a fake-set once (dictionary with values of true) and was asked to move back to this style.

It might be possible that a Set-based style could be adopted in the future, but that'd probably best be done as a separate refactoring PR (with a preceding issue verifying that it's desired).

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

I'm not convinced that it is more readable with a set. This is a special case where there are a lot of chained ones, but most of the time, you check a few conditions, then more conditions on the child... So it's easier to read when everything is inlined like this.

Also, for the Set, I'm not convinced that it is going to be faster. In this case, we're just comparing either pointers (static strings are usually unique-ified) or first few characters of a string which both are really fast. You don't need to do a function call and allocation at the beginning of the program.

I'm pretty sure we have lower hanging fruit in terms of performance than the way we do those comparisons. And as @rattrayalex mentioned, if we're going that way, we should do it across the entire codebase.

@vjeux

vjeux Apr 6, 2017

Collaborator

I'm not convinced that it is more readable with a set. This is a special case where there are a lot of chained ones, but most of the time, you check a few conditions, then more conditions on the child... So it's easier to read when everything is inlined like this.

Also, for the Set, I'm not convinced that it is going to be faster. In this case, we're just comparing either pointers (static strings are usually unique-ified) or first few characters of a string which both are really fast. You don't need to do a function call and allocation at the beginning of the program.

I'm pretty sure we have lower hanging fruit in terms of performance than the way we do those comparisons. And as @rattrayalex mentioned, if we're going that way, we should do it across the entire codebase.

This comment has been minimized.

@sunjay

sunjay Apr 6, 2017

No problem. :) I originally commented because you mentioned in another comment that this was a bit ugly. Since that's the convention elsewhere, it makes sense to follow it. :)

@sunjay

sunjay Apr 6, 2017

No problem. :) I originally commented because you mentioned in another comment that this was a bit ugly. Since that's the convention elsewhere, it makes sense to follow it. :)

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

I can't remember the context but I probably meant that it was ugly to enumerate all those node types, rather than the way we're actually doing it :)

@vjeux

vjeux Apr 6, 2017

Collaborator

I can't remember the context but I probably meant that it was ugly to enumerate all those node types, rather than the way we're actually doing it :)

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

Thanks for the input all!

Regarding "a high-level rule for semicolons", I think I'm going to go with:

  1. Prefix ExpressionStatements with a semicolon when they "start with" one of the following:
  • Array, +Unary, -Unary, Template String, JSXElement, Regex
  • Any expression wrapped in parens
  1. Postfix ClassProperties with a semicolon when:
  • followed with a "problematic Method" (computed or generator, but not get/set/async/static)
  • the property itself is "potentially problematic" (named "static", "get", or "set", with no value)

Does that sound good?

Collaborator

rattrayalex commented Apr 6, 2017

Thanks for the input all!

Regarding "a high-level rule for semicolons", I think I'm going to go with:

  1. Prefix ExpressionStatements with a semicolon when they "start with" one of the following:
  • Array, +Unary, -Unary, Template String, JSXElement, Regex
  • Any expression wrapped in parens
  1. Postfix ClassProperties with a semicolon when:
  • followed with a "problematic Method" (computed or generator, but not get/set/async/static)
  • the property itself is "potentially problematic" (named "static", "get", or "set", with no value)

Does that sound good?

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

I'll try to do some of the fuzzing today if I have time once I enact the above changes to the class property algo.

Collaborator

rattrayalex commented Apr 6, 2017

I'll try to do some of the fuzzing today if I have time once I enact the above changes to the class property algo.

]);
case "DeclareVariable":
- return printFlowDeclaration(path, ["var ", path.call(print, "id"), ";"]);
+ return printFlowDeclaration(path, ["var ", path.call(print, "id"), semi]);

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

does anyone know if Flow requires semis inside declarations and other such places?

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

does anyone know if Flow requires semis inside declarations and other such places?

This comment has been minimized.

@vjeux

vjeux Apr 6, 2017

Collaborator

@mroch, would you be able to shed some light here? This pull request attempts to implement no-semi on prettier and we're not 100% sure how flow type declarations interact with automatic-semicolon-insertion.

@vjeux

vjeux Apr 6, 2017

Collaborator

@mroch, would you be able to shed some light here? This pull request attempts to implement no-semi on prettier and we're not 100% sure how flow type declarations interact with automatic-semicolon-insertion.

+ }
+
+ // this isn't actually possible yet with most parsers available today
+ // so isn't properly tested yet.

This comment has been minimized.

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

I did test this locally by changing static to staticc both here and in the tests, so I know it works, but I feel pretty iffy about leaving this in regardless... thoughts?

@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

I did test this locally by changing static to staticc both here and in the tests, so I know it works, but I feel pretty iffy about leaving this in regardless... thoughts?

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 7, 2017

Member

I got lots to catch up on here 😄 Good work!

Member

jlongster commented Apr 7, 2017

I got lots to catch up on here 😄 Good work!

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 7, 2017

Collaborator

I feel good about where this is but want to do some more comprehensive testing. Basically check that prettier(prettier(code)) === prettier(code) with the no-semi option across as much of a repertoire as possible.

Which, now that I think about it, should probably be an ordinary component of the test suite. Would obviously slow things down but probably worth it.

Hope to play with lydell's fuzzer as well.

Waiting a few days for this to land is almost definitely worth it; apologies to those following along with eagerness, though 😄

Collaborator

rattrayalex commented Apr 7, 2017

I feel good about where this is but want to do some more comprehensive testing. Basically check that prettier(prettier(code)) === prettier(code) with the no-semi option across as much of a repertoire as possible.

Which, now that I think about it, should probably be an ordinary component of the test suite. Would obviously slow things down but probably worth it.

Hope to play with lydell's fuzzer as well.

Waiting a few days for this to land is almost definitely worth it; apologies to those following along with eagerness, though 😄

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 7, 2017

Collaborator

Ah, I see there's the AST_COMPARE test. I see several (51) failures running that on master – should those be fixed? Ideally this PR would pass that test prior to merge – I'd be okay blocking this PR on fixing those breakages if that'd be acceptable.

Perhaps a run of AST_COMPARE=1 jest should be added to the travis run?

I'm also seeing a few failures from the fuzzer:

  • for (const v of /[|\h-\l)-]+?/gmy): prettier removed necessary parens around the regex.
  • newlines in case statements unstable on the latest master (as of a few hours ago).
  • function x(y = class {} <= 1) {: prettier removed the necessary parens around the class {}
  • there were some unstable comments as well when running it with --comments

The fuzzer did catch one or two cases which I have fixed – awesome tool!!

Collaborator

rattrayalex commented Apr 7, 2017

Ah, I see there's the AST_COMPARE test. I see several (51) failures running that on master – should those be fixed? Ideally this PR would pass that test prior to merge – I'd be okay blocking this PR on fixing those breakages if that'd be acceptable.

Perhaps a run of AST_COMPARE=1 jest should be added to the travis run?

I'm also seeing a few failures from the fuzzer:

  • for (const v of /[|\h-\l)-]+?/gmy): prettier removed necessary parens around the regex.
  • newlines in case statements unstable on the latest master (as of a few hours ago).
  • function x(y = class {} <= 1) {: prettier removed the necessary parens around the class {}
  • there were some unstable comments as well when running it with --comments

The fuzzer did catch one or two cases which I have fixed – awesome tool!!

@@ -1095,7 +1104,7 @@ function genericPrintNoParens(path, options, print) {
const hasBraces = isCurlyBracket(clause);
if (hasBraces) parts.push(" while");
- else parts.push(concat([line, "while"]));
+ else parts.push(concat([hardline, "while"]));

This comment has been minimized.

@vjeux

vjeux Apr 7, 2017

Collaborator

👍

@vjeux

vjeux Apr 7, 2017

Collaborator

👍

src/printer.js
@@ -1,5 +1,14 @@
"use strict";
+// other cases that are unstable/problematic:

This comment has been minimized.

@vjeux

vjeux Apr 7, 2017

Collaborator

Let's create issues for those instead of adding comments

@vjeux

vjeux Apr 7, 2017

Collaborator

Let's create issues for those instead of adding comments

This comment has been minimized.

@rattrayalex

rattrayalex Apr 7, 2017

Collaborator

Yeah that was just a note-to-self since I was coding without internet. I'll be back online in an hour or two and will create issues if you don't beat me to it 😉

@rattrayalex

rattrayalex Apr 7, 2017

Collaborator

Yeah that was just a note-to-self since I was coding without internet. I'll be back online in an hour or two and will create issues if you don't beat me to it 😉

This comment has been minimized.

@rattrayalex

rattrayalex Apr 7, 2017

Collaborator

(for posterity, vjeux filed as babel/babylon#457 and babel/babylon#456, as well as several comment-related issues on this repo)

@rattrayalex

rattrayalex Apr 7, 2017

Collaborator

(for posterity, vjeux filed as babel/babylon#457 and babel/babylon#456, as well as several comment-related issues on this repo)

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 7, 2017

Collaborator

Perhaps a run of AST_COMPARE=1 jest should be added to the travis run?

We've never brought it down to 0, good call! I'll work on it today.

Collaborator

vjeux commented Apr 7, 2017

Perhaps a run of AST_COMPARE=1 jest should be added to the travis run?

We've never brought it down to 0, good call! I'll work on it today.

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 7, 2017

Collaborator

Alright, I've fuzzed and I've AST_COMPARE'd and I've found a handful of edge cases that needed fixing and now I think we're pretty much good to go.

There aren't any more AST_COMPARE breakages on this branch than master (both at 51), but theoretically there could be a few hiding behind breakages, or corner cases we haven't found yet.

Similarly, when I run the fuzzer repeatedly, the only breakages I can find are the same ones previously reported – so there's a chance there's a corner case hiding somewhere, but seems unlikely.

The most likely stone left unturned at this point is semicolons added in that aren't necessary; I looked for ";" in the codebase, but if there are semicolons added in other ways, they could sneak through. I might try to hunt more carefully now; otherwise, maybe we can smoke those out later on.

I think this is ready for review from @jlongster whenever you're ready.

Collaborator

rattrayalex commented Apr 7, 2017

Alright, I've fuzzed and I've AST_COMPARE'd and I've found a handful of edge cases that needed fixing and now I think we're pretty much good to go.

There aren't any more AST_COMPARE breakages on this branch than master (both at 51), but theoretically there could be a few hiding behind breakages, or corner cases we haven't found yet.

Similarly, when I run the fuzzer repeatedly, the only breakages I can find are the same ones previously reported – so there's a chance there's a corner case hiding somewhere, but seems unlikely.

The most likely stone left unturned at this point is semicolons added in that aren't necessary; I looked for ";" in the codebase, but if there are semicolons added in other ways, they could sneak through. I might try to hunt more carefully now; otherwise, maybe we can smoke those out later on.

I think this is ready for review from @jlongster whenever you're ready.

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 10, 2017

Collaborator

I checked for other insertions of ; and found a few and fixed them.

I think this is ready pending final review, though I'd also like to put up a straw man PR with semi: false as the default option so folks can audit the output. I tried looking myself on the command line for a bit and it was a bit much.

Thinking about it more, I also think a solution based on the printed doc would actually be more correct, robust, and performant than the AST solution in this case. Not sure it's worth putting together though.

Collaborator

rattrayalex commented Apr 10, 2017

I checked for other insertions of ; and found a few and fixed them.

I think this is ready pending final review, though I'd also like to put up a straw man PR with semi: false as the default option so folks can audit the output. I tried looking myself on the command line for a bit and it was a bit much.

Thinking about it more, I also think a solution based on the printed doc would actually be more correct, robust, and performant than the AST solution in this case. Not sure it's worth putting together though.

+
+ // Whether to add a semicolon at the end of every line (semi: true),
+ // or only at the beginning of lines that may introduce ASI failures (semi: false)
+ semi: true

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

I think it's easier to have options that are by default false, so that people can just add them on the CLI easier. How about "noSemi" and the CLI arg would be --no-semi? For some reason I just like the args themselves describing the change.

@jlongster

jlongster Apr 11, 2017

Member

I think it's easier to have options that are by default false, so that people can just add them on the CLI easier. How about "noSemi" and the CLI arg would be --no-semi? For some reason I just like the args themselves describing the change.

This comment has been minimized.

@vjeux

vjeux Apr 11, 2017

Collaborator

@jlongster I made the recommendation for it to be true by default. This way the argument parser that we use makes --no-semi work. But maybe it's too clever.

@vjeux

vjeux Apr 11, 2017

Collaborator

@jlongster I made the recommendation for it to be true by default. This way the argument parser that we use makes --no-semi work. But maybe it's too clever.

This comment has been minimized.

@lydell

lydell Apr 11, 2017

Collaborator

We already have bracketSpacing as true by default and --no-bracket-spacing for turning it off.

@lydell

lydell Apr 11, 2017

Collaborator

We already have bracketSpacing as true by default and --no-bracket-spacing for turning it off.

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

Ok, yeah the CLI arg is the main thing I was thinking about. I guess it's fine if the API option is different and the CLI one is --no-semi.

@jlongster

jlongster Apr 11, 2017

Member

Ok, yeah the CLI arg is the main thing I was thinking about. I guess it's fine if the API option is different and the CLI one is --no-semi.

+ // For now, we're just passing that information by mutating the AST here,
+ // but it would be nice to find a cleaner way to do this.
+ node.needsParens = needsParens;
+ }

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

Here's my idea: I don't see any reason why we can't have access to the path instead of the node, and we can just call path.needsParens() multiple times. To fix the performance hit, we could cache that method so if it's already been calculated it just returns the value.

The only problem is that the path abstraction using mutation heavily, so writing the cache isn't too straight-forward, but it's doable. I think we've talked about making the path abstraction better anyway.

I'll make another comment below about this. But I don't think this code is complex enough to block this PR from landing. We can work on this later. I feel OK about letting this stuff in prettier because we aren't exposing these objects over an API or anything so these hacks don't impact users, as long as the output is good.

@jlongster

jlongster Apr 11, 2017

Member

Here's my idea: I don't see any reason why we can't have access to the path instead of the node, and we can just call path.needsParens() multiple times. To fix the performance hit, we could cache that method so if it's already been calculated it just returns the value.

The only problem is that the path abstraction using mutation heavily, so writing the cache isn't too straight-forward, but it's doable. I think we've talked about making the path abstraction better anyway.

I'll make another comment below about this. But I don't think this code is complex enough to block this PR from landing. We can work on this later. I feel OK about letting this stuff in prettier because we aren't exposing these objects over an API or anything so these hacks don't impact users, as long as the output is good.

This comment has been minimized.

@rattrayalex

rattrayalex Apr 11, 2017

Collaborator

Cool. I think if we were to make a change at this point I'd rather go to reading tokens. It dodges this issue through elegance rather than complexity, and should be more robust/performant to boot.

My primary thinking is that it's fundamentally an output-token-based problem, not an AST problem (other than only applying it to statements, which is useful). The question you're trying to answer when deciding whether to prepend a semicolon isn't "is this a TypeCastExpression" or what have you, it's "does this start with a paren".

As the algorithm stands, we're vulnerable to parens being inserted in the future in places that we're not currently looking for them. It's only through extensive smoke tests that I was able to discover cases like ArrowFunctionExpressions, which sometimes begin with a paren, for example.

Note that I think the class body stuff should probably still be done with the AST, since the path is more appropriate there.

... maybe it's time I stop talking and start doing with this, and we can see how it looks. I'll add as a second commit, and will be happy to go back to the AST approach if that's what we prefer.

@rattrayalex

rattrayalex Apr 11, 2017

Collaborator

Cool. I think if we were to make a change at this point I'd rather go to reading tokens. It dodges this issue through elegance rather than complexity, and should be more robust/performant to boot.

My primary thinking is that it's fundamentally an output-token-based problem, not an AST problem (other than only applying it to statements, which is useful). The question you're trying to answer when deciding whether to prepend a semicolon isn't "is this a TypeCastExpression" or what have you, it's "does this start with a paren".

As the algorithm stands, we're vulnerable to parens being inserted in the future in places that we're not currently looking for them. It's only through extensive smoke tests that I was able to discover cases like ArrowFunctionExpressions, which sometimes begin with a paren, for example.

Note that I think the class body stuff should probably still be done with the AST, since the path is more appropriate there.

... maybe it's time I stop talking and start doing with this, and we can see how it looks. I'll add as a second commit, and will be happy to go back to the AST approach if that's what we prefer.

+ return false;
+ }
+
+ return exprNeedsASIProtection(getLeftSide(node));

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

If you were dealing with path instead, you'd do path.call(exprNeedsASIProtection, getLeftSideName(path)) or something like that. But I don't know.

@jlongster

jlongster Apr 11, 2017

Member

If you were dealing with path instead, you'd do path.call(exprNeedsASIProtection, getLeftSideName(path)) or something like that. But I don't know.

+function exprNeedsASIProtection(node) {
+ // HACK: node.needsParens is added in `genericPrint()` for the sole purpose
+ // of being used here. It'd be preferable to find a cleaner way to do this.
+ const maybeASIProblem = node.needsParens ||

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

I see that this is recursive, and the top-level statement kicks this off. I'm assuming it's possible for a node to be mutated deep down when printing, and then the recursion here eventually picks that up? I thought at first you only need to check top-level statements, but I think it's more complex than that.

As I said above I'm not too worried about merging this as is, and if we ever get around to cleaning it up, we can. We should keep this in mind if we ever encourage people to pass ASTs straight in though, as we will be modifying it (though we modify it with comments anyway).

@jlongster

jlongster Apr 11, 2017

Member

I see that this is recursive, and the top-level statement kicks this off. I'm assuming it's possible for a node to be mutated deep down when printing, and then the recursion here eventually picks that up? I thought at first you only need to check top-level statements, but I think it's more complex than that.

As I said above I'm not too worried about merging this as is, and if we ever get around to cleaning it up, we can. We should keep this in mind if we ever encourage people to pass ASTs straight in though, as we will be modifying it (though we modify it with comments anyway).

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

This looks pretty straight-forward in general, and all the complicated logic is isolated to its own section. Great work! Just talking out loud in my comments, but I don't think much needs to be changed before merging. The main thing is the name of the option. What do you think about --no-semi? I think it makes it clear that we encourage semicolons and its the default.

Member

jlongster commented Apr 11, 2017

This looks pretty straight-forward in general, and all the complicated logic is isolated to its own section. Great work! Just talking out loud in my comments, but I don't think much needs to be changed before merging. The main thing is the name of the option. What do you think about --no-semi? I think it makes it clear that we encourage semicolons and its the default.

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 11, 2017

Collaborator

Sounds like the consensus is to leave semi: false and tell users to use --no-semi at the command line, which is fine.

I'd like to take a crack today at the doc-based approach; if I'm not able to get it done today or if y'all don't end up finding it preferable, I'm happy to ship this.

EDIT: You're also welcome to merge, and I can propose the alternative approach as a follow-up PR.

Collaborator

rattrayalex commented Apr 11, 2017

Sounds like the consensus is to leave semi: false and tell users to use --no-semi at the command line, which is fine.

I'd like to take a crack today at the doc-based approach; if I'm not able to get it done today or if y'all don't end up finding it preferable, I'm happy to ship this.

EDIT: You're also welcome to merge, and I can propose the alternative approach as a follow-up PR.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

I see where you are going with the doc-based approach. It might be simpler for most of the use cases, but not sure how you'd solve some of the edge cases. It'd be interesting for you to try it, but I'll leave it up to @vjeux if he wants to go ahead and merge this. My gut feeling is we should and you can land a new implementation later if it's simpler and works.

Member

jlongster commented Apr 11, 2017

I see where you are going with the doc-based approach. It might be simpler for most of the use cases, but not sure how you'd solve some of the edge cases. It'd be interesting for you to try it, but I'll leave it up to @vjeux if he wants to go ahead and merge this. My gut feeling is we should and you can land a new implementation later if it's simpler and works.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

Note that I think the class body stuff should probably still be done with the AST, since the path is more appropriate there.

Oh, those are the edge cases I was thinking about. In that case, if you can spend a little time on that approach you could let us know if it looks promising enough to hold off?

Member

jlongster commented Apr 11, 2017

Note that I think the class body stuff should probably still be done with the AST, since the path is more appropriate there.

Oh, those are the edge cases I was thinking about. In that case, if you can spend a little time on that approach you could let us know if it looks promising enough to hold off?

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

I'm doing a pretty big release right now (first one in 3 weeks), so let me know if you want this one included :)

Collaborator

vjeux commented Apr 11, 2017

I'm doing a pretty big release right now (first one in 3 weeks), so let me know if you want this one included :)

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 11, 2017

Collaborator
Collaborator

rattrayalex commented Apr 11, 2017

@vjeux vjeux merged commit 36bec87 into prettier:master Apr 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

<3 This release is going to be huge!

Collaborator

vjeux commented Apr 11, 2017

<3 This release is going to be huge!

@frol frol referenced this pull request Apr 12, 2017

Closed

Semicolon option #736

@frol

This comment has been minimized.

Show comment
Hide comment
@frol

frol Apr 12, 2017

I have just tried it with my project and the output is nice and 100% valid! Thank you so much!

P.S. I couldn't find --no-semi option on the CLI utility, so I patched it slightly just so it has semi: false by default in it. Is that intended or exposing the CLI parameter should be just be done in a separate PR?

frol commented Apr 12, 2017

I have just tried it with my project and the output is nice and 100% valid! Thank you so much!

P.S. I couldn't find --no-semi option on the CLI utility, so I patched it slightly just so it has semi: false by default in it. Is that intended or exposing the CLI parameter should be just be done in a separate PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment