gkz / LiveScript

LiveScript is a language which compiles to JavaScript. It has a straightforward mapping to JavaScript and allows you to write expressive code devoid of repetitive boilerplate. While LiveScript adds many features to assist in functional style programming, it also has many improvements for object oriented and imperative programming.
http://livescript.net
MIT License
2.31k stars 156 forks source link

Strange output for x = a.b.c: d #1072

Open ceremcem opened 5 years ago

ceremcem commented 5 years ago

Compiling following code:

x = a.b.c: d

produces this:

var x;
x = {
  c: a.b.d
};

Where I would expect something like this:

var x;
x = {
  a: {
    b: {
      c: d
    }
  }
};

...or a proper exception. Is this a bug or something expected?

rhendric commented 5 years ago

What's happening here is that the :, since it appears outside of braces, is triggering the implicit braces rewriter, which converts the input code into:

x = a.b.{c: d}

That's an object slice, and it's correctly compiling to what you see produced.

Personally, I do think that this is pretty confusing to people who aren't expecting it, and I don't see anything in tests or the docs that suggest that this particular interaction is intended, so I'm inclined to agree that this should be a compiler error instead.

Note to future self (or someone else fixing this): . is not the only token that should prevent the insertion of implicit braces. Other tokens like ? and @, when followed by an unspaced c: d, lead to Unexpected '{' parse errors, which should also be avoided if possible since there is no { in the source. A reason (the only reason?) those cases result in (confusing) errors instead of also silently (and confusingly) becoming object slices is that the @adi! call in do-ID is skipped by property keys, and the dot is necessary to trigger slice syntax.

ceremcem commented 5 years ago

So in the future, this will throw syntax error?

rhendric commented 5 years ago

Probably, unless someone talks me out of it, and assuming I or someone else gets around to making it so.

pepkin88 commented 5 years ago

If any character deserves to be an exception to be allowed to be a part of an identifier, without quotes, it should be ..

The reason is pragmatic, as . is heavily used in keys of MongoDB query objects, and possibly other RDBMs/ODMs. Every time I need to reference some nested field in a Mongo document, I need to quote the whole key (or use a backslash string with a space at the end, not to connect with the :) - not very convenient.

So if it comes to me, I would opt for x = a.b.c: d being just x = 'a.b.c': d. It's unambiguous enough and I think the only reason it isn't already that way, is because the previous languages (JS, CS, Coco) didn't have that.

rhendric commented 5 years ago

That's a pretty reasonable proposal. I want to be cautious about adding little inconsistencies like that, so I'd like to take a little longer to think it over, but you've made some good points.

In terms of the implementation, there's still a bug in the brace insertion logic in the lexer that needs fixing, which is separate from hacking a.b.c into being a valid property key. (Consider, for example, a.b .c: d, which I think should throw a syntax error even if a.b.c: d doesn't.)

pepkin88 commented 5 years ago

Going back to my proposal for a moment: one place, where it might be ambiguous, is property shorthand {a.b.c}, where it is {c: a.b.c} and not {'a.b.c': a.b.c} (but that one doesn't make much sense, left side is a single string, right side is a 3-part expression, there are no variables named a.b.c). So I'd leave {a.b.c} to be {c: a.b.c}.

rhendric commented 5 years ago

Yes, agreed.

There's also the weird case of ...: o, which is basically the same as {...o}, but permits the programmer to avoid braces. Clearly this should not regress to {'...': o}, so our rules for allowing dots in property keys shouldn't be so generous as to include that case—I would probably say that the first character in a property key can't be a dot. Hyphenated property keys also deserve a thought—foo-bar: x becomes {fooBar: x}, so should foo-bar.baz: x become {'fooBar.baz': x}? I think so.

pepkin88 commented 5 years ago

:O Interesting, I didn't know that ...: o is a thing. Is it a side effect or an intended feature? I didn't see that documented. Hmm, hyphens, yeah, they probably should camelCase themselves. And in case it is unwanted, should be used with quotes, just like in cases without dots. I often forget about the hyphenated vars, because I don't use them.

rhendric commented 5 years ago

...: o was a Coco feature; it shows up in test/literal.ls. It's not documented, but I don't think {...o} is either, so...

pepkin88 commented 5 years ago

{...o} is a JS feature now. I was trying to gauge how likely ...: o is to stay, should I rely on it or not.

determin1st commented 5 years ago

https://jsperf.com/object-spread-vs-polyfill/1

rhendric commented 5 years ago

{...o} wasn't supported by any JavaScript engine I know of when those docs were originally written, was my point. But no matter; yes, if something is explicitly in the tests, I expect to continue to support it unless there's an obvious flaw, so you can rely on ...: o for the foreseeable future.

determin1st commented 5 years ago

hey, what about a little optimization for the import$ function? is it really needed to do var own = {}.hasOwnProperty; assignment before the cycle?

ceremcem commented 5 years ago

@rhendric I think the reason for {...o} hasn't been documented might be that it's a little bit confusing feature (to me, at least). It performs a clone only for the first level, but rest of objects are only assigned with pointers:

x = {a: 1, b: {c: 2}}
y = {...x}  # => same as x
x.a = 5 # => y.a is still 1
x.b.c = 4 # => y.b.c is 4

It might be implemented because of compatibility reasons, however it seems an accurate call to exclude it from documentation.

pepkin88 commented 5 years ago

@ceremcem AFAIK there is no built-in deep cloning features in LS or any of the ancestor languages (JS, CS, Coco), there's only an undocumented deep comparison ===, so I'm not sure why would you expect deep cloning there. It's also analogous to array spread [...a], which also works on the first level only. LS's implementation of {...o} is quite compatible with JS's definition (although it triggers setters, which is a very minor detail), so no big surprise there. So I don't think it should be the reason to keep it from being documented.

@determin1st While we're at it, we could do like Babel is doing and take Object.assign as import$, with a fallback implementation.

determin1st commented 5 years ago

@pepkin88 Object.assign is slower than import$$ in that test, why do you choice it? Im not quite clear about "fallback implementation" you mentioned, what is that?

pepkin88 commented 5 years ago

@determin1st Speed depends on the engine implementation. My current browser shows that Object.assign is only 9% slower than your import$$ and native spread being the slowest, while the table below shows how much native spread is crushing on Chrome 70. But more importantly, import$$ doesn't work with objects which don't have hasOwnProperty in their prototype chain, e.g. those created with Object.create(null). Why choose Object.assign? Because it is already implemented in most cases, so there is no need to create some additional function in every module that uses import$. A fallback implementation is needed, when there is no Object.assign. See arrayFrom$ ([...a]).

determin1st commented 5 years ago

@pepkin88 okay, checkout another import$$$ variant: https://jsperf.com/object-spread-vs-polyfill

How do livescript detect that there is no Object.assign? at runtime? hmm.. i've checked, yes. Is it really hard to make all things constant at compile time? to make some compile option/flag that will generate different code?

pepkin88 commented 5 years ago

@determin1st Yeah, looks good. Yes, it's doing it at runtime, as you said doing it at compile time requires some flags, so without that no code pruning opportunity there.