Fix #4533: chained calls incorrectly wrapping enclosing implicit objects #4534
Conversation
|
LGTM. Do you think it’s enough of a breaking-change risk that we should make this only on |
|
I think this should be fixed in |
test/formatting.coffee
Outdated
| a: id 1 | ||
| b: id {a: 42} | ||
| .a | ||
| eq 42, result.b |
There was a problem hiding this comment.
This test would pass if it was parsed as id({ a: 42 }.a) or id({ a: 42 }).a.
There was a problem hiding this comment.
Good catch, let me fix that.
|
Can you please explain how this would be a breaking change? |
|
This is a breaking change in the sense that code which previously compiled now compiles to something else. Luckily the previous compilation was not really meaningful, so no one should have hopefully relied on it. Changes like these are rather common here. |
|
FYI @xixixao I reviewed all the open issues. I put “Chaining:” in the titles for ones involving chaining or leading dots: https://github.com/jashkenas/coffeescript/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20Chaining Do you mind reviewing those after you’re done with this? I bet most of them are related. I also put “Lexer:” in the title for anything that seemed like it had to do with the code in |
|
I've already done that, I'll put the summary here later. I'm definitely no expert in any part of the compiler. |
xixixao
force-pushed
the
chains-in-object-literals
branch
from
74bf845 to
253e9d4
Compare
xixixao
force-pushed
the
chains-in-object-literals
branch
from
253e9d4 to
4b5e777
Compare
|
Rebased and fixed tests. |
|
@lydell what do you think? |
There was a problem hiding this comment.
I have always had trouble understanding the rewriter. This is a nice-looking little patch. @xixixao probably knows best how chaining works in CoffeeScript. I can't really tell what this patch does, but if the tests pass and this fixes a bug, then I'd say go for it.
Fixes #4533.
I looked through the other open and closed method chaining issues and this doesn't seem to fix any of them. It's possible it fixes some other issues I'm not aware of (I kinda feel like it should change some behavior, but we don't have any existing tests covering it).