Skip to content
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

Information loss with current variable name binding model (JSON -> Json) #2551

Closed
nixxquality opened this issue Apr 8, 2024 · 6 comments · Fixed by #2556
Closed

Information loss with current variable name binding model (JSON -> Json) #2551

nixxquality opened this issue Apr 8, 2024 · 6 comments · Fixed by #2556

Comments

@nixxquality
Copy link
Contributor

nixxquality commented Apr 8, 2024

Environment

  • Elixir & Erlang/OTP versions (elixir --version): Erlang/OTP 26, Elixir 1.16.2
  • Operating system: Windows 10
  • How have you started Livebook (mix phx.server, livebook CLI, Docker, etc): mix phx.server
  • Livebook version (use git rev-parse HEAD if running with mix): cb11b26
  • Browsers that reproduce this bug (the more the merrier): All

Current behavior

Some background: I wanted to write an all-Erlang livebook, to check this project out, and to experiment with live documentation for a project of mine.
That project is a JSON parser, and so I used the variable name "JSON" for a binary with data that any reader could quickly edit.
However, I ran into an issue where the "JSON" term would disappear after one cell. I figured this had to do with the variable name normalization between Elixir and Erlang, and I've now checked the code to confirm those suspicions.

First: A simple test case.

<!-- livebook:{"default_language":"erlang"} -->

# Untitled notebook

## Section

```erlang
JSON = "[1,2,3]".
```

```erlang
JSON.
```

image

I hoped this would be an easy fix, but I took a look at the code, and it seems like variable names are always remembered in the Elixir form even if you only have Erlang blocks in a document.
Of course, when you convert "JSON" to "json", you have no idea what the capitalization is supposed to be when you turn it back. "JSON" or "Json"? In this case, Livebook chooses "Json".
image

So it's not an easy fix, it's a bit of a head scratcher and to me, being new to your codebase, this is out of my league.

I would like to hear your thoughts on the matter.

@josevalim
Copy link
Contributor

Thank you for the report. Maybe we will need to keep a mapping in the evaluator to know how to handle those. I also can't think of alternatives. :(

@nixxquality
Copy link
Contributor Author

Perhaps instead of keeping a list of bindings by name only, you could keep them with their original name and a tag suggesting an origin, which can be converted when necessary.
An example, in Erlang terms, would be:

Bindings = [{erlang, Name}, ...].

to_erlang_var({erlang, Name}) -> Name;
to_erlang_var({elixir, Name}) -> to_camel_case(Name).

@jonatanklosko
Copy link
Member

Good catch. There is more to it, the current conversion maps JsON and JsOn to the same Elixir name, so if both names were to be defined it can actually break the semantics.

I think we really need an unambiguous mapping. My vote is to convert JSON to j_s_o_n, this is not pretty, but only matters if the user writes both Erlang and Elixir. Besides, one may argue the variable should be Json anyway :) @josevalim wdyt?

@josevalim
Copy link
Contributor

I think we don’t need to worry about multiple Elixir vars converting to the same Erlang one, that’s fine. Just change your Elixir var name. But Erlang vars should go back and forth without issues.

@jonatanklosko
Copy link
Member

jonatanklosko commented Apr 9, 2024

Just change your Elixir var name.

Ideally we should not have conflicts in the conversion, I think that's confusing.

My point is that with an unambiguous mapping we achieve both and it seems like the simplest approach.

There are a couple more cases, because technically Erlang env vars can also have underscores. But I think the following would be unambiguous (same procedure both ways):

  1. Toggle the first char case
  2. _x <-> X

And this results in:

Erlang <-> Elixir

# Usual
Json <-> json
JSON <-> j_s_o_n
JsOn <-> js_on
JsON <-> js_o_n

# Mostly irrelevant edge cases
J_s_o_n <-> jSON
Js__on <-> js_On
Js__o_n <-> js_ON
JsO_N <-> js_oN
Js_on <-> jsOn

@josevalim
Copy link
Contributor

regardless of what I said, that’s by far the simplest option, so I agree. :)

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

Successfully merging a pull request may close this issue.

3 participants