Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

use consistent semicolon elision rules #26

Closed
flying-sheep opened this issue Jan 7, 2016 · 60 comments
Closed

use consistent semicolon elision rules #26

flying-sheep opened this issue Jan 7, 2016 · 60 comments

Comments

@flying-sheep
Copy link

my JS style elides semicolons, and method literals in class declarations have no trailing delimiter neither.

therefore requiring semicolons results in a idiosyncracy compared to the rest of the language’s grammar.

i’m aware of #25 which was about a quick fix for ambiguities by requiring them temporarily. this bug is about the backwardds-compatible idiomatic final grammar which will allow eliding semicolons.

Updated status:

this was a misunderstanding. @jeffmo explained it here.

ASI is in effect here, and babel won’t require semicolons anymore soon

@LinusU
Copy link

LinusU commented Jan 7, 2016

Please do not require us to use semicolons just for class fields and static properties.

It might be true that including semicolons can speed up the parsing of the source, but that is true in a lot of other places as well. Yet, we don't require them.

There are a huge number of people writing their javascript without semicolons, and requiring them in just a few places will increase the cognitive load of reading and writing the code.

Parsing the code without semicolons isn't a problem, babel for example did this up until very recently. The decision to require semicolons is a pure performance question. I don't think it's fair at all to force this on to the developers. If it's that important to speed up parsing, we should require semicolons everywhere.

I really don't think the benefit outweighs the negative here, and I think it's very unfair to force this upon people who normally doesn't use semicolons in their javascript.

@Nyalab
Copy link

Nyalab commented Jan 7, 2016

please be consistent with the core language specification

@MoOx
Copy link

MoOx commented Jan 7, 2016

