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

Change the semantic of MlBytes.toString #997

Merged
merged 2 commits into from
May 6, 2020
Merged

Change the semantic of MlBytes.toString #997

merged 2 commits into from
May 6, 2020

Conversation

hhugo
Copy link
Member

@hhugo hhugo commented Apr 23, 2020

Until now, MlBytes.toString() would return a utf16 javascript string.

This is becoming problematic now that we want to change the representation of ocaml string to use JavaScript strings.
The reason is that JavaScript VMs will transparently rely on the toString method when converting a value to a primitive type. For example, the following snippet will behave differently depending on how we represent strings.

let get o (name : string) = Js.Unsafe.get o name
function get(o, name) { return o[name] }
  • If implemented like ocaml bytes, a conversion from utf8 to utf16 will happen (the vm will call toString)
  • If implemented like a javascript string, no conversion will happen

This PR propose to change the semantic of MlBytes.toString() to not apply the conversion to utf16 and return the raw sequence of bytes.

@hhugo hhugo requested a review from TyOverby April 24, 2020 15:00
@hhugo hhugo added this to the 3.7 milestone Apr 24, 2020
@@ -425,9 +425,14 @@ MlBytes.prototype.toString = function(){
}
this.t = 8; /*BYTES | NOT_ASCII*/
case 8: /*BYTES | NOT_ASCII*/
return caml_utf16_of_utf8(this.c);
return this.c;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the type of this.c?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the top of that file. It describes the memory representation of mlBytes

@hhugo hhugo merged commit 90c2ad2 into master May 6, 2020
@hhugo hhugo deleted the mlbytes-toString branch May 6, 2020 21:35
@dbuenzli
Copy link
Contributor

If I understood correctly @hhugo's explanations in the xrefs, a side effect of that PR is that caml_js_{get,set,delete} can be given for their string argument either:

  • A JavaScript string
  • An US-ASCII OCaml string

I had naively expected that caml_js_object and caml_js_meth_call would behave the same way. That is however not the case since they call caml_jsstring_of_string on their string arguments and thus really expect an OCaml string.

Is that expected behaviour or something that was overlooked ?

@hhugo
Copy link
Member Author

hhugo commented Sep 21, 2020

Looking at the Js_of_ocaml.Js implementation:
caml_js_{set,get,delete} are polymorphic on all their arguments and results.
caml_js_object and caml_js_meth_call expect an ocaml string.

The inconsistency is unfortunate. One could imagine introducing more primitives to have versions of caml_js_object and caml_js_meth_call expecting JavaScript string

facebook-github-bot pushed a commit to facebook/flow that referenced this pull request Jun 26, 2022
Summary:
Changelog: [fix] The JS build for try-flow is now fixed. It's recently broken since the 0.181.0 release.

We don't wrap the input arguments with `Js.to_string`, so we are likely impacted by the string representation change (See ocsigen/js_of_ocaml#997) since our js_of_ocaml 4.0 update in the last release.

Fix #8899

Reviewed By: pieterv

Differential Revision: D37439740

fbshipit-source-id: 8536c6d0ac36b2214cafc5dbaf3daf0e9bece55f
facebook-github-bot pushed a commit to facebook/flow that referenced this pull request Jun 26, 2022
Summary:
Changelog: [fix] The JS build for try-flow is now fixed. It's recently broken since the 0.181.0 release.

We don't wrap the input arguments with `Js.to_string`, so we are likely impacted by the string representation change (See ocsigen/js_of_ocaml#997) since our js_of_ocaml 4.0 update in the last release.

Fix #8899

Reviewed By: pieterv

Differential Revision: D37439740

fbshipit-source-id: f26a762f358d0539301b96b88d45ff110f5e3c32
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 this pull request may close these issues.

3 participants