Make external... = "" optional in reason syntax

[romanschejbal]

Fixes #2422

[jordwalke]

Great change!

[anmonteiro]

@romanschejbal I made another pass at this -- it seems that you still need to handle the .rei case.

Other than that this is really shaping up!

[anmonteiro]

I think this is good to go.

@romanschejbal thanks again for working on this. I think a good next step could be to parse external x: ... = ""; into external x .. = "x" and avoid having empty strings in the AST at all. I think this would get us rid of the bucklescript warnings.

[romanschejbal]

Thanks to you for the intro and help! And yes, that makes total sense. I will look into it when I get some time. :-)

[romanschejbal]

I think this one is now good to go. It removes those empty strings during parsing phase so we don't get bs warnings. Do you mind taking another look @anmonteiro ?

And sorry if the rebase messed up the code-reviewing.

Thanks

[jordwalke]

Thank you for your contribution @romanschejbal !

[bobzhang]

I think this is good to go.

@romanschejbal thanks again for working on this. I think a good next step could be to parse external x: ... = ""; into external x .. = "x" and avoid having empty strings in the AST at all. I think this would get us rid of the bucklescript warnings.

In bs, bs.obj expect string to be empty, that's why I made such feature request.
If you do the punning here, it will still make refactoring fragile: change x also change the underlying js names which defeats the purpose of warning

[jordwalke]

So what is the desired behavior? Should it default to empty if there is no string contents supplied?

[jordwalke]

I do like the behavior of having external x : typ; defaulting to = "x". It is really nice for c bindings, but I agree we cannot make a breaking change without coordinating a major version bump. In future versions of bs, is it possible to relax the requirement that the string content be empty in the AST? Is there a reason why bs.obj cannot ignore the string content? (Again, in the future - not right now because of the breaking change).

[bobzhang]

See my comments on punning: it is fragile to refactoring (both for C and JS)
So the desired behavior should be empty string, but I am unclear why it broke the CI since we did not use this feature yet

[romanschejbal]

Yes that's what's happening here. Anyway, I'm happy to create another PR rn that'll preserve the empty string if that's what you'll settle on. :)

[bobzhang]

@romanschejbal thanks for looking into it.

desired behavior

• external f : 'a => 'b ; is a short-cut for empty string
• no punning involved

[romanschejbal]

@bobzhang see the new PR pls, hope it's correct! :)