Skip to content

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented Sep 6, 2025

First step in cleaning up string processing in the compiler. Define a unified String_kind.t as follows (replacing the existing delim type), make it mandatory and use it throughout the compiler pipeline (with the exception of the Parsetree for now).

    type t =
      | Standard (* Normal JS string with escaping *)
      | Verbatim (* Literal quoted string for tags *)
      | RawJs (* Raw JavaScript expressions *)
      | Template (* Template literals *)

Copy link

pkg-pr-new bot commented Sep 6, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7852

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7852

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7852

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7852

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7852

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@7852

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7852

commit: 7379d01

@cknitt cknitt requested review from tsnobip and cristianoc September 6, 2025 11:43
@cknitt cknitt marked this pull request as ready for review September 6, 2025 11:43
@cristianoc
Copy link
Collaborator

@codex review this change and explain the use of the various kinds of strings.
Is this just a refactor or could it change anything observable?
Make comments about style, clarity and possible simplifications.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

@cknitt
Copy link
Member Author

cknitt commented Sep 6, 2025

@cristianoc For now it should be just a refactor that shouldn't change any behavior.
I left out the Parsetree on purpose here as that's where things started breaking. 🙂
Can continue there later in a separate PR.

and property_map = (property_name * expression) list
and length_object = Js_op.length_object
and delim = External_arg_spec.delim = DNone | DStarJ | DNoQuotes | DBackQuotes
and string_kind = String_kind.t = Standard | Verbatim | RawJs | Template
Copy link
Member

Choose a reason for hiding this comment

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

The current naming (DStarJ and such) is pretty bad, but I'm not a big fan of Standard and Verbatim, Verbatim is a double quoted string right? First time I hear about verbatim for strings but might be more common in other languages. But what about standard, what does it mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll revisit this next week. Maybe I'll split the PR into two to separate the renaming from the other changes, and maybe we want to choose different names.

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