search

reasonml / reason

Simple, fast & type safe code that leverages the JavaScript & OCaml ecosystems
http://reasonml.github.io
MIT License
10.14k stars 428 forks source link

Fix uncurry attribute on function application #2566

Closed anmonteiro closed 4 years ago

anmonteiro commented 4 years ago

fixes https://github.com/facebook/reason/issues/2565

cc @bobzhang

bobzhang commented 4 years ago

A bit confused why this is not resolving in the parsing stage instead of post-processing

anmonteiro commented 4 years ago

@bobzhang could you clarify your question? The change I made is in the parser

bobzhang commented 4 years ago

My question is that when parser see f(. a,b), it parse it as uncurried function, why a post-processing is involved here?

anmonteiro commented 4 years ago

I'm not sure, looks like legacy code that's moving attributes from the expression to the structure item.

bobzhang commented 4 years ago

For this particular patch, you can use List.exists

anmonteiro commented 4 years ago

I can certainly switch to that, though I wanted to make the diff as easy to review as possible.

The bug here is that we were checking the entire expression's attributes (could be a let .. in) instead of the application expression's attributes for an uncurry attribute.

bobzhang commented 4 years ago

do you have an example of what mkstrexp is trying to achieve? I am a bit lost here Maybe you can add a test case

bobzhang commented 4 years ago

ping?

anmonteiro commented 4 years ago

happy to merge if you're happy with it. I don't have bandwidth to get rid of mkstrexp, or dig through history to find why it was added in the first place.

Otherwise I'm also happy to close this PR and let you fix the issue.

bobzhang commented 4 years ago

it seems to be introduced by this commit when introducing uncurried support: dabe031 @IwanKaramazow would you have a look why we need change mkstrexp here? thanks

IwanKaramazow commented 4 years ago

Some context: [@attr] expr is Pstr_eval (expr, @attr). The purpose of mkstrexp is building a Pstr_eval that contains the attributes, not the expression inside the Pstr_eval. I misunderstood this back in the days when implementing the original uncurried support. [@bs] should not be lifted to the Pstr_eval level. I'm wondering if this branch https://github.com/facebook/reason/blob/535eb95cacef59eb6dc1ca8c34d053d16ba089df/src/reason-parser/reason_parser.mly#L432-L449 can be removed. Printer will need to be fixed.