Use RescriptError for exceptions #6979

DZakh · 2024-08-26T19:11:14Z

No description provided.

DZakh

@cknitt Letting you know that tests fail with the following error. I know there's an unresolved issued with imports in the PR, so I don't even expect them to pass. But the error is not useful at all:

jscomp/core/js_dump.ml

DZakh · 2024-08-26T20:02:53Z

jscomp/runtime/caml_js_exceptions.res

+let internalFromExtension = (_ext: 'a): exn => {
+ %raw(`Object.assign(new RescriptError(_ext.RE_EXN_ID), _ext)`)
+}


I think it can be optimised to store the whole payload in an error field instead of spreading the object. But it requires modifying the matching logic, so I've done the easiest thing here to have at least something working

I'd like to finish the PR with the implementation and improve it in another PR.

Couldn't we just new RescriptError(id, payload) ?

It'll still require to do Object.assign inside of the RescriptError constructor. Until I updated matching logic, it's not possible to get rid of Object.assign

DZakh · 2024-08-26T20:04:50Z

jscomp/core/js_dump.ml

 : J.expression =
 let field_name =
 match ext with
- | Blk_extension -> (
+ | Blk_extension _ -> (
 fun i ->
 match i with 0 -> Literals.exception_id | i -> "_" ^ string_of_int i)


This change should make extension fields have the same logic as variants. Currently if an extension has a payload, it starts with _1 while normal variants start with _0. I think it makes sense to have it unified. Should I include the change in a separate PR?

Suggested change

match i with 0 -> Literals.exception_id | i -> "_" ^ string_of_int i)

match i with 0 -> Literals.exception_id | i -> "_" ^ string_of_int (i - 1))

Unified sounds good to me. If this causes a lot of output changes, then yes, maybe better in a separate PR.

DZakh · 2024-08-26T20:07:09Z

The change example

cometkim · 2024-08-26T20:50:25Z

It'll be better to completely remove the dependency on Caml_exceptions.

It has its own runtime that tracks exn symbols. It can be easily replaced with JavaScript native symbols.

-let A = Caml_exceptions.create("A");
+let A = Symbol.for("A");

DZakh · 2024-08-27T05:40:57Z

It has its own runtime that tracks exn symbols. It can be easily replaced with JavaScript native symbols.

I think it's a good idea, I can probably do it in another PR. But probably it should be Symbol("A") instead of Symbol.for

cometkim · 2024-08-27T05:44:24Z

It has its own runtime that tracks exn symbols. It can be easily replaced with JavaScript native symbols.

I think it's a good idea, I can probably do it in another PR. But probably it should be Symbol("A") instead of Symbol.for

Symbol("A") is "unique" symbol, not a globally shared one.

DZakh · 2024-08-27T05:52:37Z

It has its own runtime that tracks exn symbols. It can be easily replaced with JavaScript native symbols.

I think it's a good idea, I can probably do it in another PR. But probably it should be Symbol("A") instead of Symbol.for

Symbol("A") is "unique" symbol, not a globally shared one.

Isn't it what we need?

cometkim · 2024-08-27T05:53:29Z

It'll be better to completely remove the dependency on Caml_exceptions.

I mentioned it because I wanted to drop the Caml_exceptions module, too

cometkim · 2024-08-27T05:56:11Z

It has its own runtime that tracks exn symbols. It can be easily replaced with JavaScript native symbols.

I think it's a good idea, I can probably do it in another PR. But probably it should be Symbol("A") instead of Symbol.for

Symbol("A") is "unique" symbol, not a globally shared one.

Isn't it what we need?

We can use Symbol.for as the drop-in replacement of Caml_exceptions.create.

But if we use unique symbols in runtime, we need another protocol (import/export) for it.

DZakh · 2024-08-27T06:45:49Z

The created exception identifiers are already exported, so the same mechanism should work with symbols

cknitt · 2024-08-27T06:48:59Z

@cknitt Letting you know that tests fail with the following error. I know there's an unresolved issued with imports in the PR, so I don't even expect them to pass. But the error is not useful at all:

Ok, will have a look at that error later.

DZakh · 2024-08-27T20:07:43Z

It's not the complete implementation, but it works

DZakh · 2024-08-28T05:17:58Z

@cknitt @cristianoc ready for review

cknitt · 2024-08-28T08:57:04Z

jscomp/core/js_dump.ml

 : J.expression =
 let field_name =
 match ext with
