Use RescriptError for exceptions

[DZakh]

[DZakh]

The change example

[cometkim]

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]

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]

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]

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]

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]

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]

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

[cknitt]

@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]

It's not the complete implementation, but it works

[DZakh]

@cknitt @cristianoc ready for review

[nojaf]

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

[DZakh]

@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]

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

[cknitt]

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

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

[DZakh]

Solves: https://github.com/rescript-lang/rescript-compiler/issues/6929 https://github.com/rescript-lang/rescript-compiler/issues/6972 https://github.com/rescript-lang/rescript-compiler/pull/6931

[cknitt]

Could you undo the CHANGELOG formatting changes?

[cknitt]

@zth wrote in https://github.com/rescript-lang/rescript-compiler/pull/6984#issuecomment-2368330859, 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]

I'll do after the PR is merged https://github.com/rescript-lang/rescript-compiler/pull/6984

[cometkim]

deadlock 😄 https://github.com/rescript-lang/rescript-compiler/pull/6984#issuecomment-2366934375

[cknitt]

@DZakh I think this could be picked up again if/when you have time.

#6984 is merged, and AST compatibility with the editor extension are not an issue anymore after https://github.com/rescript-lang/rescript-vscode/pull/1055.

[DZakh]

I'll have time in the second part of the Feburary