- Notifications
You must be signed in to change notification settings - Fork471
Add completion for throw keyword#7905
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
pkg-pr-newbot commentedSep 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
rescript@rescript/darwin-arm64@rescript/darwin-x64@rescript/linux-arm64@rescript/linux-x64@rescript/runtime@rescript/win32-x64commit: |
analysis/src/CompletionBackEnd.ml Outdated
| |None ->[])) | ||
| (* Collect exception constructor names from cmt infos.*) | ||
| letexceptions_from_cmt_infos (infos: Cmt_format.cmt_infos) : |
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.
This part is vibe coded I'm afraid.
Please review it and let me know if there is a better way.
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.
Can a typed tree AST iterator be used here instead of the hand rolled traversal functions below? Would feel more idiomatic, and make the code clearer as you could just add a visitor for the relevant extension nodes.
analysis/src/CompletionBackEnd.ml Outdated
| (* Predefined Stdlib/Pervasives exceptions.*) | ||
| letpredefined_exceptions :(string * bool) list= | ||
| [ | ||
| ("Not_found",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.
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't help but wonder whether we should be encouraging using these builtins. They're really just heritage from the OCaml era. So I think a case could be made to not include them at all.
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 adds completion support for thethrow keyword in ReScript, providing suggestions for both built-in and user-defined exceptions when typing "throw".
- Implements completion functionality that suggests exception constructors when typing "throw"
- Adds logic to extract exception information from compiled module files (CMT)
- Includes predefined standard library exceptions with appropriate argument placeholders
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| analysis/src/CompletionBackEnd.ml | Core implementation of throw completion logic including exception extraction and completion generation |
| tests/analysis_tests/tests/src/Throw.res | Test file with custom exceptions for validation |
| tests/analysis_tests/tests/src/expected/Throw.res.txt | Expected completion output for throw keyword test |
| tests/analysis_tests/tests/src/expected/CompletionJsxProps.res.txt | Updated test expectations including new Throw module |
| tests/analysis_tests/tests/src/expected/Completion.res.txt | Updated test expectations including new Throw module |
| CHANGELOG.md | Documentation of the new feature |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Nice! Some initial questions.
analysis/src/CompletionBackEnd.ml Outdated
| (* Predefined Stdlib/Pervasives exceptions.*) | ||
| letpredefined_exceptions :(string * bool) list= | ||
| [ | ||
| ("Not_found",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.
I can't help but wonder whether we should be encouraging using these builtins. They're really just heritage from the OCaml era. So I think a case could be made to not include them at all.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| match (result, path)with | ||
| |[], [prefix]whenUtils.startsWith"throw" prefix -> | ||
| completionsForThrow~env~full | ||
| |_ -> 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.
This should probably be done in a "safer" way that matches by type and not by name. We'd need to check if this thing actually resolves toPervasives.throw.
Also, whyUtils.startsWith? Is it not supposed to be an exact match?
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.
@nojaf this has not been answered I believe.
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.
We'd need to check if this thing actually resolves to Pervasives.throw.
If you only havethr you can't do that.
Also, why Utils.startsWith? Is it not supposed to be an exact match?
As mentioned, I want the completions to show up while I'm typing towards the wordthrow. Only having it once the full word is typed I find unintuitive.
analysis/src/CompletionBackEnd.ml Outdated
| |None ->[])) | ||
| (* Collect exception constructor names from cmt infos.*) | ||
| letexceptions_from_cmt_infos (infos: Cmt_format.cmt_infos) : |
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.
Can a typed tree AST iterator be used here instead of the hand rolled traversal functions below? Would feel more idiomatic, and make the code clearer as you could just add a visitor for the relevant extension nodes.
Maybe this should more autocomplete I cannot ever remember those things and I basically want to have something show up when I type the I'm okay with leaving the camels behind. |
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 agree with showing completions forJsError.throwWithMessage andJsExn.throw in addition tothrow 👍
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.
Still some outstanding questions and comments from me. Also, is this just for thethrow case, without parenthesis and args? A natural extension of this would be to also complete inside ofthrow(<com>). Unless that already works ofc.
| match (result, path)with | ||
| |[], [prefix]whenUtils.startsWith"throw" prefix -> | ||
| completionsForThrow~env~full | ||
| |_ -> 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.
@nojaf this has not been answered I believe.
| (match item.str_descwith | ||
| |Tstr_exception_ -> in_toplevel_exception:=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.
Why is there an extra unit here...?
| (match item.sig_descwith | ||
| |Tsig_exception_ -> in_toplevel_exception:=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.
Same, extra unit.
| letmoduleIter =TypedtreeIter.MakeIterator (struct | ||
| includeTypedtreeIter.DefaultIteratorArgument |
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.
Do we use the functor based approach elsewhere in the code base? Or is there no way for the typed tree to just define a mapper like we do for the AST mapper? I don't remember how this works exactly.
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.
Yes, this was weird to me as well, it is a departure from what we have on the untyped side, but this seems to be the mapper we have?
| (* Only collect top-level exception declarations (Tstr_exception/Tsig_exception). | ||
| Avoid picking up exceptions from Texp_letexception by tracking context.*) |
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.
In theory I guess the most robust approach would be to collect and use whatever defined exceptions are in scope at the current position. Collecting at the top level is good and simple, but you could also define them in sub modules, and so on.
It might be worth exploring making exceptions part of the tracked scope instead, and track them just like we track types and values today when doing completion. Then we could just extend the current mechanism a bit, and hopefully get the correct scope tracking for free.
Uh oh!
There was an error while loading.Please reload this page.