Same feeling. All my recent codebase has not a single semilcolon (no, I don't have a single for loop), and now I have to have some. This feel like going backward.
Saying "it's for the parser" feel really weird. Grammar should adapt to users, not the opposite due to some "possible performances issues".

This should be discussed in ESDiscuss probably...

@flying-sheep
Copy link
Author

yeah, it’s a matter of consistency/predictability.

if you’re accustomed to ASI, a place where it doesn’t work is very surprising and therefore bad language design.

@jeffshaver
Copy link

I opened #25 because I don't generally use semi-colons and I wanted clarification. I would also prefer them not to be required.

@michaelficarra
Copy link
Member

Where did you people come from? Please stop.

The committee will not accept a proposal that requires a parser to either rewind the token stream or build multiple potential parse results simultaneously. In fact, a requirement for this was recently removed: function f(a = 0) { "use strict"; } is now an early error because it would require going back through the parameter list (which may be very complex) once the "use strict" directive is reached. If you want this feature at all, it is going to have semicolons required. Nothing you say here will change that.

I pray I never have to read any of the code that commenters in this thread have written.

@domenic
Copy link
Member

domenic commented Jan 7, 2016

Yes, it seems like the most likely outcome of these complaints will be to sink the class fields proposal, if the evidence is that the community doesn't want it in a form the committee will accept. Be careful.

@jeffmo
Copy link
Member

jeffmo commented Jan 7, 2016

First off: Thanks for the feedback everyone (no like really, I think it's important that we talk through this). It's a tough call to make here for sure. I want to reassure everyone that this isn't a decision we took lightly (and if we can find a reasonable way out of this, I'm definitely open to considering options).

The choice to require semicolons was made based on technical considerations exclusively and was not an effort to enforce any particular style, so I hope that's clear to begin with.

As mentioned in #25, there is a real ambiguity here that affects parser performance. Parser performance, in turn, affects the willingness of people within TC39 to accept the proposal and the willingness of people who implement JS runtimes (like browser vendors) to implement this feature. The reason the people within TC39 have an opinion on this is because this kind of parser performance won't just affect usage sites of this particular feature, but the parser as a whole. In other words: A misstep here could affect JS web performance as a whole.

I'm empathetic to the concerns here, but I hope everyone understands the rock/hard-place situation we're in with this detail of the proposal.

Please, if you feel strongly about this issue, try to help constructively by offering alternatives that address the core issue. I can't promise that just any alternative will succeed, but all will be considered if they make sense and the trade-offs seem reasonable.

@flying-sheep
Copy link
Author

OK, so dumb question:

using the same ASI rules as elsewhere would fail… why?

@jeffmo
Copy link
Member

jeffmo commented Jan 7, 2016

@flying-sheep:

using the same ASI rules as elsewhere would fail… why?

Consider this JS of today:

var obj = {}

var a = obj
[42]

console.log(a) // prints "undefined"

Consider this example with this proposal:

var obj = {}

class Foo {
  a = obj
  [someComputedPropertyName]() { ... }
}

In the former, we can interpret the "var a = obj\n[42]" as a member expression, in the latter we could not.

@flying-sheep
Copy link
Author

ok. hmm. two ways out:

  1. make this a syntax error and require computed property names to not follow a static property without trailing semicolon
  2. change ASI rules to insert semicolons when static properties are followed by opening brackets.

both surprising, but better than requiring semicolons unlike everywhere else except in for loops.

@electerious
Copy link

I don't know if it's true, but I have the feeling that classes in JS are especially helpful for newbies. Classes might be familiar as they know them from other languages. The explanation "In the former, we can interpret the 'var a = obj\n[42]' as a member expression, in the latter we could not." makes sense, but is really really hard to understand for anyone new or not deeply involved in JS development.

Requiring semicolons here is a hard thing to remember. It's that kind of thing you will always accidentally do wrong, even when you know how to do it right. It could also heat up the "Do I need semicolons in JS" discussion, again.

That said, I don't know if "Class Fields and Static Properties" in their current proposal are good for the language. I would really like to use them, but the semicolon thing could have a negative impact. At least the latest change in Babel shows that there's little understanding on what's going on.

@michaelficarra
Copy link
Member

@flying-sheep Your first option is already the case thanks to ASI. You should only need to include a literal semicolon in places where it would cause a syntax error if you did not.

This is okay:

class a {
  a = b
  c(){}
}

This is not okay:

class a {
  a = b
  [c](){}
}

This is okay:

class a {
  a = b
  ;[c](){}
}

But please don't write it like that.

@flying-sheep
Copy link
Author

there’s no reason apart from the grammar complexity to disallow the second one.

it can only be interpreted in one way.

@michaelficarra
Copy link
Member

@flying-sheep I am aware, and that's what I was saying would never be accepted by the committee. ASI rules insert a semicolon when the following token would cause a parse error. So in that example, the semicolon would be inserted before {, which would still be a parse error. It'd be impractical to try to insert semicolons between every token before the parse error, looking for a possible parse.

@Removed-5an
Copy link

The "For the parser" argument really makes sense here.

Omitting semicolons using ASI is nice and all but they are part of ECMAScript and if omitting them in this case would slow down the parser significantly, is it really worth it?

My preference would be to drop ASI completely and require semicolons everywhere, but that's never going to happen so I guess I agree with the others above and have consistent language design at the cost of parser performance, altho if I understand @michaelficarra correctly that's also not going to fly, so I guess we're stuck...

@mjackson
Copy link

mjackson commented Jan 8, 2016

@michaelficarra Is

class a {
  [c](){}
}

already a thing? It seems like that's the real issue here.

@loganfsmyth
Copy link

@mjackson Just a normal ES6 computed property method.

@michaelficarra
Copy link
Member

Yes, it is already a thing.

@mjackson
Copy link

mjackson commented Jan 8, 2016

Just a normal ES6 computed property method.

@loganfsmyth I know this ship has sailed, but there's nothing normal about class a { [c](){} }.

An object property definition will always be preceded by either { (the beginning of an object literal) or the , which trails the previous key/value pair. This makes it easy to parse. But in a class definition, where we don't separate method definitions with , more care should have been taken before approving computed method names.

Besides, method definitions are not properties. In fact, there are a few important differences:

  • A property assignment has a : between the property name and the value of that property, as in { a: 'b' }. Method definitions don't have a : between the method name and its definition.
  • In a property assignment, both a property name and its value may be computed. In a method definition, the "left-hand side" may be computed (i.e. the [c]) but the "right-hand side" cannot be. Besides the irregularity, this also makes method definitions a lot less flexible than property definitions.

Besides this, consider that the list of places where you currently must use semi-colons in JavaScript is very short. It includes:

  • lines that end with an expression where the next non-empty line begins with ( or [ (which are rare, especially now that most people use modules instead of immediately-invoked closures)
  • for statements (which are often replaced with forEach)

If requiring a ; after static property initializers gets approved, it will easily be the most commonly used place in JavaScript where a semi-colon may not automatically be inserted by the parser.

@michaelficarra
Copy link
Member

method definitions are not properties

Method definitions are properties.

lines that end with an expression where the next non-empty line begins with ( or [

Or + or / or - or ```.

most commonly used place in JavaScript where a semi-colon may not automatically be inserted by the parser

It can be automatically inserted for static property names. How often are you using computed property names?

@flying-sheep
Copy link
Author

yeah, i think requiring it before computed class property names is just as surprising as requiring it after class property declarations, but will end up making things less ugly.

in all JS i ever wrote, i doubt i started more than 2 lines or so with ( or [: IIFEs are hacks, and things like [x, y, z].forEach(e => ...) can be written by binding the temporary array to a variable name first.

i also think that computed property names will be rare apart from [Symbol.Iterator](), so that should be a good compromise

@mjackson
Copy link

mjackson commented Jan 8, 2016

Method definitions are properties.

Inside a class body, method definitions are declarations, not object property definitions. i.e. they aren't followed by a ,.

How often are you using computed property names?

Never. And I'm certainly not using computed names for method declarations. That's why this is so annoying. Because in order to use this feature I'll have to use semi-colons, which I literally never have to use anywhere else.

It can be automatically inserted for static property names

I'm referring to babel/babel#3225 where a change was recently made that causes an error to be thrown when class properties do not have a semi-colon. If they can be automatically inserted, that change should be reverted. But it looks like it was actually considered a bug in the way Babel interprets this spec.

@PinkaminaDianePie
Copy link

If removing semicolons from this proposal will slow down its shipping by TC39 - ill prefer to use semicolons. Its better to have good language feature, with small part of code style you dont like, than dont have anything at all. Javascript does not have consistent code style, such as Java's and its not so good to discard anything because you dont like its style. So if TC39 can accept this proposal without semicolons - its OK, if it will discard it - its better to stay with semicolons. Code style should not be the main criteria at all!

@flying-sheep
Copy link
Author

it’s better to have a good language than to slap on features with no perspective or consideration.

that would be how you get PHP and how JS got into the state it was in before ES2015. fucking variable hoisting, type coercion, all that bullshit.

no, we don’t want to go back there.


also if there is anything wrong with my proposal above i want to hear it.

a good advice for any conversation is:

listen, don’t wait to speak

this thread has seen only rudimentary discussion but many people chiming in with emotion and little to say.


SO.

solutions

rated by cognitive load, inconsistency in language design, and code style compatibility.

those three are to consider because 1. you don’t want programmers to think too much about syntax while coding. it’s a tool and should stay out of the way. 2. good language design is as internally consistent as possible. 3. people already write code in this language, so suddenly “blessing”
a certain code style by requiring it in parts of the language is only bound to make them angry

keep semicolons

  • [-1] exception to the ASI rules (simple to grasp)
  • [-2] very inconsistent: deviates in to two ways from how the rest of the language works:
    • for loops have a fixed number of required semicolons. they don’t act as arbitrary statement delimiters there, but precisely between the three expressions necessary in a for loop
    • ASI everywhere else
  • [-1] ugly with certain code styles

allow everything

  • [-0] no cognitive load
  • [-0] consistent
  • [-0] allows all code styles
  • [-∞] complex grammar: if a computed-name method follows a class property (class X { a = b\n[c]() {...} }), the only way to distinguish it from a index access on the right-hand side (class X { a = b\n[c]() }) is the braces.

complex grammar is apparently a deal-breaker, so -∞ points

introduce special clarification rule (semicolon between class property and computed-name method)

  • [-1] exception to ASI rules (simple to grasp)
  • [-1] slightly inconsistent: exception to usual ASI rules by requiring a semicolon in one edge case
  • [-0] allows all code styles (semicolon-less requires clarifying semicolons in some places already)

in the end:

  • keep semicolons: -4
  • allow everything: -∞
  • special clarification rule: -2

a winner by points is the special clarification rule

i hope my point scheme was not too biased… but i wouldn’t have rooted for it if i didn’t weigh the pros and cons already and came to the conclusion that it was best

@bcomnes
Copy link

bcomnes commented Jan 8, 2016

I think @mjackson brings up an extremely important point:

The list of places where you currently must use semi-colons in JavaScript is very short.

This requirement adds complexity and edge cases to existing aspects of the language's ASI that we have to write books and tutorials about and make the language altogether more difficult to learn. I agree its important to require syntax lend itself to parsing performance, but this solution seems to be covering for awkward inconsistencies in the class syntax with the rest of the language.

@michaelficarra
Copy link
Member

Another syntax error for you semicolon haters:

class a {
  a = b
  *c(){}
}

😜

The "very short" list of problematic line start characters now has seven members.

@allenwb
Copy link
Member

allenwb commented Feb 2, 2016

@michaelficarra Do not fear, ASI rules are not changed with this proposal.

Exactly, I never intended to imply anything other than that the normal ASI rules apply, as quoted.

Of course, I will continue to argue that this confusion is a very good reason for not having this be a keyworldless declaration form.

It would be so much better if a keyword was required:

class Foo {
  field bar = baz
  field [bing[
}

@jeffmo
Copy link
Member

jeffmo commented Feb 3, 2016

@allenwb: A prefix keyword would help with properties that follow other properties, but given generator methods and any computed members we'll still have the same issues.

@michaelficarra: Good catch. Main point is that it's a parse error, though

@allenwb
Copy link
Member

allenwb commented Feb 3, 2016

@jeffmo
The prefix keyword is probably getting off topic for this issue. But if you are willing to consider that syntax alternative we should probably start a new issue to talk about it. I think there are lots of reasons that only concise methods should should be keyword free.

@littledan
Copy link
Member

My mistake, thanks for clearing it up everyone.

@dtothefp
Copy link

dtothefp commented Feb 3, 2016

for all you hipsters or :neckbeard: 's, I'm not sure which is appropriate here, who are "smart" enough to remember all the use cases where it is a must to use semicolons, and then "smart" enough to avoid those use cases just to avoid typing an extra character, you should have a look at destructuring syntax, in particular "assignment without declaration";

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Assignment_without_declaration.

Below is in reference to @flying-sheep #26 (comment) on how to easily avoid breaking syntax if not using semicolons.

let bleep, bloop

const str = 'bleep-bloop'
const obj = str.split('-').reduce((acc, key) => ({...acc, [key]: key}), {})
({bleep, bloop} = obj)
//Cannot read property 'bleep' of undefined
let bleep, bloop

const str = 'bleep-bloop'
const arr = str.split('-')
([bleep, bloop] = arr)
//Invalid attempt to destructure non-iterable instance

Looks like there might be more uses cases now if you all enjoy writing es6, which is what I think this debate is about...better start exercising that pinky finger.

@mjackson
Copy link

mjackson commented Feb 3, 2016

Thanks for not breaking ASI, @jeffmo. 👍

@flying-sheep
Copy link
Author

@dtothefp don’t start calling people names.

about your points:

  1. ASI exists. if it wouldn’t in this case, it still would elsewhere.

  2. ASI does affect you even if you use semicolons, e.g. when forgetting that

    return
        foo;

    in fact returns undefined

  3. think of ASI only when starting a line with punctuation and you’re good.

@bakkot
Copy link
Contributor

bakkot commented Aug 3, 2016

For posterity, this discussion is missing a couple more cases, namely, uninitialized properties named get, set, or static:

class A {
  get
  x
}

is a syntax error.

class A {
  static
  x
}

is a legal declaration of an uninitialized static property named x, not two non-static properties named static and x.

So if you want to omit semicolons, the rule is something like "whenever starting a line with punctuation or the previous line was an uninitialized property named 'get', 'set', or 'static' ". 😐

@flying-sheep
Copy link
Author

flying-sheep commented Aug 3, 2016

So if you want to omit semicolons […]

no, that rule is always, i.e. if you put semicolons in your examples, that doesn’t change anything, it’ll be a syntax error / static initialization anyway.

@bakkot
Copy link
Contributor

bakkot commented Aug 3, 2016

@flying-sheep, why? This looks like a syntactically valid class declaration with two uninitialized fields to me; am I missing something?

class A {
  get;
  x;
}

@michaelficarra
Copy link
Member

@bakkot I believe the examples you've given should be a syntax error. From the ASI rules,

When, as a Script or Module is parsed from left to right, a token (called the offending token) is encountered that is not allowed by any production of the grammar, then a semicolon is automatically inserted before the offending token if one or more of the following conditions is true:

Notice my emphasis. A semicolon would not be inserted after get because a production exists where x may follow get.

@bakkot
Copy link
Contributor

bakkot commented Aug 3, 2016

@michaelficarra, I agree; that's my point. (Well, except that my second example is a syntactically valid static field declaration.)

But with semicolons inserted manually, they should be fine, I think. That is, I think this is valid syntax:

class A {
  get;
  x;
}

So, if I am understanding correctly, this is another place where you can't get away with skipping semicolons.

@michaelficarra
Copy link
Member

Yes, that's right.

@ziemkowski
Copy link

sounds a lot like "Don't make beautiful and more readable code because if you use that clean style to write awful code that would confuse most people and any style guide should prohibit, it can end up confusing the runtime too."

@michaelficarra
Copy link
Member

class A { x = 0; }

Eww, gross.

class A { x = 0 }

Beautiful! Totally worth all the refactoring hazards. I only need to look ahead for 1 of 10 problematic tokens every time I make a change.

@ziemkowski
Copy link

In real code, in large files, yes all these crufty archaic semicolons are just unnecessary noise.

Once you work in semicolon-free js for a couple days, the perspective switches from "why shouldn't we?" to "why should we?" with thoughts like:

"Why are people adding these unnecessary semicolons everywhere when not one place here needs them?"

Because you just don't run into these scenarios in real code and even if you did they're things that become immediately apparent and easily fixable.

@ljharb
Copy link
Member

ljharb commented Aug 4, 2016

That's a very subjective opinion and controversial debate that we ABSOLUTELY MUST NOT get into in this thread. Just stop.

ASI rules (whether using or omitting) both require memorizing a list of rules, and applying them in your code.

A linter can help you remember, rendering the question of "which one's harder to remember" moot.

Whichever you use, any additions to the language will not make this particular debate any worse by changing those ASI rules - that's simply a nonstarter. If a given approach (using vs omitting) ends up being trickier with a syntax addition when following the existing rules, TOUGH. Adapt, or change your approach.

@michaelficarra
Copy link
Member

michaelficarra commented Aug 4, 2016

@ziemkowski I hope you think of me every time you look at

class A {
  x = 0
  [Symbol.iterator]() { /* ... */ }
}

or

class A {
  x = 0
  *generatorMethod() { /* ... */ }
}

and say to yourself, "what's wrong?".

edit: I apologise to those who are following the thread for the noise caused by my responses to @ziemkowski's trolling. I was not in the mood for this kind of ASI snobbery.

@bakkot
Copy link
Contributor

bakkot commented Aug 4, 2016

@allenwb, my reading of ASI is that an offending token must be one that is "not allowed by any production of the grammar", but here x is allowed by MethodDefinition even though the remainder of MethodDefinition doesn't match, and so ASI cannot occur.

@allenwb
Copy link
Member

allenwb commented Aug 4, 2016

@bakkot oops, I think you're right. I forgot about the "any production" restriction. In that case ASI produces:

class {
get 
x; }

which is a non-ASI-correctable syntax error.

I really dislike these keyword free property declarations (same for keyword free privates). It would all work so much better if property declarations started with a contextual keyword such as public. For example:

class {
  public get /* semi-inserted here*/
  public x /*semi-inserted here */
}

or

class {
  public get, y  /* semi-inserted here */
}

That was the sort of thing we were expecting when we made ; a ClassElement. We intended that ClassBody would be treated similarly to the treatment of StatementList

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests