Make "external f: 'a => 'b;" shortcut for empty string

[romanschejbal]

This is a followup to #2464 based on the discussion in there.

[anmonteiro]

this is regressing from #2464 in that the printer shouldn't print the = "render"; part

[romanschejbal]

I'm not sure as the input for this particular output looks like this

/* "react-dom" shouldn't spread the attribute over multiple lines */
[@bs.val] [@bs.module "react-dom"]
external render : (reactElement, element) => unit = "render";

And the tests are passing, too. There's something else failing on the CI though which I don't understand.

[anmonteiro]

I think we agreed in the last PR that if the ident and the primitive name agree we can drop it on the floor, but I could be misremembering

[romanschejbal]

Or did I misunderstand the assignment from Bob? 😄

[romanschejbal]

I would like to drop it if they are the same too btw. But I don't know all the in & outs of bs.

[anmonteiro]

Ah I didn’t read the discussion in the other issue.

I disagree with the outcome, but oh well.

[ryyppy]

I think you all know this, but I still want to write this out for anyone following this discussion.

@anmonteiro I think we agreed in the last PR that if the ident and the primitive name agree we can drop it on the floor, but I could be misremembering

With this behavior, it would be a breaking change for BuckleScript.
The recommendation is to always explicitly write the string value, even if the identifier matches the value itself.

[@bs.val] [@bs.module "react-dom"]
external render : (reactElement, element) => unit = "render";

Previously, you could leave it out blank as well, but this will now yield a warning that this is discouraged. As @bobzhang said, the reason is to prevent binding errors due to quick renaming of the identifier (with the punning, you wouldn't even know what's going on, especially if you are newcomer and never used the string representation for some reason).

Why does [@bs.obj] require a = "" then? This seems to be documented here:

[@bs.obj] external route: (
  ~_method:string,
  ~path:string,
  ~action:(list(string) => unit),
  ~options:Js.t({..})=?,
  unit
) => _ = "";

Note: the = "" part at the end is just a dummy placeholder, due to syntactic limitations. It serves no purpose currently.

So i guess the whole goal of this general external syntax feature is to get rid of this confusing [bs.obj] syntax?

[bobzhang]

The diff looks good to me.
Note the old PR changed the semantics that's why it broke our CI.
I argued for dropping the punning since it makes refactoring fragile (for both C and JS) but have no strong opinions -- we used to like punning but figured it does not work with editor based refactoring (cc @chenglou)

[romanschejbal]

Thanks for the clarification @ryyppy 👌

Can someone point me in the direction of how can I make the CI happy? Menhir version seems to be incompatible?

[cristianoc]

CI breaks for unrelated reasons (also happens on a change-nothing diff which used to build OK).

[jordwalke]

@anmonteiro I think you and I agree on what would be ideal here. Punning is nice because if you omit a string, there's really only one thing you could ever want it to be - punned. The only exception is if you have to be using the current version of bs's ppx (which many are). Since breaking people isn't an option, we need to find a way to get to the ideal state without breaking people. Do you have an alternative suggestion to get to the ideal state of punning without breaking anyone?

There's a couple of changes that we are making a list of that need to be coordinated with a bs release. One is fixing label mangling, and the other is this punning. I'm keeping a list of these things.

[jordwalke]

@bobzhang What would be involved in making the next release of bs gracefully ignore the contents of the string when using bs.obj? It seems like that would be a non-breaking BS change, that allows us to add this feature back. Without bs being graceful in allowing that, it is currently eating up a nice syntactic real estate for non-bs use cases (needlessly?)

[TheSpyder]

With this update, isn't the change list for 3.6.0 incorrect?

external myFn: (string) => unit is now equivalent to external myFn: (string) => "myFn"

(putting aside the typo, it should read external myFn: (string) => unit = "myFn")

I saw that and expected to be able to remove the = "myFn" from my code but I still get BS warning 105. This PR explains why.

[ryyppy]

@TheSpyder jup, i opened a PR a few days ago #2540 ... hasn't been merged yet