Add completion for throw keyword #7905

nojaf · 2025-09-19T08:28:39Z

pkg-pr-new · 2025-09-19T08:37:36Z

rescript

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

@rescript/darwin-arm64

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

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/runtime

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

@rescript/win32-x64

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

commit: 7dc6ff1

nojaf · 2025-09-19T09:01:37Z

analysis/src/CompletionBackEnd.ml

      | None -> []))

+(* Collect exception constructor names from cmt infos. *)
+let exceptions_from_cmt_infos (infos : Cmt_format.cmt_infos) :


This part is vibe coded I'm afraid.
Please review it and let me know if there is a better way.

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.

nojaf · 2025-09-19T09:01:55Z

analysis/src/CompletionBackEnd.ml

+(* Predefined Stdlib/Pervasives exceptions. *)
+let predefined_exceptions : (string * bool) list =
+  [
+    ("Not_found", true);


As found on https://rescript-lang.org/docs/manual/v12.0.0/exception#built-in-exceptions

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.

Copilot

Pull Request Overview

This PR adds completion support for the throw 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 or learn how to get started.}

analysis/src/CompletionBackEnd.ml

zth

Nice! Some initial questions.

zth · 2025-09-23T18:59:16Z

analysis/src/CompletionBackEnd.ml

+(* Predefined Stdlib/Pervasives exceptions. *)
+let predefined_exceptions : (string * bool) list =
+  [
+    ("Not_found", true);


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.

analysis/src/CompletionBackEnd.ml

zth · 2025-09-23T19:02:42Z

analysis/src/CompletionBackEnd.ml

+    match (result, path) with
+    | [], [prefix] when Utils.startsWith "throw" prefix ->
+      completionsForThrow ~env ~full
+    | _ -> result)


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 to Pervasives.throw.

Also, why Utils.startsWith? Is it not supposed to be an exact match?

@nojaf this has not been answered I believe.

We'd need to check if this thing actually resolves to Pervasives.throw.

If you only have thr 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 word throw. Only having it once the full word is typed I find unintuitive.

zth · 2025-09-23T19:05:42Z

analysis/src/CompletionBackEnd.ml

      | None -> []))

+(* Collect exception constructor names from cmt infos. *)
+let exceptions_from_cmt_infos (infos : Cmt_format.cmt_infos) :


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.

nojaf · 2025-09-24T13:34:04Z

Maybe this should more autocomplete JsError.throwWithMessage("Hello!") and JsExn.throw("some non-error value!") instead. (docs)

I cannot ever remember those things and I basically want to have something show up when I type the throw keyword. (Or I'm typing toward that keyword, should trigger on thr as well).

I'm okay with leaving the camels behind.

mediremi

I agree with showing completions for JsError.throwWithMessage and JsExn.throw in addition to throw 👍

zth

Still some outstanding questions and comments from me. Also, is this just for the throw case, without parenthesis and args? A natural extension of this would be to also complete inside of throw(<com>). Unless that already works ofc.

zth · 2025-09-26T10:30:18Z

analysis/src/CompletionBackEnd.ml

+    match (result, path) with
+    | [], [prefix] when Utils.startsWith "throw" prefix ->
+      completionsForThrow ~env ~full
+    | _ -> result)


@nojaf this has not been answered I believe.

zth · 2025-09-26T10:31:43Z

analysis/src/ProcessCmt.ml

+        (match item.str_desc with
+        | Tstr_exception _ -> in_toplevel_exception := true
+        | _ -> ());
+        ()


Why is there an extra unit here...?

zth · 2025-09-26T10:31:50Z

analysis/src/ProcessCmt.ml

+        (match item.sig_desc with
+        | Tsig_exception _ -> in_toplevel_exception := true
+        | _ -> ());
+        ()


Same, extra unit.

zth · 2025-09-26T10:32:41Z

analysis/src/ProcessCmt.ml

+    let module Iter = TypedtreeIter.MakeIterator (struct
+      include TypedtreeIter.DefaultIteratorArgument


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.

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?

zth · 2025-09-26T10:35:41Z

analysis/src/ProcessCmt.ml

+    (* Only collect top-level exception declarations (Tstr_exception/Tsig_exception).
+     Avoid picking up exceptions from Texp_letexception by tracking context. *)


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.

Add completion for throw keyword

cb88ad9

nojaf added 3 commits September 19, 2025 10:59

Try completions if path didn't have any results

4d7ce2e

Add changelog entry

497b372

extract to helper function

678220d

nojaf commented Sep 19, 2025

View reviewed changes

nojaf marked this pull request as ready for review September 19, 2025 09:04

Merge branch 'master' into complete-throw

69bc0d0

nojaf requested review from zth, cristianoc and Copilot September 19, 2025 09:05

Copilot AI reviewed Sep 19, 2025

View reviewed changes

analysis/src/CompletionBackEnd.ml Outdated Show resolved Hide resolved

analysis/src/CompletionBackEnd.ml Outdated Show resolved Hide resolved

zth requested changes Sep 23, 2025

View reviewed changes

Use typed tree ast iterator

5f1e834

nojaf added 3 commits September 24, 2025 15:34

Extract cmt helper to ProcessCmt

5cd2754

Remove ReScript exceptinons and add hardcoded throw functions.

d23722d

Update snapshot

7dc6ff1

nojaf added the editor Everything concerning the analysis binary label Sep 26, 2025

mediremi approved these changes Sep 26, 2025

View reviewed changes

nojaf requested a review from zth September 26, 2025 09:57

zth requested changes Sep 26, 2025

View reviewed changes

		let module Iter = TypedtreeIter.MakeIterator (struct
		include TypedtreeIter.DefaultIteratorArgument

		(* Only collect top-level exception declarations (Tstr_exception/Tsig_exception).
		Avoid picking up exceptions from Texp_letexception by tracking context. *)

Add completion for throw keyword #7905

Are you sure you want to change the base?

Add completion for throw keyword #7905

Conversation

nojaf commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkg-pr-new bot commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

zth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojaf commented Sep 24, 2025

Uh oh!

mediremi left a comment

Choose a reason for hiding this comment

Uh oh!

zth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nojaf commented Sep 19, 2025 •

edited

Loading

pkg-pr-new bot commented Sep 19, 2025 •

edited

Loading