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

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

Open
ceremcem opened this issue Oct 11, 2018 · 19 comments
Open

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

ceremcem opened this issue Oct 11, 2018 · 19 comments
Labels

Comments

@ceremcem
Copy link

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
Copy link
Collaborator

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.

@rhendric rhendric added the bug label Oct 11, 2018
@ceremcem
Copy link
Author

So in the future, this will throw syntax error?

@rhendric
Copy link
Collaborator

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

@pepkin88
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

pepkin88 commented Nov 11, 2018

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
Copy link
Collaborator

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
Copy link
Contributor

: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
Copy link
Collaborator

rhendric commented Nov 12, 2018

...: 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
Copy link
Contributor

pepkin88 commented Nov 12, 2018

{...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
Copy link

@rhendric
Copy link
Collaborator

{...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
Copy link

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

@ceremcem
Copy link
Author

@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
Copy link
Contributor

@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
Copy link

@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
Copy link
Contributor

pepkin88 commented Nov 14, 2018

@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
Copy link

determin1st commented Nov 15, 2018

@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
Copy link
Contributor

@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.

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

No branches or pull requests

4 participants