Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

PoC of let?#7582

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
zth merged 21 commits intomasterfromlet-unwrap-poc
Aug 27, 2025
Merged

PoC of let?#7582

zth merged 21 commits intomasterfromlet-unwrap-poc
Aug 27, 2025

Conversation

@zth
Copy link
Member

@zthzth commentedJun 30, 2025
edited
Loading

npm i https://pkg.pr.new/rescript-lang/rescript@7582
letgetXWithResultAsync=asyncs=> {let?Ok({s}asres)=awaitdoStuffResultAsync(s)Console.log(s)let?Ok(x)=awaitdecodeResAsync(res)Ok(x++"test")}

EDIT: This is now hidden behind a new concept of "experimental features". Therefore, this is ready to be reviewed and merged as experimental, if we want to,

TODO

  • Make sure error messages make sense and are well covered
  • Make sure editor tooling works as expected

mediremi, aspeddro, tsnobip, leoliu, rolandpeelen, LeoLeBras, fhammerschmidt, hackwaly, joakin, and MichalKalita reacted with hooray emoji
@pkg-pr-new
Copy link

pkg-pr-newbot commentedJun 30, 2025
edited
Loading

Open in StackBlitz

rescript

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

@rescript/darwin-arm64

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

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/runtime

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

@rescript/win32-x64

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

commit:5386466

@leoliu
Copy link

Looks like a great feature in development!

BTW some languages place ? around the = like ?= or =?. I wonder if this has been considered. This can potentially also be used with if.

@zth
Copy link
MemberAuthor

zth commentedJul 2, 2025

@leoliu thank you! There's now a forum post for the proposal where you can add your thoughts if you want:https://forum.rescript-lang.org/t/proposing-new-syntax-for-zero-cost-unwrapping-options-results/6227

@cknittcknitt added this to thev12.1 milestoneJul 3, 2025
@zthzthforce-pushed thelet-unwrap-poc branch 2 times, most recently from61aff32 to0d067f1CompareAugust 23, 2025 06:31
@zthzth marked this pull request as ready for reviewAugust 23, 2025 06:47
Copy link
Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements the "let?" syntax (LetUnwrap) as an experimental feature for ReScript, providing syntactic sugar for early-return patterns with Result and Option types. The implementation hides this functionality behind an experimental feature flag that must be explicitly enabled.

Key changes:

  • Adds thelet? syntax that automatically unwraps Result/Option types with early return behavior
  • Implements experimental feature infrastructure with configuration support in rescript.json
  • Provides comprehensive error handling and validation for the new syntax

