flying-sheep commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

please be consistent with the core language specification

MoOx commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

OK, so dumb question:

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

jeffmo commented 8 years ago

@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 commented 8 years ago

ok. hmm. two ways out:

make this a syntax error and require computed property names to not follow a static property without trailing semicolon
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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

it can only be interpreted in one way.

michaelficarra commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

@michaelficarra Is

class a {
  [c](){}
}

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

loganfsmyth commented 8 years ago

@mjackson Just a normal ES6 computed property method.

michaelficarra commented 8 years ago

Yes, it is already a thing.

mjackson commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 https://github.com/babel/babel/pull/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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Another syntax error for you semicolon haters:

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

:stuck_out_tongue_winking_eye:

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

mjackson commented 8 years ago

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

And yet it's very rare that I actually have to tell someone "you need a semi-colon there". This proposal changes that.

flying-sheep commented 8 years ago

there’s good discussion in #25 about the proposal.

summary:

uninitialized class fields never need semicolons:

class A {
  b  // b[... is no valid start of a ClassElement → ASI or ;
  [c](){}
  d  // d*... is no valid start of a ClassElement → ASI or ;
  *e(){}
}

initialized class fields / static properties need semicolons when followed by generator methods and _ComputedPropertyName_s. If they don’t have the semicolon, it is a SyntaxError:
```
class F {
  g = h;  // h[... is a valid start of an AssignmentExpression → ;
  [i](){}
  static j = k;  // k*... is a valid start of an AssignmentExpression → ;
  *l(){}
}
```
initialized class fields / static properties need no semicolons when followed by normal methods or other class fields / static properties:
```
class M {
  n
  o = p
  static q = r
  s(){}
}
```

and to recap: while the second case is not ambiguous, it would make the grammar too complex, which is a no-go in the eyes of the comittee and wouldn’t be accepted.

can we all agree on this?

hansifer commented 8 years ago

It seems that something like a "require semicolons" directive could address the controversy, empowering devs to decide when to trade ASI for certain performance advantages.

ziemkowski commented 8 years ago

@jeffmo Seeing as you're removing this topic from the TC39 agenda, is there a resolution to this?

Am I correct in assuming that adding a new keyword or prefix, to limit the cost of parsing while also making the vars more explicit in what they do, is not an option? Something like prop

flying-sheep commented 8 years ago

wouldn’t work. the problem is computed names and generator functions coming after the static properties. and you can’t change their syntax anymore.

jeffmo commented 8 years ago

@flying-sheep's summary is correct. After chatting further with @allenwb this week, I think I might be to blame for lack of clarification and understanding of what was changed in Babel.

Per the grammar for this proposal, you can see that semicolons are required by the grammar. But when they are missing, ASI will attempt to fulfill that requirement automagically. There are cases where said ASI will succeed (i.e. class Foo { bar \n [qux]() { ... } } --ASI--> class Foo { bar; \n [qux]() { ... } }) and there are cases where it will fail (leading to a parse error). The misunderstanding seems to revolve around the fact that ASI will always try to place them for you but it will not always succeed (i.e. class Foo { bar=baz \n [qux]() { ... } } --ASI--> class Foo { bar=baz[qux]() ; { ... } --> Parse Error).

I will follow up with Babel and to get this fixed so that ASI works as expected there.

flying-sheep commented 8 years ago

yay!

legomind commented 8 years ago

Double yay!

flying-sheep commented 8 years ago

@jeffmo: how to move on?

we need to file a bug on phabricator that supersedes/inverts T6873 and T7040
we need to revert babel/babel#3225

right?

littledan commented 8 years ago

@jeffmo I'm concerned about this solution. Even if the spec text implies what @allenwb explained, that doesn't make it easy for implementations like V8 to do ASI based on such long lookahead/rewind. I'd prefer if we maintained the invariant that ASI is possible to do by looking ahead just one token.

jmm commented 8 years ago

@littledan There's no rewind -- ambiguous code without literal semicolon will be a syntax error.

littledan commented 8 years ago

Sorry, I must be misunderstanding the solution. Did you end up with the proposal at https://github.com/jeffmo/es-class-fields-and-static-properties/issues/26#issuecomment-171587367 or something else?

Will a semicolon be required for something like

class Foo {
  bar = baz
  [bing]() {}
}

following the baz?

On the other hand, will ASI be expected to not kick in for code like this?

class Foo {
  bar = baz
  [bing]
}

I'm not sure how to interpret the grammar that @jeffmo linked to to answer these questions.

Earlier posts by Allen seemed to be based around a model of ASI where the parser would end up seeing what syntax errors occur and inserting semicolons as appropriate (which is how the spec language is organized), rather than operationally inserting a semicolon based on looking at the next token after a newline (which is how V8 implements it--a correct implementation only because current syntactic constructs maintain the invariant that these two understandings match up).

There's a sentence in @jeffmo 's comment that I don't quite understand:

The misunderstanding seems to revolve around the fact that ASI will always try to place them for you but it will not always succeed (i.e. class Foo { bar=baz \n [qux]() { ... } } --ASI--> class Foo { bar=baz[qux]() ; { ... } --> Parse Error).

I don't actually see why ASI would put a semicolon there. Generally, what's the misunderstanding and how is it resolved?

michaelficarra commented 8 years ago

@littledan The answer to your first question is yes, a semicolon will be required there for a successful parse. The answer to your second question is no, this is syntactically valid all on its own due to ASI adding the semicolon before }.

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

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:

The offending token is separated from the previous token by at least one LineTerminator.

The offending token is }.

The previous token is ) and the inserted semicolon would then be parsed as the terminating semicolon of a do-while statement (13.7.2).

Unfortunately, @jeffmo's explanation is wrong since the offending token in his example is neither on the next line nor is it a }. So it's simply a parse error, and ASI never triggers.

allenwb commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

My mistake, thanks for clearing it up everyone.

dtothefp commented 8 years ago

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 https://github.com/jeffmo/es-class-fields-and-static-properties/issues/26#issuecomment-177720642 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 commented 8 years ago

Thanks for not breaking ASI, @jeffmo. :+1:

flying-sheep commented 8 years ago

@dtothefp don’t start calling people names.

about your points:

ASI exists. if it wouldn’t in this case, it still would elsewhere.
ASI does affect you even if you use semicolons, e.g. when forgetting that
```
return
   foo;
```
in fact returns undefined
think of ASI only when starting a line with punctuation and you’re good.

bakkot commented 8 years ago

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' ". :neutral_face:

flying-sheep commented 8 years ago

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 commented 8 years ago

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

tc39 / proposal-class-public-fields

use consistent semicolon elision rules #26

Updated status:

solutions

keep semicolons

allow everything

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