Skip to content

[WIP] Allow foo.'bar-bat' and foo.1 to access object members #3828

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

Closed
wants to merge 2 commits into from

Conversation

pscollins
Copy link

This change addresses #3520. I wasn't sure if there is a consensus on that issue, but I think that it's a simple change that could clean up code in some places.

@epidemian
Copy link
Contributor

👍

This makes for a nice symmetry with literal object notation.

obj = 
  # Can use barewords for keys that look like identifiers.
  foo: 42
  # But need enclosing commas for non-identifier-like keys.
  'wordy key': 43

# Can access identifier-like keys by a bareword after the dot.
obj.foo # => 42
# But need enclosing commas for accessing non-identifier-like keys.
obj.'wordy key' # => 43

@jashkenas
Copy link
Owner

I'm not sure really what to think about this. There is a certain symmetry, but there's also a certain awkwardness, especially when it comes to more complicated accesses:

configuration.'ajax-get-request'(url, options)

... that's just odd. Any staunch defenders of this idea?

@vendethiel
Copy link
Collaborator

It works in coco/LS, but I admit I don't use it that much. It's only interesting if you have to deal with "strange" json schemas...

@lydell
Copy link
Collaborator

lydell commented Feb 4, 2015

If this is going to be supported, numbers should be as well. a.1

@michaelficarra
Copy link
Collaborator

@lydell: Agreed. I'm 👍 for both of them.

@pscollins
Copy link
Author

@lydell maybe it would be easiest to do something like

PropertyIdentifier ::= Identifier | AlphaNumeric 

and then replacing the existing uses of Identifier with PropertyIdentifier in Accessor?

@lydell
Copy link
Collaborator

lydell commented Feb 5, 2015

especially when it comes to more complicated accesses:

configuration.'ajax-get-request'(url, options)

... that's just odd.

I always try to avoid the a[b](c) pattern, because IMO it is not very readable. configuration.'ajax-get-request'(url, options) isn’t more odd than configuration['ajax-get-request'](url, options) or the syntactically valid JS "string"().

@jashkenas
Copy link
Owner

Alright then. I'm persuaded. Let's add numbers to this patch, and some more robust tests, and merge.

@michaelficarra
Copy link
Collaborator

😀

@lydell
Copy link
Collaborator

lydell commented Feb 6, 2015

@pscollins Please do what you suggested and add tests for all the access variants (not just .) and tests that you can call such accesses as well as access deeper properties. Also test heredocs.

@pscollins
Copy link
Author

@lydell :: and ?:: as well, or just . and ?.? My guess is no, since things accessible through :: won't have been written as object literals, and so shouldn't have crazy names.

@michaelficarra
Copy link
Collaborator

@pscollins: Of course those should be allowed.

a::' ' = ->
a.prototype = ' ': ->

@jashkenas
Copy link
Owner

Yes. If it's for any accessor, it should be for all accessors. And tested ;)

@pscollins
Copy link
Author

I've got it working and tested for every form except

 obj.NUMBER 

It's listed in the testcases I just pushed, commeted out. Uncommenting it yields:

test/objects.coffee:474:9: error: unexpected .42
eq obj.42, 3

I assume that what's happening is Lexer#numberToken is consuming the . and lexing it as IDENTIFIER NUMBER rather than IDENTIFIER . NUMBER.

I'm not sure how this could be dealt with in the parser. The best case I can imagine would be adding in a special IDENTIFIER . [0-9]+ case in the lexer that comes before numberToken is called, but that seems a little hacky.

It's also worth noting that something like

obj = 
  4.1: 1
  4:
    1: 2

will be ambiguous.

What's the best way to approach it? Make a special case for numeric accessors, and forbid non-integers? Require that numeric accessors go in parens? Restrict it to strings?

For comparison, it looks like coco basically forbids non-integers:

obj =
  4: 1
  5: 2
  5.1: 3
  6:
    1: 4

console.log obj.4 // 1
console.log obj.5.1 // undefined
console.log obj.6.1 // 4

but that seems really confusing to me.

@jashkenas
Copy link
Owner

Re: the parser error, I don't know — but as to your second question...

obj = 
  4.1: 1

... would simply be:

obj = 
  "4.1": 1

no? I don't see the ambiguity.

@pscollins
Copy link
Author

@jashkenas Sorry, I didn't mean that defining the object would be ambiguous -- I meant accessing it would be ambiguous.

My worry is that obj.4.2 can be interpreted either as obj[4][2] or obj[4.2]. If your object looks like obj = {4.2: "foo"}, you're going to be confused if obj.4.2 throws an error for trying to access the property 2 on the undefined obj[4]. If your object looks like obj = {4: {2: "foo"}}, then you're going to be confused if obj.4.2 gives you back undefined for trying to access obj[4.2].

Or you might have

obj = 
  4:
    2: 1
  4.2: 2

Now, what should obj.4.2 be? 1, or 2?

It seems to me like it would be easy for someone new to the language to get tripped up by floating point numbers behaving differently from other kinds of numbers, but maybe I'm overreacting?

@michaelficarra
Copy link
Collaborator

It should be a decimal integer, not floats, not hex, not scientific notation, etc. So my answer is it would be obj[4][2].

