- Notifications
You must be signed in to change notification settings - Fork471
Add try..finally syntax#7903
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
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
rescript@rescript/darwin-arm64@rescript/darwin-x64@rescript/linux-arm64@rescript/linux-x64@rescript/runtime@rescript/win32-x64commit: |
Maybe add a |
| attach t.trailing expr.pexp_loc after_expr; | ||
| walk_list (cases|>List.map (funcase ->Case case)) t rest | ||
| (* unary expression: todo use parsetreeviewer*) | ||
| |Pexp_try (expr,cases,finally_expr) -> ( |
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.
why was this case not handled at all before?
I would expect a change that only deals with finally, here
| elseif | ||
| Parser.lookahead p (funstate -> | ||
| match state.Parser.tokenwith | ||
| |Lident"finally" ->true |
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.
what's finally in the grammar? just an identified?
Not a keyword I guess
| }catch { | ||
| |InnerError=>Console.log("Inner caught") | ||
| } | ||
| }catch { |
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.
Is this the indentation we want? Does TS look like this when formatted?
| let finally_typed= | ||
| type_expect~context:None env exprPredef.type_unit | ||
| in | ||
| (* Finally blocks must return unit*) |
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.
why?
| *) | ||
| |Texp_try of expression* caselist | ||
| (** try E with P1 -> E1 | ... | PN -> EN*) | ||
| |Texp_try of expression* caselist* expressionoption |
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.
nit: get assistant to use a record in a hypothetical follow-up PR, if this goes ahead
| Sexp.list (map_empty~f:case cases); | ||
| ] | ||
| |Pexp_try (expr,cases) -> | ||
| |Pexp_try (expr,cases,finally_expr) -> |
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 keep on forgetting why we have this debugger file. Gives maintenance burden but can't remember why we need it.
| |Lstaticraise ofint* tlist | ||
| |Lstaticcatch of t* (int* identlist)* t | ||
| |Ltrywith of t* ident* t | ||
| |Ltrywith of t* ident* toption* toption |
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.
opportunity: this file is entirely uncommented
a separate PR couldassist in adding some explanation
More bigger / longer term assistance opportunity: why do we have both lambda and lam? Is that a historical artefact for what used to be backwards compatibility, or can they possibly be merged now?
| P.string fL.finally; | ||
| P.space f; | ||
| brace_block cxt f b)) | ||
| P.space f; |
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.
curious whyfin was already here
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 PR should have a good amount of text in the description explaining what this PR does at high level.
Uh oh!
There was an error while loading.Please reload this page.
Adds support for
try..finallyandtry..catch..finallysyntax to ReScript.No breaking changes / full backwards compatibility.
Resolves#6974.