Skip to content

Clean up string processing: delimiters - #7852

Closed
cknitt wants to merge 2 commits into
rescript-lang:masterfrom
cknitt:refactor-strings
Closed

Clean up string processing: delimiters#7852
cknitt wants to merge 2 commits into
rescript-lang:masterfrom
cknitt:refactor-strings

Conversation

@cknitt

@cknitt cknitt commented Sep 6, 2025

Copy link
Copy Markdown
Member

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 *)

@pkg-pr-new

pkg-pr-new Bot commented Sep 6, 2025

Copy link
Copy Markdown

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 cristianoc and tsnobip September 6, 2025 11:43
@cknitt
cknitt marked this pull request as ready for review September 6, 2025 11:43
@cristianoc

Copy link
Copy Markdown
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.

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

Copy link
Copy Markdown

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".

Comment thread compiler/core/lam.ml
@cknitt

cknitt commented Sep 6, 2025

Copy link
Copy Markdown
Member Author

@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.

Comment thread compiler/core/j.ml
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
Copy Markdown
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
Copy Markdown
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.

@cknitt

cknitt commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Superseded by #8606

@cknitt cknitt closed this Sep 4, 2026
@cknitt
cknitt deleted the refactor-strings branch September 4, 2026 14:17
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