Reviewed Changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
tests/tests/src/LetUnwrap.resTest cases demonstrating let? syntax with Result, Option, and async patterns
tests/tests/src/LetUnwrap.mjsGenerated JavaScript output showing the transformation logic
tests/syntax_tests/res_test.mlParser test update for new Let token structure
tests/syntax_tests/data/printer/expr/letUnwrap.resPrinter test cases for let? syntax
tests/syntax_tests/data/parsing/grammar/expressions/letUnwrap.resGrammar parsing test cases
tests/syntax_tests/data/parsing/errors/signature/letUnwrap.resiError test for let? in signatures
tests/syntax_tests/data/parsing/errors/expressions/letUnwrapRec.resError test for let? with rec
tests/build_tests/super_errors/fixtures/*Error handling test fixtures
rewatch/src/config.rsConfiguration support for experimental features in rescript.json
rewatch/src/build/compile.rsCompiler argument generation for experimental features
compiler/syntax/src/res_token.mlToken definition updates to support let?
compiler/syntax/src/res_scanner.mlScanner updates to recognize let? syntax
compiler/syntax/src/res_printer.mlPretty printer support for let?
compiler/syntax/src/res_grammar.mlGrammar updates for let? parsing
compiler/syntax/src/res_core.mlCore parsing logic for let? with validation
compiler/ml/experimental_features.mlExperimental feature management system
compiler/frontend/bs_syntaxerr.mlError message definitions for let?
compiler/frontend/bs_builtin_ppx.mlAST transformation logic for let? to switch expressions
compiler/frontend/ast_attributes.mlAttribute handling for let.unwrap marker
compiler/bsc/rescript_compiler_main.mlCommand line flag support for experimental features

Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.

Comment on lines +18 to +21
letenable_from_string (s: string)=
match from_string swith
|Somef -> enabled_features:=FeatureSet.add f!enabled_features
|None ->()

Choose a reason for hiding this comment

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

Theenable_from_string function silently ignores unknown feature names. This could lead to configuration errors being missed. Consider logging a warning or returning a result type to indicate failure.

Suggested change
letenable_from_string (s: string)=
match from_string swith
|Somef -> enabled_features:=FeatureSet.add f!enabled_features
|None ->()
letenable_from_string (s: string) :bool=
match from_string swith
|Somef ->
enabled_features:=FeatureSet.add f!enabled_features;
true
|None ->
Printf.eprintf"Warning: Unknown feature name '%s' (ignored)\n" s;
false

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,11 @@

Copy link
Member

@mediremimediremiAug 23, 2025
edited
Loading

Choose a reason for hiding this comment

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

Another super error test that I think we could have is for the case wherelet? is used in a function whose return type is notresult/option:

@@config({flags: ["-enable-experimental","LetUnwrap"]})letfn= ():int=> {let?Some(x)=None42}

Copy link
MemberAuthor

@zthzthAug 23, 2025
edited
Loading

Choose a reason for hiding this comment

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

Good catch! I added one:47b70bb

...and the error is terrible. I gave it a quick shot to improve it:
5f196a0

@mediremi can you think of a good error message here?

EDIT: This is the current error:

We'vefoundabugforyou!tst.res:6:8-1445letfn= ():int=> {6let?Some(x)=x7Some(x)8 │ }Thishastype:option<'a>Butthislet?isusedinacontextexpectingthetype:intlet?canonlybeusedinacontextthatexpectsoptionorresult.

Copy link
Member

@mediremimediremiAug 23, 2025
edited
Loading

Choose a reason for hiding this comment

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

The new error is a nice improvement to the original error message mentioning a switch 💪

Would it be possible to detect if the context is a function? That way we can have a clearer error message for that case where we say something along the lines oflet? can only be used in a function that returns option or result - sincecontext that expects may be confusing.

Copy link
Member

@mediremimediremi left a comment

Choose a reason for hiding this comment

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

I tried this out in a bsb project with"bsc-flags": ["-enable-experimental", "LetUnwrap"] and it all works perfectly 🎉

Great work@zth 😁

zth and fhammerschmidt reacted with hooray emoji
@zth
Copy link
MemberAuthor

zth commentedAug 23, 2025

@mediremi would you have another look at all the error messages now?

@mediremi
Copy link
Member

The new error messages forlet? being used in the wrong context look good to me ✔️

zth reacted with thumbs up emoji

@zth
Copy link
MemberAuthor

zth commentedAug 24, 2025

Please feel free to add any other feedback in the coming days. We'll merge this under the experimental flag before the next beta release if nothing more comes up.

("-absname",
set absname,
"*internal* Show absolute filenames in error messages" );
("-enable-experimental",
Copy link
Member

Choose a reason for hiding this comment

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

I can imagine this might grow in the future.
Should this be a list to be future proof?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

It is a list, just that you pass it multiple times. Or are you referring to making it-enable-experimental FeatureX,FeatureY instead of-enable-experimental FeatureX -enable-experimental FeatureY?

@nojaf
Copy link
Member

Amazing work here!

Some remarks:

  • I miss a test example that involves promises. How does this work with async/await? I’d like to see a more real-world example.
  • The experimental configuration is fine, but I’d be comfortable making this part ofcompiler-flags. It’s less intrusive and feels more ephemeral.
  • We should document what users can expect from experimental features. I suggest clearly stating that these are features we’re trying out: they may not make it into the stable product and can change without semantic-versioning guarantees. If users like a feature, encourage them to give feedback to the core team.
zth reacted with thumbs up emoji

Copy link
Collaborator

@cristianoccristianoc left a comment

Choose a reason for hiding this comment

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

Auto gen quick summary + detailed risk analysis:


What’s new (user-facing, with a dev sketch)

Feature:let? (“let-unwrap”) — a tiny syntax that unwrapsresult/option values andearly-returns onError/None. It’s explicitlyexperimental anddisabled by default behind a new “experimental features” gate. ([GitHub]1, [ReScript Forum]2)

How you’d use it

letgetUser=async (id)=> {let?Ok(user)=awaitfetchUser(id)let?Ok(decodedUser)=decodeUser(user)Console.log(`Got user ${decodedUser.name}!`)let?Ok()=awaitensureUserActive(decodedUser)Ok(decodedUser)}

This desugars to a sequence ofswitch/early-returns that you’d otherwise write by hand, so there’sno extra runtime cost and it plays nicely withasync/await. Same idea works foroption withSome(...) (and the PR also extends support so the left pattern can beError(...)/None, not justOk(...)/Some(...)). ([ReScript Forum]2, [GitHub]3)

Where it works (and doesn’t)

  • Targetsbuilt-ins only:result andoption. (Custom variants still needswitch.) ([ReScript Forum]2)
  • Block/local bindings only; tests show top-level usage is rejected. ([GitHub]3)
  • The printed JS is the straightforward if/return form (i.e., “zero cost”). ([ReScript Forum]2)

How to enable it (experimental)

  • The PR adds anexperimental-features infrastructure to the toolchain andCLI support:-enable-experimental …. Rewatch reads config fromrescript.json and forwards the feature(s) to the compiler. (Seerewatch config + compile changes and the new compiler flag.) ([GitHub]3)
  • There are tests and docs stubs around the experimental toggle plus “feature not enabled” error cases. ([GitHub]3)

In short: add the feature inrescript.json via the new “experimental features” setting (per the updatedCompilerConfigurationSpec.md), or pass-enable-experimental in your build; otherwiselet? is unavailable. ([GitHub]3)

Dev notes (what changed under the hood)

  • Lexer/Parser/Printer: new token forlet?, grammar rules, and pretty-printer support. ([GitHub]3)
  • Frontend transform: AST handling lowerslet? to the equivalentswitch/early return; tailored super-errors for misuse. ([GitHub]3)
  • Feature gate: new module(s) to register/enable experimental features, with CLI wiring and rewatch config reading. ([GitHub]3)
  • Tests: syntax parsing, printer snapshots, super-errors (e.g., “feature not enabled”, “top-level not allowed”, “not supported variant”, and return-type mismatch). ([GitHub]3)

Safety analysis: can this break anything whenoff?

Bottom line: extremely low risk when the flag isoff.

Why:

  1. Gated by default. The feature is unreachable unless explicitly enabled; usinglet? without the gate yields a dedicated “feature not enabled” error (there are fixtures for this). That means existing codebases remain unaffected. ([GitHub]3)
  2. New syntax, not repurposed.let? didn’t previously parse; recognizing it only at alet binding start avoids collisions with existing? uses (e.g., ternary) elsewhere in expressions. Parser tests were added to lock this down. ([GitHub]3)
  3. No runtime path changes. It’s a compile-time sugar that lowers to the sameswitch/return structure you’d hand-write. If you don’t use it, nothing in your generated JS changes. ([ReScript Forum]2)
  4. Confined scope. Even when enabled, top-levellet? is rejected; only local/block bindings are supported. This reduces any accidental global impact. ([GitHub]3)
  5. Tooling guarded. The PR tracks TODOs for error coverage and editor support; with the gate off, current editor plugins behave as before. ([GitHub]1)

Potential edge considerations (still low risk off):

  • Tokenization side-effects: The scanner learns thelet? token, but the feature gate + grammar location prevents it from altering how existing, valid programs lex/parse when you don’t writelet?. Tests cover “not enabled” and misuse shapes. ([GitHub]3)
  • Error text churn: Some super-error messages were added (including a hint when code lookseligible forlet?). That only triggers in error paths; it won’t change successful builds. ([GitHub]3)

Conclusion: With the experimental flagoff, there are no functional or runtime changes, and parser behavior for all previously valid code paths is preserved. It’s safe to ship behind the flag.


If you want, I can also jot down a tiny rollout checklist (enable flag in a sample app, verify editor plugin snapshot, run the super-errors suite) next.

zth reacted with thumbs up emoji
@zthzth merged commitce7f09c intomasterAug 27, 2025
25 checks passed
@zthzth deleted the let-unwrap-poc branchAugust 27, 2025 14:58
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@mediremimediremimediremi approved these changes

Copilot code reviewCopilotCopilot left review comments

@jfrolichjfrolichjfrolich approved these changes

@nojafnojafnojaf approved these changes

@cristianoccristianoccristianoc approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v12.1

Development

Successfully merging this pull request may close these issues.

9 participants

@zth@leoliu@mediremi@nojaf@jfrolich@cristianoc@cknitt@tsnobip

[8]ページ先頭

©2009-2025 Movatter.jp