-
Notifications
You must be signed in to change notification settings - Fork 470
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PoC of let? #7582
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
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. |
@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 |
61aff32
to
0d067f1
Compare
There was a problem hiding this 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 the
let?
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
File | Description |
---|---|
tests/tests/src/LetUnwrap.res |
Test cases demonstrating let? syntax with Result, Option, and async patterns |
tests/tests/src/LetUnwrap.mjs |
Generated JavaScript output showing the transformation logic |
tests/syntax_tests/res_test.ml |
Parser test update for new Let token structure |
tests/syntax_tests/data/printer/expr/letUnwrap.res |
Printer test cases for let? syntax |
tests/syntax_tests/data/parsing/grammar/expressions/letUnwrap.res |
Grammar parsing test cases |
tests/syntax_tests/data/parsing/errors/signature/letUnwrap.resi |
Error test for let? in signatures |
tests/syntax_tests/data/parsing/errors/expressions/letUnwrapRec.res |
Error test for let? with rec |
tests/build_tests/super_errors/fixtures/* |
Error handling test fixtures |
rewatch/src/config.rs |
Configuration support for experimental features in rescript.json |
rewatch/src/build/compile.rs |
Compiler argument generation for experimental features |
compiler/syntax/src/res_token.ml |
Token definition updates to support let? |
compiler/syntax/src/res_scanner.ml |
Scanner updates to recognize let? syntax |
compiler/syntax/src/res_printer.ml |
Pretty printer support for let? |
compiler/syntax/src/res_grammar.ml |
Grammar updates for let? parsing |
compiler/syntax/src/res_core.ml |
Core parsing logic for let? with validation |
compiler/ml/experimental_features.ml |
Experimental feature management system |
compiler/frontend/bs_syntaxerr.ml |
Error message definitions for let? |
compiler/frontend/bs_builtin_ppx.ml |
AST transformation logic for let? to switch expressions |
compiler/frontend/ast_attributes.ml |
Attribute handling for let.unwrap marker |
compiler/bsc/rescript_compiler_main.ml |
Command line flag support for experimental features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
let enable_from_string (s : string) = | ||
match from_string s with | ||
| Some f -> enabled_features := FeatureSet.add f !enabled_features | ||
| None -> () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enable_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.
let enable_from_string (s : string) = | |
match from_string s with | |
| Some f -> enabled_features := FeatureSet.add f !enabled_features | |
| None -> () | |
let enable_from_string (s : string) : bool = | |
match from_string s with | |
| Some f -> | |
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 @@ | |||
|
There was a problem hiding this comment.
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 where let?
is used in a function whose return type is not result
/option
:
@@config({flags: ["-enable-experimental", "LetUnwrap"]})
let fn = (): int => {
let? Some(x) = None
42
}
There was a problem hiding this comment.
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've found a bug for you!
tst.res:6:8-14
4 │
5 │ let fn = (): int => {
6 │ let? Some(x) = x
7 │ Some(x)
8 │ }
This has type: option<'a>
But this let? is used in a context expecting the type: int
let? can only be used in a context that expects option or result.
There was a problem hiding this comment.
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 of let? can only be used in a function that returns option or result
- since context that expects
may be confusing.
There was a problem hiding this 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 😁
@mediremi would you have another look at all the error messages now? |
The new error messages for |
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. |
@@ -402,6 +402,10 @@ let command_line_flags : (string * Bsc_args.spec * string) array = | |||
( "-absname", | |||
set absname, | |||
"*internal* Show absolute filenames in error messages" ); | |||
( "-enable-experimental", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?
Amazing work here! Some remarks:
|
There was a problem hiding this 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 unwraps result
/option
values and early-returns on Error
/None
. It’s explicitly experimental and disabled by default behind a new “experimental features” gate. ([GitHub]1, [ReScript Forum]2)
How you’d use it
let getUser = async (id) => {
let? Ok(user) = await fetchUser(id)
let? Ok(decodedUser) = decodeUser(user)
Console.log(`Got user ${decodedUser.name}!`)
let? Ok() = await ensureUserActive(decodedUser)
Ok(decodedUser)
}
This desugars to a sequence of switch
/early-returns that you’d otherwise write by hand, so there’s no extra runtime cost and it plays nicely with async/await
. Same idea works for option
with Some(...)
(and the PR also extends support so the left pattern can be Error(...)
/None
, not just Ok(...)
/Some(...)
). ([ReScript Forum]2, [GitHub]3)
Where it works (and doesn’t)
- Targets built-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 an experimental-features infrastructure to the toolchain and CLI 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 in
rescript.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 for
let?
, grammar rules, and pretty-printer support. ([GitHub]3) - Frontend transform: AST handling lowers
let?
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 when off?
Bottom line: extremely low risk when the flag is off.
Why:
- Gated by default. The feature is unreachable unless explicitly enabled; using
let?
without the gate yields a dedicated “feature not enabled” error (there are fixtures for this). That means existing codebases remain unaffected. ([GitHub]3) - 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) - No runtime path changes. It’s a compile-time sugar that lowers to the same
switch
/return structure you’d hand-write. If you don’t use it, nothing in your generated JS changes. ([ReScript Forum]2) - Confined scope. Even when enabled, top-level
let?
is rejected; only local/block bindings are supported. This reduces any accidental global impact. ([GitHub]3) - 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 the
let?
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 looks eligible for
let?
). That only triggers in error paths; it won’t change successful builds. ([GitHub]3)
Conclusion: With the experimental flag off, 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.
Co-authored-by: Copilot <[email protected]>
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