o '. AccessorIdentifier', -> new Access $2
o '?. AccessorIdentifier', -> new Access $2, 'soak'
o ':: AccessorIdentifier', -> [LOC(1)(new Access new Literal('prototype')), LOC(2)(new Access $2)]
o '?:: AccessorIdentifierIdentifier', -> [LOC(1)(new Access new Literal('prototype'), 'soak'), LOC(2)(new Access $2)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's correct in the compiled JS though. Remember to rebuild everything.

@lydell
Copy link
Collaborator

lydell commented Feb 7, 2015

I think a . b is valid, so there should be tests for a . 'abc' too

@lydell
Copy link
Collaborator

lydell commented Feb 7, 2015

and tests that you can call such accesses as well as access deeper properties

You forgot this.,

# identifiers, string literals, and numbers.
AccessorIdentifier: [
o 'Identifier'
o 'AlphaNumeric'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible solution for the number problem could be to replace this line with:

o 'STRING'
o 'INTEGER'

Then, in AlphaNumeric replace o 'NUMBER' with o 'Number' and add:

Number: [
  o 'NUMBER'
  o 'INTEGER'
]

and adjust the NUMBER regex in the lexer to match integers separately from other decimal numbers and make Lexer::numberToken tag integers as such.

@michaelficarra
Copy link
Collaborator

I forgot we even allowed leading dots in number literals. Who thought that was a good idea?

@lydell
Copy link
Collaborator

lydell commented Feb 7, 2015

I forgot we even allowed leading dots in number literals.

Me too. That’s really making things more complicated here.

@lydell
Copy link
Collaborator

lydell commented Feb 7, 2015

I think the best way to solve it is to add return 0 if number.charAt(0) is '.' and @tag() in INDEXABLE in Lexer::numberToken.

@jashkenas
Copy link
Owner

I forgot we even allowed leading dots in number literals. Who thought that was a good idea?

It's not a good idea ;) We shouldn't have let that sneak in.

@lydell
Copy link
Collaborator

lydell commented Feb 10, 2015

See #383.

@jashkenas
Copy link
Owner

It should be a decimal integer, not floats, not hex, not scientific notation, etc. So my answer is it would be obj[4][2].

Agreed.

@lydell
Copy link
Collaborator

lydell commented Feb 10, 2015

We shouldn't have let that sneak in.

That makes it sound like they were never intended but there were no tests for it and during some refactor they were allowed by mistake. Since that's not the case, do you think that we should remove them (which makes this PR a lot simpler to finish, but likely results in 10 duplicate issues the same day it is released) or let them stay?

@jashkenas
Copy link
Owner

Ha ha. There's a certain class of feature (of which there are dozens of examples in CoffeeScript) that I intentionally didn't include at the beginning because they're not my personal preferred style ... but that managed to sneak in over time because people ask for them, and you say "why not?".

This was one of those.

I don't think we can remove them at this point. But perhaps we could tag them specially in the lexer (or something) to make this change easier.

@michaelficarra
Copy link
Collaborator

Numbers with a leading . instead of 0. are exceptionally uncommon in JS, and I bet even more so in CoffeeScript. Same goes with numbers with trailing .. I think we can safely remove them with a simple comment in the changelog.

@jashkenas
Copy link
Owner

I'd love it if that were really the case. My anecdotal sense is that lots of folks prefer to write speed + 0.5 as speed + .5.

Can we employ Github's code search to find out if folks are using it in CoffeeScript?

@lydell
Copy link
Collaborator

lydell commented Feb 10, 2015

At least in CSS I’ve seen lots of .5s. (Don’t know about JS though.)

@lydell
Copy link
Collaborator

lydell commented Feb 20, 2015

#1982 (comment)

@lydell
Copy link
Collaborator

lydell commented May 1, 2015

Just found that this would close #1334 as well.

@shreeve
Copy link
Contributor

shreeve commented Oct 2, 2016

Any status on this PR? Seems like the issue was related to a possible breaking change if some people used numbers like this: x = y + .25

Is breaking this use case the reason for this PR not being accepted?

@lydell
Copy link
Collaborator

lydell commented Oct 3, 2016

  1. It seems like it was decided that disallowing .5 etc. is too much of a breaking change to be a real option.
  2. Some trickery is required to get foo.1.2 working.
  3. There are lots of merge conflicts.

@pscollins
Copy link
Author

@shreeve IIRC I had trouble getting it to work in the .5 case and I got busy with other projects --- feel free to revive this/close this PR for a new one/etc.

@GeoffreyBooth
Copy link
Collaborator

FYI we should probably put this on hold until tagged template literals are supported, unless we’re certain that there won’t be any ambiguity between this proposed syntax and the syntax we’ll need for that. I would think tagged template literals should have a much higher priority if we’re forced to choose between them.

@GeoffreyBooth
Copy link
Collaborator

@pscollins or anyone on this thread, I’d love to get this updated and targeted against the 2 branch. It seems like it was rather close?

@GeoffreyBooth GeoffreyBooth changed the title Allow foo.'bar-bat' to access object members. Closes #3520 Allow foo.'bar-bat' and foo.1 to access object members. May 1, 2017
@GeoffreyBooth GeoffreyBooth changed the title Allow foo.'bar-bat' and foo.1 to access object members. Allow foo.'bar-bat' and foo.1 to access object members May 1, 2017
@GeoffreyBooth GeoffreyBooth changed the title Allow foo.'bar-bat' and foo.1 to access object members [WIP] Allow foo.'bar-bat' and foo.1 to access object members May 1, 2017
@GeoffreyBooth
Copy link
Collaborator

What’s going on with this? Is this something we’re still interested in getting finished, or should I close this as an abandoned effort? If we are trying to finish it, are there any breaking changes that would result from merging this in?

@michaelficarra
Copy link
Collaborator

I can't speak to the quality of this PR, but this is a great feature and should be pursued.

@GeoffreyBooth
Copy link
Collaborator

At this point the codebase has changed so much since this PR was started that a new PR will be necessary, so I’m closing this one. I’m leaving issue #1334 open as a reminder that we’re interested in pursuing this; that issue has a link to this thread.

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

Successfully merging this pull request may close these issues.

8 participants