- | Blk_extension -> (
+ | Blk_extension _ -> (
 fun i ->
 match i with 0 -> Literals.exception_id | i -> "_" ^ string_of_int i)


Unified sounds good to me. If this causes a lot of output changes, then yes, maybe better in a separate PR.

jscomp/core/js_dump.ml

cknitt · 2024-08-28T09:06:57Z

jscomp/runtime/caml_js_exceptions.res

 } else {
- JsError(e)
+ {


This is what we throw if we are rethrowing a non-ReScript exception, right?
So in this case we are throwing something that's not an instance of Error/RescriptError?

Oh, it's a mistake

Actually, I wanted to investigate a special case for JsError where it's not wrapped in RescriptError

nojaf · 2024-08-28T09:32:36Z

@DZakh out of curiosity, does this change anything for the end-users?

DZakh · 2024-08-28T09:35:52Z

@DZakh out of curiosity, does this change anything for the end-users?

If they don't do dirty things with runtime representation of exceptions, then it should stay the same for them.

DZakh · 2024-08-28T09:37:53Z

Btw, do we want to include RescriptError name in the log?

cknitt · 2024-08-28T09:40:21Z

Btw, do we want to include RescriptError name in the log?

Would be good to have it in the log I think.

DZakh · 2024-08-28T09:40:46Z

Solves:
#6929
#6972
#6931

cristianoc

Leaving some comments.
I'd like to see a detailed review from @cometkim

cristianoc · 2024-08-28T09:45:10Z

jscomp/ml/ctype.ml

@@ -3674,7 +3674,7 @@ let rec subtype_rec env trace t1 t2 cstrs =
 true (* handled in the fields checks *)
 | Record_unboxed b1, Record_unboxed b2 -> b1 = b2
 | Record_inlined _, Record_inlined _ -> repr1 = repr2
- | Record_extension, Record_extension -> true
+ | Record_extension b1, Record_extension b2 -> b1.is_exception = b2.is_exception


Is this casting any exception to any other exception?
OK seems legit.

I actually don't know what the code is about, I just made it compile in the most safe way :)

cristianoc · 2024-08-28T09:46:21Z

jscomp/ml/matching.ml

@@ -2918,7 +2918,9 @@ let partial_function loc () =
 let fname = 
 Filename.basename fname
 in 
- Lprim(Praise Raise_regular, [Lprim(Pmakeblock(Blk_extension),
+ Lprim(Praise Raise_regular, [Lprim(Pmakeblock(Blk_extension {
+ is_exception = true;


How do you know that there's no other place in the compiler where one could have forgotten to do this?
Are there enough tests?

E.g. let e = E(10)

Are you talking about adding the correct is_exception value? There is compiler complaining in every place where it's needed + there's record_extension_test.js and multiple other tests showing that output is correct.

cristianoc · 2024-08-28T09:47:37Z

jscomp/ml/types.ml

@@ -282,15 +283,15 @@ and constructor_tag =
 Cstr_constant of int (* Constant constructor (an int) *)
 | Cstr_block of int (* Regular constructor (a block) *)
 | Cstr_unboxed (* Constructor of an unboxed type *)
- | Cstr_extension of Path.t    (* Extension constructor *)
+ | Cstr_extension of Path.t * bool (* Extension constructor. true if is_exception *)


This might break binary compatibility.
Need to check.
CC @zth

I'm curious about this too.

We could modify ml types to avoid major complexity, but wouldn't that make potential compatibility issues? At least until we have a first-class toolchain for building PPXs.

Code Review Report: Impact of Proposed Change in Runtime Representation

High-Level Summary

The proposed change in runtime representation introduces significant structural differences that can lead to severe incompatibilities with existing code. Specifically, when casting old data types to the new representation using Obj.magic, the program is prone to crashes due to mismatches in memory layout. This highlights the need for careful handling and testing when evolving data structures to ensure backward compatibility and avoid runtime errors.

Test Results

New Value with Old Code: The program successfully processed a value created with the new type using the old code. The output was correct, and no crashes occurred. This indicates that the old code is capable of handling the new structure, at least when it ignores additional fields that were introduced in the new representation.

Old Value with New Code: The program crashed with a segmentation fault when attempting to process a value created with the old type using the new code. The crash occurred because the new code attempted to access fields that do not exist in the old memory layout, demonstrating the risk of using the new representation with existing data.

Supporting Code

The following code was used to test the impact of the proposed runtime representation change. It illustrates the potential issues that arise when interacting with old and new data structures:

module Path = struct type t = string list end (* Old variant type definition *) type my_variant_old = | Old_extension of Path.t (* Define a more complex structure for the new variant *) type additional_info = { id: int; description: string; } (* New variant type definition *) type my_variant_new = | New_extension of Path.t * additional_info (* Function that operates on the old type definition *) let process_variant_old = function | Old_extension path -> Printf.printf "Inside process_variant_old\n%!"; let length = List.length path in Printf.printf "Length: %d\n%!" length; length (* Function that operates on the new type definition *) let process_variant_new = function | New_extension (path, info) -> Printf.printf "Inside process_variant_new\n%!"; Printf.printf "ID: %d\n%!" info.id; Printf.printf "Description: %s\n%!" info.description; Printf.printf "Length: %d\n%!" (List.length path) (* Function to test a new value with the old code *) let test_new_value_with_old_code () = Printf.printf "Testing new value with old code:\n%!"; let new_value = New_extension (["a"; "b"; "c"], { id = 42; description = "Test" }) in Printf.printf "Created new value\n%!"; let casted_value = Obj.magic new_value in Printf.printf "After casting to old value\n%!"; process_variant_old casted_value (* Function to test an old value with the new code *) let test_old_value_with_new_code () = Printf.printf "Testing old value with new code:\n%!"; let old_value = Old_extension ["a"; "b"; "c"] in Printf.printf "Created old value\n%!"; let casted_value = Obj.magic old_value in Printf.printf "After casting to new value\n%!"; process_variant_new casted_value (* Main function to run the tests *) let () = Printf.printf "Running tests...\n%!"; test_new_value_with_old_code (); Printf.printf "Finished testing new value with old code\n%!"; test_old_value_with_new_code (); Printf.printf "Finished testing old value with new code\n%!"

This code confirms that the proposed changes to the runtime representation work without issue when the new structure is used with old code but cause a crash when old data is processed by the new code.

@zth this restrict the way the editor extension can evolve -- what do you think?

What happens if the current arg is changed to an inline record instead? Would that preserve the memory layout enough?

So overall what this situation is saying is:
1 make this change and all is fine
2 update the vendored .ml files in the vscode extension and run it on older version of the compiler: recipe for trouble

One possible solution is:
1.5 the extension moves inside the compiler

One possible solution is:
1.5 the extension moves inside the compiler

Created #6994.

@cristianoc Currently working on moving the extension into the compiler.
Can this PR be merged already, or do you want to wait until the move is complete?

That's fine by me.

Then I'll resolve conflicts, when I have time

cristianoc · 2024-08-28T09:48:54Z

jscomp/ml/types.ml

- | Cstr_extension (path1), Cstr_extension (path2) -> 
- Path.same path1 path2
+ | Cstr_extension (path1, is_exception1), Cstr_extension (path2, is_exception2) -> 
+ Path.same path1 path2 && is_exception1 = is_exception2


The code is good. Not a suggestion to change.
Just an additional curiosity: if the path is the same, is it possible that the is_exception values don't agree?

If path includes the module, then checking only path value should be enough.

cristianoc · 2024-08-28T09:50:31Z

lib/es6/caml_exceptions.js

- if (e == null) {
- return false;
+function is_extension(any) {
+ if (any) {


Can this be made more precise? Should it be not null, not undefined, both?
Perhaps it's safe anyway. Just double checking.

It's safe, but I wonder why && was generated to if/else

cometkim · 2024-08-29T10:21:48Z

jscomp/test/406_primitive_test.js

- throw new Error(exn.RE_EXN_ID, {
- cause: exn
- });
+ throw exn;


I understand that this PR respects existing behavior. So this is just note for the future change

Suggested change

throw exn;

throw raw_exn;

For example, suppose an exception raised in JS goes through a utility written in ReScript and then reaches JS again.

ReScript wraps all thrown values in RescriptError, regardless of whether it matches or not.
It can be problematic for FFIs because it can harm the intent of the external source.

Here's the ideal output I can think of:

let A = Symbol.for("rescript.exn$Module.A") try { throw new RescriptError(A, payload); } catch (raw) { if (RescriptError.isExn(raw, A)) { // ...just match, no implicit conversion } // rethrow value as-is throw raw; }

I personally had the issue in rescript-schema. Had to solve with a hacky workaround https://github.com/DZakh/rescript-schema/blob/617ba9f38e1f06e0f4f98be029b78bcaa10898c7/src/S_Core.res#L413-L419

cknitt · 2024-09-03T07:13:05Z

Could you undo the CHANGELOG formatting changes?

cknitt · 2024-09-24T08:03:11Z

@zth wrote in #6984 (comment), referring to this PR:

Me and @cristianoc discussed a bit, and we think it's fine to merge that PR as is, because the editor extension doesn't touch any of these things where we've extended the AST. However, we need to figure out a real solution for this mid to long term.

So @DZakh could you rebase so that we can get this merged (and I guess you need to include the revert of the revert from #7016 first)?

DZakh · 2024-09-26T07:33:34Z

I'll do after the PR is merged #6984

cometkim · 2024-09-26T09:11:48Z

deadlock 😄 #6984 (comment)

DZakh marked this pull request as draft August 26, 2024 19:11

DZakh force-pushed the differentiate-exception-extensions-2 branch from ed0627e to 574b53b Compare August 26, 2024 19:52

DZakh commented Aug 26, 2024

View reviewed changes

DZakh changed the title ~~Differentiate exception extensions part 2~~ Use RescriptError for exceptions Aug 26, 2024

DZakh marked this pull request as ready for review August 27, 2024 20:07

DZakh requested a review from cometkim August 28, 2024 05:17

cknitt reviewed Aug 28, 2024

View reviewed changes

DZakh requested a review from cknitt August 28, 2024 09:23

DZakh force-pushed the differentiate-exception-extensions-2 branch from 311ba6e to e10a8c0 Compare August 28, 2024 09:30

cknitt approved these changes Aug 28, 2024

View reviewed changes

cknitt requested a review from cristianoc August 28, 2024 09:40

cristianoc reviewed Aug 28, 2024

View reviewed changes

cometkim reviewed Aug 29, 2024

View reviewed changes

cknitt mentioned this pull request Aug 31, 2024

Move VS Code extension into compiler repo #6994

Open

DZakh added 6 commits September 3, 2024 10:30

Pass is_exception to Blk_extension

3978f09

Pass through the is_exception everywhere it's needed

e1d0587

Use RescriptError class for exceptions

13f9227

Add caml_js_exceptions imports

bc0aba9

Commit output files

98cba2e

Fix internalAnyToExn

cc31e08

DZakh force-pushed the differentiate-exception-extensions-2 branch from e10a8c0 to 66f833b Compare September 3, 2024 06:32

Add changelog

1b392d9

DZakh force-pushed the differentiate-exception-extensions-2 branch from 66f833b to 1b392d9 Compare September 3, 2024 07:19

cknitt mentioned this pull request Sep 6, 2024

Revert #6611 #7016

Merged

cometkim mentioned this pull request Sep 22, 2024

Drop Caml runtimes and primitives #6984

Merged

	match i with 0 -> Literals.exception_id \| i -> "_" ^ string_of_int i)
	match i with 0 -> Literals.exception_id \| i -> "_" ^ string_of_int (i - 1))

Use RescriptError for exceptions #6979

Are you sure you want to change the base?

Use RescriptError for exceptions #6979

Conversation

DZakh commented Aug 26, 2024

DZakh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DZakh commented Aug 26, 2024

cometkim commented Aug 26, 2024 • edited Loading

DZakh commented Aug 27, 2024

cometkim commented Aug 27, 2024

DZakh commented Aug 27, 2024

cometkim commented Aug 27, 2024

cometkim commented Aug 27, 2024

DZakh commented Aug 27, 2024

cknitt commented Aug 27, 2024

DZakh commented Aug 27, 2024

DZakh commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Aug 28, 2024

DZakh commented Aug 28, 2024

DZakh commented Aug 28, 2024

cknitt commented Aug 28, 2024

DZakh commented Aug 28, 2024

cristianoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Code Review Report: Impact of Proposed Change in Runtime Representation

High-Level Summary

Test Results

Supporting Code

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cometkim Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cknitt commented Sep 3, 2024

cknitt commented Sep 24, 2024

DZakh commented Sep 26, 2024

cometkim commented Sep 26, 2024

cometkim commented Aug 26, 2024 •

edited

Loading

cometkim Aug 29, 2024 •

edited

Loading