Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrap variables/fields to clarify list and avoid ASI hazards #39

Closed
leobalter opened this issue Mar 16, 2018 · 25 comments
Closed

Wrap variables/fields to clarify list and avoid ASI hazards #39

leobalter opened this issue Mar 16, 2018 · 25 comments

Comments

@leobalter
Copy link

I like this proposal but I think there is still a gap for ASI hazards here in the var declarations, specially if someone wants to use ASI as a feature (omitting semicolons).

My suggestion is to wrap the variable declaration with some characters (here I'm using parentheses).

ClassElement [Yield, Await]:
  InstanceFieldsDeclaration [?Yield, ?Await]
  MethodDefinition [?Yield, ?Await]
  HiddenMethod [?Yield, ?Await]
  static MethodDefinition [?Yield, ?Await]
  static HiddenMethod [?Yield, ?Await]
  ClassInitializer
  ;

InstanceFieldsDeclaration [Yield, Await]:
  fields ( FieldsList ) [?Yield, ?Await]
  
FieldsList
  BindingIdentifier [?Yield, ?Await]
  FieldsList, BindingIdentifier [?Yield, ?Await]

I'm also using fields as the keyword here for the lack of a better idea for now, please replace it with anything else you find better.

class C {
  fields ( foo, bar )
}

I also imagine this can be extended with an initializer, but this part might depend on further discussions.

FieldsList
  BindingIdentifier [?Yield, ?Await]
  FieldsList, BindingIdentifier [?Yield, ?Await] Initializer[?Yield, ?Await]_opt

the current var declaration list avoids ASI hazards only as long it's not extended with initializers, wrapping them would allow it.

class A {
  fields (
    foo = 42,
    bar = "string"
  ) /* fields end here */
  *gen() {}
}

class B {
  fields (
    foo = 42,
    bar = "string"
  ) /* same line */ *gen() {}
}

class C {
  fields (
    foo = 42,
    bar = "string"
  ) [method]() {}
}
@allenwb
Copy link
Collaborator

allenwb commented Mar 16, 2018

Classes 1.1 is a Max-mim style proposal. As such it tries to eliminate problem areas and points of disagree by minimizing its feature "surface area". One of the reasons (there are multiple) for not including initializers in instance variable declarations and using a reserved word declarator name is to eliminate some of these ASI hazard.

Adding new syntactic conventions seems to go in the opposite direction.

Our intent was never to open the door to relitigating the design of the complete feature set in the currently active class extension proposals. Instead we asking how can we simplify? What features can be eliminated. What is the set of features that we all agree are necessary and design to support that set.

@leobalter
Copy link
Author

Adding new syntactic conventions seems to go in the opposite direction.

We can still cut off the initializer from my idea, and get the fields wrapping so we can afford extensions in the future.

InstanceFieldsDeclaration [Yield, Await]:
  fields ( FieldsList ) [?Yield, ?Await]
  
FieldsList
  BindingIdentifier [?Yield, ?Await]
  FieldsList, BindingIdentifier [?Yield, ?Await]

the problem we have with the current class fields and asi hazard today today is due to the initializers positioning mixed with other class elements.

I don't think the current proposed instance var declaration prevents that in any way better than the current class fields proposal.

@ljharb
Copy link

ljharb commented Mar 16, 2018

The features supported by the current proposals are the minimum; that’s what stage 2 signifies - that the committee agrees that solutions to those problems will eventually be in the language. Initialisers that allow omitting of the constructor, for example, is one of those.

@rhuanjl
Copy link

rhuanjl commented Mar 17, 2018

Another thing to bear in mind when people talk about a min-max proposal is that maybe it sounds like a nice short term solution BUT if/when it's accepted you're stuck with it.

You can't go back and alter it later:

e.g. if having a class field block or some other class field syntax would make other desired features more practical to implement excluding such now could block/make far more difficult any future implementation of those features.

@littledan
Copy link
Collaborator

My understanding is that TC39 decided to include ASI in class bodies, including with class fields, based on an understanding that the hazards weren't that bad. Given that, I'm not sure what problem this proposal is trying to solve.

@allenwb
Copy link
Collaborator

allenwb commented Mar 17, 2018

@littledan for example: tc39/proposal-class-fields#7 (comment)

BTW, ASI never has to be included, it is always there by default. If you don't what ASI in some situation it has be be explicitly excluded.

@littledan
Copy link
Collaborator

littledan commented Mar 17, 2018

We explicitly discussed this list of hazards and whether ASI should be included at the September and November 2017 TC39 meetings, and eventually reached consensus to include it, together with being more explicit about what ASI hazards are.

The idea at the time was to include the semicolon recommendation; I don't know if we should reconsider the ASI decision if we don't including the semicolon recommendation. But presumably the group of people who are opposed to the semicolon recommendation would also be opposed to forcing semicolon usage in class bodies.

@rhuanjl
Copy link

rhuanjl commented Mar 17, 2018

@littledan That list of hazards has been mentioned as one of the motiviations for this proposa (i.e. js-classes 1.1 - including the use of a keyword and the omission of initialisers for private fields) instead of the existing class related proposals.

In that context this issue makes some sense - (I disagree with all of this logic)

@hax
Copy link
Contributor

hax commented Mar 19, 2018

We explicitly discussed this list of hazards and whether ASI should be included at the September and November 2017 TC39 meetings, and eventually reached consensus to include it, together with being more explicit about what ASI hazards are.

The idea at the time was to include the semicolon recommendation; I don't know if we should reconsider the ASI decision if we don't including the semicolon recommendation.

Obviously the "semicolon recommendation" is not welcome by at least half part of the community. It's a tension between humans who support different semicolon coding style for years, TC39 is not very wise to add a new fire. As I commented in the related PR, there is a signal of interest conflict which TC39 should restrain itself.

But presumably the group of people who are opposed to the semicolon recommendation would also be opposed to forcing semicolon usage in class bodies.

I think I'm one of it. Note I'm not against any new ASI rule. For example, if we add limited initializer in this proposal, there is still new ASI issue before *gen() {}. But this is acceptable because it just add a simple new exception to the list of ([/-+. But tc39/proposal-class-fields#7 (comment) is unacceptablely crazy (especially the in/instanceof cases). It just ruins the whole semicolon-less coding style. I hope the members of TC39 respect all the efforts devote to simple and viable coding style in last 10 years.

@bakkot
Copy link

bakkot commented Mar 19, 2018

@hax,

I hope the members of TC39 respect all the efforts devote to simple and viable coding style in last 10 years.

We did: that's why class bodies as currently proposed do not require semicolons, leading to those edge cases. From talking to people in the community, I get the impression that those are considered acceptable in exchange for having initialized class fields and not requiring semicolons in bodies in general. I don't think most of those people would be happy with the outcome of simply not having public class fields, as this proposal is advocating, and would instead prefer the existing proposal, including the new edge cases for semi-free style.

We're not totally driven by community desires, of course. But if the new edge cases are bad because they would make people unhappy, I think that has to be weighed against how unhappy they would be with not having class fields, which is the main alternative.

@arackaf
Copy link

arackaf commented Mar 19, 2018

It's worth pointing out that destructuring introduced ASI hazard edge cases back in ES6, and life has gone on just fine. I'm struggling to understand why other ASI hazard edge cases in other new language features are suddenly a point of intense concern.

@leobalter
Copy link
Author

My suggestion is mostly the class elements list, where I honestly feel it's unnatural to use semicolons.

At some point, the () wrapping ends up just like adding a single ; at the end. So it's only a choice of the nicer evil to avoid ASI hazards. The biggest challenge I have here is: ; is already featured since ES2015. Semicolon is a feature and should be used for clarity whenever it's necessary. I don't like the idea of avoiding it at all costs because a specific code style rules them out.

In terms of overloading the specs, using a ; to separate names when it's necessary will be a feature we won't need the stress of roll through a whole staging process, it's already there.

My idea of wrapping is only a viable thing if we ever make it always required for fields/instance names. If it's optional it will be just another way to do the same thing ; already does, and I'm not interested to invest extra time on it this way.

@hax
Copy link
Contributor

hax commented Mar 20, 2018

@bakkot

We did: that's why class bodies as currently proposed do not require semicolons, leading to those edge cases.

Then unfortunately you might make it wrong. I'm sorry, but it's the truth. Let me explain it. (It will be a long post.)

Semicolon-less style doesn't simply mean omit semicolons as many as possible, but: you only need prefix semicolon as guard for the lines start with several tokens which very easy to understand and remember.

Note, even in some cases (like var a<NEWLINE>;[]) prefix semicolon is not necessary, we don't care about it. (That said, the absolute amount of semicolons is not the point of semicolon-less style.)

The tokens are:

  • (
  • [
  • + as unary plus (not addition)
  • - as unary negative (not subtraction)
  • / as start of regexp literal (not division or comments)
  • ` (should not forgot it, thank you @ljharb to remind)
  • * denote generator methods (not multiplication) -- we may add one if this proposal + class initializer landed

The beauty advantage of the semicolon-less style is, you only need check the first token of the line. Period.

The rule is so simple that it's very friendly for both coders and code reviewers (especially you are immune from OCD of always reading to end due to worrying about the possible missing semicolon in the end of each line).

It's also extremely easy to write a tool to enforce such style. I used a simple regexp based tool before ESLint maturity. I can't find it at hand now, but I'm very confident I can write such a tool in 5 minutes.

[Note, the next paragraph assume you need to read/write/review code in the situation that the linter is absent or not good enough.]
On the contrary, when you review a semicolon-must style code, you always need to check the end of each line. If you see no semicolon there, you may need to move your eyes back to the start and check the next line. In some bad cases you may turn your eyes several rounds. And a much trouble and very very common case is determine whether there should be a semicolon after final } token. You need to find whether it's a code block (which should not add semicolon), object literal (which should), class declaration (which should not), class expression (which should), function declaration (which should not), function expression (which should)... Sometimes you need to find the corresponding { 1000 lines before, you can try it in a github PR page.

Note, I do not want to advocate semicolon-less here (ok, I just did advocate it, but it's not my purpose). It's fine to use semicolon-must style with the protection of IDE/ESLint now, and we always have to switch semicolon style when contribute different open source projects using different style.

I just want to make it clear the core value of the semicolon-less style is the simplicity though it's not very obvious in first glance.

But the edge cases of current field proposal will ruin the simplicity by add these things to the token list:

  • in as field/method name
  • instanceof as field/method name
  • any class element which previous line is get set static as field

Note the last one is not a token at all. And it requires you to check the previous line. (in fact, it could be several lines before, which empty lines or comments lines should be skipped.)

This is very unacceptable, it's crazy to do such backward check for each class element, especially no one really use get set static field, but the FUD against "no-semi" style is just you will fail in some edge cases. So the only reasonable choice is do not rely on prefix semicolon in the last case, use normal ending semicolon for get set static field cases. But it break the consistency of prefix semicolon usage in semicolon-less style, and the "beautyadvantage" of semicolon-less style will become:

You only need check the first token of the line (6 special tokens in all places, 2 more keyword tokens in class body), comma, and there is an exception: use normal semicolon in get set static field.

Not so bad, hum?

Yes they are edge cases. But the whole semicolon style war is about edge cases and simplicity (both concept and practice).

It's clear to me that current field proposal will destroy the core value of semicolon-less style.

Note, I'm not saying, semicolon-less style is the most important thing which you should never touch, consider almost all JS programmers have been educated to use ESLint. I just hope TC39 guys (especially those who not use semicolon-less style) really understand the fact: current field proposal ruin the semicolon-less style, and please carefully calculate the price of breaking it.

From talking to people in the community, I get the impression that those are considered acceptable in exchange for having initialized class fields and not requiring semicolons in bodies in general.

Not everyone fully understand the core value of semicolon-less style, even they use it. You need to talk to the guys who really thoughtful in this issue, like, me 😜 .

And, do they know we still have the options that keep both? Like this proposal. Though initializer is not included, but we can add it later. And about public field, I'm not convinced it's a must-have. Even we want all, use a keyword like own also can avoid the destroy of semicolon-less style.

I don't think most of those people would be happy with the outcome of simply not having public class fields, as this proposal is advocating, and would instead prefer the existing proposal, including the new edge cases for semi-free style.

So I'm on the other side. I want to repeat my opinion, public field is questionable idea in OO languages like Java/C#. It's more problematic in JS because public field has similar but subtle different semantic with private field in current proposal. The practice of public field in TS/Babel is not sufficient enough to prove it's ok, because we never have private/hidden support, so no one really have chance to use more common pattern (wrap private state in public accessor) in other mainstream OO languages. We should first land private/hidden, then check the practise of the community, then next step. This is what max-min methodology can help.

Note, my against to landing public field in this aspect is not related to semicolon-less style problem. But it indeed make me frustrating that current proposal just do many things (public field semantic, semicolon problem, #priv syntax) wrong in only one time.

We're not totally driven by community desires, of course. But if the new edge cases are bad because they would make people unhappy, I think that has to be weighed against how unhappy they would be with not having class fields, which is the main alternative.

Yes, I totally agree TC39 should never only driven by community desires, TC39 should recognize what are the most important things. And make it right one by one.

It's not about how happy or how unhappy. This is totally wrong IMO. It's about why happy or why unhappy.

If someone unhappy because we do not have xxx feature, I don't think it's very important. As a old guy who start use JS in Netscape era, I have enough patience, and now we have Babel, TS and many altJS which can supplement programmers if they really need them.

But if someone unhappy because they think it's a feature designed in a wrong way, it's much important, even the reason is mainly aesthetic (like the voices against #priv), because it may cause community break.

My experiences in the observation of the language evolutions in past ten years told me TC39 mostly do the right decisions. But after ES6, it seems something changed in TC39, for example, it seems some members doubt max-min class methodology in Harmony development?! There are some signals that TC39 may lose the control in some way. But I see this proposal is a good fix, return to the right direction which I believe in.

Thank you for reading.

@hax
Copy link
Contributor

hax commented Mar 20, 2018

@arackaf

It's worth pointing out that destructuring introduced ASI hazard edge cases back in ES6, and life has gone on just fine. I'm struggling to understand why other ASI hazard edge cases in other new language features are suddenly a point of intense concern.

Array destructing introduced new ASI hazard only in the spec, but not introduce anything new in the practice of semicolon-less style. You can read my last post to learn the difference.

@ljharb
Copy link

ljharb commented Mar 20, 2018

@hax Don't forget a backtick, for tagged template literals! An easy class of mistake to make when using ASI - and in the future, there will likely be more of these easy mistakes to make.

@bakkot
Copy link

bakkot commented Mar 20, 2018

@hax,

The tokens are:

You've missed one. I only point this out because it showcases that even advocates of semi-free style like yourself tend not to be aware of all the existing edge cases. When they learn of a new one the response is in my experience always "oh, well, that never comes up, so it doesn't really bother me", which is exactly the same response when I have pointed out the new edge cases in the current proposal to users of semi-free style.

And, do they know we still have the options that keep both? Like this proposal. Though initializer is not included, but we can add it later.

Yes. I've talked to users of semi-free style about leading keywords, about disallowing uninitialized properties, and so on, and nearly universally the preference was for the proposal as it is.

Your opinion here - wanting both semi-free style and wanting not to have class fields - really is a very small minority.

We should first land private/hidden, then check the practise of the community, then next step. This is what max-min methodology can help.

Separately, I very much disagree with the idea of "we can add it later". Whether or not class fields are initialized crucially affects the overall design; the thing we'd get if we designed the feature on the assumption that there would not be initializers would almost certainly be strictly worse than the thing we'd get if we designed it coherently from the start.

The reason these features are currently tied together is because their design is tied together. This proposal would not be coherent if it included class fields as they're currently specified, so we cannot just do this incrementally.

There are some signals that TC39 may lose the control in some way.

I believe the only way that TC39 could lose control would be if browser vendors or other implementors lost faith in its process, and this proposal represents are far more significant risk of that than anything else in a long while.

@leobalter
Copy link
Author

The beauty of the semicolon-less style is, you only need check the first token of the line. Period.

I'm sorry, I lose my interest when I see arguments based on "beauty".

I'm not interested in continuing the suggestion I introduced in this PR.

@rwaldron
Copy link

I believe the only way that TC39 could lose control would be if browser vendors or other implementors lost faith in its process

Yes. This echoes the mistakes that lead to the failure of ES4. @bakkot thanks for shining a light on this.

@hax
Copy link
Contributor

hax commented Mar 20, 2018

@leobalter I'm sorry "beauty" make you unhappy, we could just change a word, like "advantage"? Anyway it's not my intention to advocate semicolon-less style here, I respect all the choices of different coding style. I just want to make it clear TC39 members understand the issue thoroughly.

@ljharb

Don't forget a backtick, for tagged template literals!

Yeah, I just forgot it! Hope the issue of tagged template literals hadn't happen. This is a good example of mistake. It should be tag [nonewline] template_literal but it may be too late to fix. At least it not as fatal as current field proposal, we just need to add ` to the token list.

and in the future, there will likely be more of these easy mistakes to make.

So this is the keypoint, how some new mistakes we will introduce and make the semicolon-less guys much hard to maintain their "advantage" of style.

Note not all "mistakes" are equal. The troubles introduced by current field proposal is much much fatal than ` case for semicolon-free style. It's a big Slippery Slope from ` to others.

@hax
Copy link
Contributor

hax commented Mar 20, 2018

You've missed one. I only point this out because it showcases that even advocates of semi-free style like yourself tend not to be aware of all the existing edge cases.

Yeah I become a little inertia after I am used to enjoy the protections of linters, and my brain is just tired when writing such long comments in a non-mother-language in my wee hours.

But what's wrong with that? I could rely on linters does not mean break the prefix semicolon usage in semicolon-less style is acceptable.

When they learn of a new one the response is in my experience always "oh, well, that never comes up, so it doesn't really bother me", which is exactly the same response when I have pointed out the new edge cases in the current proposal to users of semi-free style.

Yes. it doesn't bother me either, if I only consider the real world usage. But it just break the concept simplicity of semicolon-less style. Which is why semicolon-less style is created and how it be taught to others.

To be clear, I'm not here to say keep the core value of semicolon-less style should be the first goal of TC39, I just hope you TC39 guys notice the issue, understand the consequence of your decision, and realize there is a interest conflict between the language designers who want to add some very bad edge cases, with the community of semicolon-less style.

Your opinion here - wanting both semi-free style and wanting not to have class fields - really is a very small minority.

So you just provide them 2 options,

  1. break semi-free (a little) + class fields
  2. keep semi-free + no class fields

While I see never a fair way.

To be clear again,

  1. I'm not fully against field feature, I just think we'd better land private first, then see what happen.
  2. Semicolon issue is a separate concern, which could be easily solved by own keyword.

The "minority" is irrelevant here. Most JS programmers who have Java/C# background will tell you they know public field may be questionable in OO. But when they use TypeScript they alway use it because you don't have other choice. So if you ask them do you think public field is a good feature for JS, they will tell you YES! But this is just because you asked question in wrong way.

Separately, I very much disagree with the idea of "we can add it later". Whether or not class fields are initialized crucially affects the overall design; the thing we'd get if we designed the feature on the assumption that there would not be initializers would almost certainly be strictly worse than the thing we'd get if we designed it coherently from the start.

Yes I agree we need a overall design. But it not necessarily mean you need to deliver all features in one time.

The reason these features are currently tied together is because their design is tied together. This proposal would not be coherent if it included class fields as they're currently specified, so we cannot just do this incrementally.

Note, though @allenwb show his preference of no initializer, I believe this proposal have no difficulty to support limited C# like initializer if it's really desired. And I also believe decorators could provide functionalities like public field and initializer in some degree though the syntax @own(1) x may not optimal. I just don't understand why we need finalize many controversial designs now and deliver all such things in one time when you obviously can give programmers much chances to develop new possibilities.

I believe the only way that TC39 could lose control would be if browser vendors or other implementors lost faith in its process,

Like PTC vs STC? hum? Yes, TC39 process already failed in that case IMO.

and this proposal represents are far more significant risk of that than anything else in a long while.

To be honest, this is another bad signal for me that TC39 is lose control, that many members of TC39 refuse to admit there are big risks in current class field proposals:

  • Programmers hate #priv
  • Semantic of public field diff to private field, and also differ to all public field semantic in other mainstream languages
  • You will break semicolon-less style in a much more bad manner than `

@zenparsing
Copy link
Owner

@leobalter I'm glad you created this issue. Although ASI is not a problem for the current syntax without initializers, it will be a problem in two scenarios:

  • If we change the declaring keyword to hidden
  • If we add initializers at a later time

I like your suggestion of wrapping the hidden binding name list with delimiters, but the parenthesis don't feel right. But what about curly braces? We have a precedent for using curlies to wrap binding name lists: import declarations!

Check it out:

class C {
  hidden { x, y }
  constructor() {
    this->x = 0;
    this->y = 0;
  }
}

This syntax could be expanded to include initializers like so:

class C {
  hidden { x: 0, y: 0 }
}

What do you think?

@bathos
Copy link

bathos commented Mar 26, 2018

@zenparsing I find that a lot clearer than var! It seems to tick all the boxes — it’s extensible, ASI-friendly, and because it allows using hidden again, it helps establish the relationship to hidden methods and using -> — while the eye-catching syntax also helps keep the methods and binding names from seeming more similar than they are ("hey! this is something new to learn about!").

@hax
Copy link
Contributor

hax commented Mar 27, 2018

@zenparsing I find hidden { x, y } also make it a little bit consistent with methods that all class elements can be written in one line without semicolons. (eg. class A{hidden{x} a(){} static b(){}}) 😂

@MichaelTheriot
Copy link

MichaelTheriot commented Mar 27, 2018

Careful giving "instance variables" property syntax. It suggests these could work (for better or worse):

methods

hidden {
  method() { return 0; }
}

getters/setters

hidden {
  get getter() { return 0; }
}

computed names / symbols

hidden {
  [Symbol.for('something')]: 0
}

On the other hand, we do not slip up on destructuring syntax, so I think these can be disregarded. I think being able to declare methods in the hidden block would be useful too.

@zenparsing
Copy link
Owner

@MichaelTheriot

Careful giving "instance variables" property syntax. It suggests these could work

This is a good point. None of those (computed names or methods) should work, of course. For hidden methods, we want them to have the right home object (i.e. super reference).

To distance hidden {} from object literals we could use = for our hypothetical initializers:

class C {
  hidden { x = 0, y = 0 }
}

That might work, but the downside would be inventing (yet) another novel curly brace form.

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

No branches or pull requests