Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Caml_obj equal function to support errors #6937

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Improve Caml_obj equal function to support errors #6937

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a95869
Improve Caml_obj equal function to support errors
DZakh Aug 4, 2024
de469bf
Fix tests
DZakh Aug 4, 2024
75a616e
Update changelog
DZakh Aug 4, 2024
e06248a
Revert changelog formatting
DZakh Aug 7, 2024
c912856
Fix caml_obj after review
DZakh Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 22 additions & 17 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@

# 12.0.0-alpha.2 (Unreleased)

#### :rocket: New Feature

- Updated `==` to improve performance and start supporting comparison of `Error` instances. Also, it stopped raising error on comparison of two different functions. https://github.com/rescript-lang/rescript-compiler/pull/6937

#### :nail_care: Polish

- Improve formatting in the generated js code. https://github.com/rescript-lang/rescript-compiler/pull/6932
Expand Down Expand Up @@ -245,7 +249,7 @@

#### :rocket: New Feature

- Experimental support of tagged template literals, e.g. ```sql`select * from ${table}```. https://github.com/rescript-lang/rescript-compiler/pull/6250
- Experimental support of tagged template literals, e.g. `` sql`select * from ${table} ``. https://github.com/rescript-lang/rescript-compiler/pull/6250
cknitt marked this conversation as resolved.
Show resolved Hide resolved
- Experimental support for generic/custom JSX transforms. https://github.com/rescript-lang/rescript-compiler/pull/6565
- `dict` is now a builtin type. https://github.com/rescript-lang/rescript-compiler/pull/6590

Expand Down Expand Up @@ -425,7 +429,7 @@ No changes compared to rc.9.

#### :boom: Breaking Change

- Stop mangling object field names. If you had objects with field names containing "__" or leading "_", they won't be mangled in the compiled JavaScript and represented as it is without changes. https://github.com/rescript-lang/rescript-compiler/pull/6354
- Stop mangling object field names. If you had objects with field names containing "\__" or leading "_", they won't be mangled in the compiled JavaScript and represented as it is without changes. https://github.com/rescript-lang/rescript-compiler/pull/6354

#### :bug: Bug Fix

Expand Down Expand Up @@ -480,7 +484,7 @@ No changes compared to rc.9.

#### :rocket: New Feature

- Introduced a new `%ffi` extension (*experimental* - not for production use!) that provides a more robust mechanism for JavaScript function interoperation by considering function arity in type constraints. This enhancement improves safety when dealing with JavaScript functions by enforcing type constraints based on the arity of the function. https://github.com/rescript-lang/rescript-compiler/pull/6251
- Introduced a new `%ffi` extension (_experimental_ - not for production use!) that provides a more robust mechanism for JavaScript function interoperation by considering function arity in type constraints. This enhancement improves safety when dealing with JavaScript functions by enforcing type constraints based on the arity of the function. https://github.com/rescript-lang/rescript-compiler/pull/6251
- Extended untagged variants with function types. https://github.com/rescript-lang/rescript-compiler/pull/6279

#### :boom: Breaking Change
Expand Down Expand Up @@ -558,14 +562,14 @@ No changes compared to rc.9.

#### :bug: Bug Fix

- Fix broken formatting in uncurried mode for functions with _ placeholder args. https://github.com/rescript-lang/rescript-compiler/pull/6148
- Fix broken formatting in uncurried mode for functions with \_ placeholder args. https://github.com/rescript-lang/rescript-compiler/pull/6148
- Fix issue where spreading record types with optional labels would not have their labels preserved as optional. https://github.com/rescript-lang/rescript-compiler/pull/6154
- Fix error location to be the type with the spreads when spreading record types with duplicate labels. https://github.com/rescript-lang/rescript-compiler/pull/6157
- Disable warning on `@inline` attibute on uncurried functions. https://github.com/rescript-lang/rescript-compiler/pull/6152
- Support doc comments on arguments of function types. https://github.com/rescript-lang/rescript-compiler/pull/6161
- Fix issue with record type coercion and unboxed. https://github.com/rescript-lang/rescript-compiler/issues/6158
- Fixed subtype checking for record types with "@as" attributes: The subtype relationship now takes into account the compatibility of "@as" attributes between corresponding fields, ensuring correctness in runtime representation.
https://github.com/rescript-lang/rescript-compiler/issues/6158
https://github.com/rescript-lang/rescript-compiler/issues/6158
- Emit directive above header comment. https://github.com/rescript-lang/rescript-compiler/pull/6172
- Add error message to private extension. https://github.com/rescript-lang/rescript-compiler/pull/6175

Expand Down Expand Up @@ -594,7 +598,6 @@ No changes compared to rc.9.

- Special case generation of uncurried functions with 1 argument of unit type so they don't take a parameter. https://github.com/rescript-lang/rescript-compiler/pull/6131


# 11.0.0-alpha.1

#### :rocket: Main New Feature
Expand All @@ -605,16 +608,16 @@ No changes compared to rc.9.
#### :rocket: New Feature

- Add support for uncurried mode: a mode where everything is considered uncurried, whether with or without the `.`. This can be turned on with `@@uncurried` locally in a file. For project-level configuration in `bsconfig.json`, there's a boolean config `"uncurried"`, which propagates to dependencies, to turn on uncurried mode.
Since there's no syntax for partial application in this new mode, introduce `@res.partial foo(x)` to express partial application. This is temporary and will later have some surface syntax.
Make uncurried functions a subtype of curried functions, and allow application for uncurried functions.
The `make` function of components is generated as an uncurried function.
Use best effort to determine the config when formatting a file.
https://github.com/rescript-lang/rescript-compiler/pull/5968 https://github.com/rescript-lang/rescript-compiler/pull/6080 https://github.com/rescript-lang/rescript-compiler/pull/6086 https://github.com/rescript-lang/rescript-compiler/pull/6087
Since there's no syntax for partial application in this new mode, introduce `@res.partial foo(x)` to express partial application. This is temporary and will later have some surface syntax.
Make uncurried functions a subtype of curried functions, and allow application for uncurried functions.
The `make` function of components is generated as an uncurried function.
Use best effort to determine the config when formatting a file.
https://github.com/rescript-lang/rescript-compiler/pull/5968 https://github.com/rescript-lang/rescript-compiler/pull/6080 https://github.com/rescript-lang/rescript-compiler/pull/6086 https://github.com/rescript-lang/rescript-compiler/pull/6087
- Customization of runtime representation of variants. This is work in progress. E.g. some restrictions on the input. See comments of the form "TODO: put restriction on the variant definitions allowed, to make sure this never happens". https://github.com/rescript-lang/rescript-compiler/pull/6095
- Introduce untagged variants https://github.com/rescript-lang/rescript-compiler/pull/6103
- Add support for unary uncurried pipe in uncurried mode https://github.com/rescript-lang/rescript-compiler/pull/5804
- Add support for partial application of uncurried functions: with uncurried application one can provide a
subset of the arguments, and return a curried type with the remaining ones https://github.com/rescript-lang/rescript-compiler/pull/5805
subset of the arguments, and return a curried type with the remaining ones https://github.com/rescript-lang/rescript-compiler/pull/5805
- Add support for uncurried externals https://github.com/rescript-lang/rescript-compiler/pull/5815 https://github.com/rescript-lang/rescript-compiler/pull/5819 https://github.com/rescript-lang/rescript-compiler/pull/5830 https://github.com/rescript-lang/rescript-compiler/pull/5894
- Parser/Printer: unify uncurried functions of arity 0, and of arity 1 taking unit. There's now only arity 1 in the source language. https://github.com/rescript-lang/rescript-compiler/pull/5825
- Add support for default arguments in uncurried functions https://github.com/rescript-lang/rescript-compiler/pull/5835
Expand All @@ -636,12 +639,12 @@ subset of the arguments, and return a curried type with the remaining ones https
- `rescript convert <reason files>`
- Remove obsolete built-in project templates and the "rescript init" functionality. This is replaced by [create-rescript-app](https://github.com/rescript-lang/create-rescript-app) which is maintained separately.
- Do not attempt to build ReScript from source on npm postinstall for platforms without prebuilt binaries anymore.
- Made pinned dependencies transitive: if *a* is a pinned dependency of *b* and *b* is a pinned dependency of *c*, then *a* is implicitly a pinned dependency of *c*. This change is only breaking if your build process assumes non-transitivity.
- Made pinned dependencies transitive: if _a_ is a pinned dependency of _b_ and _b_ is a pinned dependency of _c_, then _a_ is implicitly a pinned dependency of _c_. This change is only breaking if your build process assumes non-transitivity.
- Curried after uncurried is not fused anymore: `(. x) => y => 3` is not equivalent to `(. x, y) => 3` anymore. It's instead equivalent to `(. x) => { y => 3 }`.
Also, `(. int) => string => bool` is not equivalen to `(. int, string) => bool` anymore.
These are only breaking changes for unformatted code.
Also, `(. int) => string => bool` is not equivalen to `(. int, string) => bool` anymore.
These are only breaking changes for unformatted code.
- Exponentiation operator `**` is now right-associative. `2. ** 3. ** 2.` now compile to `Math.pow(2, Math.pow(3, 2))` and not anymore `Math.pow(Math.pow(2, 3), 2)`. Parentheses can be used to change precedence.
- Remove unsafe ``` j`$(a)$(b)` ``` interpolation deprecated in compiler version 10 https://github.com/rescript-lang/rescript-compiler/pull/6068
- Remove unsafe `` j`$(a)$(b)` `` interpolation deprecated in compiler version 10 https://github.com/rescript-lang/rescript-compiler/pull/6068
- Remove deprecated module `Printexc`
- `@deriving(jsConverter)` not supported anymore for variant types https://github.com/rescript-lang/rescript-compiler/pull/6088
- New representation for variants, where the tag is a string instead of a number. https://github.com/rescript-lang/rescript-compiler/pull/6088
Expand Down Expand Up @@ -690,19 +693,21 @@ These are only breaking changes for unformatted code.
# 10.1.4

#### :bug: Bug Fix

- Fix implementation of directives https://github.com/rescript-lang/rescript-compiler/pull/6052
- Fix issue if the `lib` dir is included in the sources of bsconfig.json https://github.com/rescript-lang/rescript-compiler/pull/6055
- Fix issue with string escape in pattern match https://github.com/rescript-lang/rescript-compiler/pull/6062
- Fix issue with literal comparison of string constants https://github.com/rescript-lang/rescript-compiler/pull/6065

#### :rocket: New Feature

- Add support for toplevel `await` https://github.com/rescript-lang/rescript-compiler/pull/6054

#### :nail_care: Polish

- Better error message for extension point https://github.com/rescript-lang/rescript-compiler/pull/6057
- Improve format check help https://github.com/rescript-lang/rescript-compiler/pull/6056
- Deprecate unsafe ``` j`$(a)$(b)` ``` interpolation: use string templates ``` `${a}${b}` ``` instead https://github.com/rescript-lang/rescript-compiler/pull/6067
- Deprecate unsafe `` j`$(a)$(b)` `` interpolation: use string templates `` `${a}${b}` `` instead https://github.com/rescript-lang/rescript-compiler/pull/6067

# 10.1.3

Expand Down
4 changes: 0 additions & 4 deletions jscomp/core/js_exp_make.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,6 @@ let eight_int_literal : t =
let nine_int_literal : t =
{ expression_desc = Number (Int { i = 9l; c = None }); comment = None }

let obj_int_tag_literal : t =
{ expression_desc = Number (Int { i = 248l; c = None }); comment = None }

let int ?comment ?c i : t = { expression_desc = Number (Int { i; c }); comment }

let bigint ?comment sign i : t = { expression_desc = Number (BigInt {positive=sign; value=i}); comment}
Expand All @@ -330,7 +327,6 @@ let small_int i : t =
| 7 -> seven_int_literal
| 8 -> eight_int_literal
| 9 -> nine_int_literal
| 248 -> obj_int_tag_literal
| i -> int (Int32.of_int i)

let true_ : t = { comment = None; expression_desc = Bool true }
Expand Down
1 change: 0 additions & 1 deletion jscomp/core/js_exp_make.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ val zero_int_literal : t

(* val one_int_literal : t *)
val zero_float_lit : t
(* val obj_int_tag_literal : t *)

val zero_bigint_literal : t

Expand Down
24 changes: 10 additions & 14 deletions jscomp/runtime/caml_hash.res
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,22 +117,18 @@ let hash = (count: int, _limit, seed: int, obj: Obj.t): int => {
if size != 0 {
let obj_tag = Obj.tag(obj)
let tag = lor(lsl(size, 10), obj_tag)
if obj_tag == 248 /* Obj.object_tag */ {
s.contents = hash_mix_int(s.contents, (Obj.obj(Obj.field(obj, 1)): int))
} else {
s.contents = hash_mix_int(s.contents, tag)
let block = {
let v = size - 1
if v < num.contents {
v
} else {
num.contents
}
}
for i in 0 to block {
push_back(queue, Obj.field(obj, i))
s.contents = hash_mix_int(s.contents, tag)
let block = {
let v = size - 1
if v < num.contents {
v
} else {
num.contents
}
}
for i in 0 to block {
push_back(queue, Obj.field(obj, i))
}
} else {
let size: int = %raw(`function(obj,cb){
var size = 0
Expand Down
57 changes: 23 additions & 34 deletions jscomp/runtime/caml_obj.res
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,7 @@ let rec compare = (a: Obj.t, b: Obj.t): int =>
} else {
let tag_a = Obj.tag(a)
let tag_b = Obj.tag(b)
if tag_a == 248 /* object/exception */ {
Pervasives.compare((Obj.magic(Obj.field(a, 1)): int), Obj.magic(Obj.field(b, 1)))
} else if tag_a == 251 /* abstract_tag */ {
raise(Invalid_argument("equal: abstract value"))
} else if tag_a != tag_b {
if tag_a != tag_b {
if tag_a < tag_b {
-1
} else {
Expand Down Expand Up @@ -303,53 +299,46 @@ type eq = (Obj.t, Obj.t) => bool
basic type is not the same, it will not equal
*/
let rec equal = (a: Obj.t, b: Obj.t): bool =>
/* front and formoest, we do not compare function values */
if a === b {
true
} else {
let a_type = Js.typeof(a)
if (
a_type == "string" ||
(a_type == "number" ||
(a_type == "bigint" ||
(a_type == "boolean" ||
(a_type == "undefined" || a === %raw(`null`)))))
) {
if a_type !== "object" || a === %raw(`null`) {
Comment on lines -311 to +306
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same, but shorter, faster, easier to maintain, and prevents from forgetting symbol and other types.

false
} else {
let b_type = Js.typeof(b)
if a_type == "function" || b_type == "function" {
raise(Invalid_argument("equal: functional value"))
} /* first, check using reference equality */
Comment on lines -321 to -323
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we checked the functions using strict equal a === b, it doesn't make sense to raise an error at this point.

Copy link
Collaborator

@cristianoc cristianoc Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?
The error says you cannot compare functional values. (Unless they are identical, in which case you can).
We might change the stance on this, but both choices are valid design choices.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think according to the funciton name equal it makes sense that when you compare two non-identical function values, it returns false. Also, it improves performance for non function values, since there's no need to specifically check for it.

else if (
/* a_type = "object" || "symbol" */
b_type == "number" || (b_type == "bigint" || (b_type == "undefined" || b === %raw(`null`)))
) {
if b_type !== "object" || b === %raw(`null`) {
Comment on lines -325 to +310
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good example of forgetting checking various types

false
} else {
/* [a] [b] could not be null, so it can not raise */
let tag_a = Obj.tag(a)
let tag_b = Obj.tag(b)
if tag_a == 248 /* object/exception */ {
Obj.magic(Obj.field(a, 1)) === Obj.magic(Obj.field(b, 1))
} else if tag_a == 251 /* abstract_tag */ {
raise(Invalid_argument("equal: abstract value"))
Comment on lines -333 to -336
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed legacy check

} else if tag_a != tag_b {
if tag_a !== tag_b {
false
Comment on lines +316 to 317
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure of the usefulness of the optimisation 🤔

But let's keep it.

} else {
} else if O.isArray(a) {
let len_a = Obj.size(a)
let len_b = Obj.size(b)
Comment on lines +318 to 320
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Length check makes sense only for arrays, so moved it inside of the if O.isArray(a) block

if len_a == len_b {
if O.isArray(a) {
aux_equal_length((Obj.magic(a): array<Obj.t>), (Obj.magic(b): array<Obj.t>), 0, len_a)
} else if %raw(`a instanceof Date && b instanceof Date`) {
!(Js.unsafe_gt(a, b) || Js.unsafe_lt(a, b))
} else {
aux_obj_equal(a, b)
}
if len_a !== len_b {
false
} else {
aux_equal_length((Obj.magic(a): array<Obj.t>), (Obj.magic(b): array<Obj.t>), 0, len_a)
}
} else if %raw(`a instanceof Error`) {
let a: {..} = Obj.magic(a)
let b: {..} = Obj.magic(b)
if %raw(`b instanceof Error`) && a["message"] === b["message"] {
equal(a["clause"], b["clause"])
cknitt marked this conversation as resolved.
Show resolved Hide resolved
} else {
false
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for in doesn't work on Error, so have a special case for them.

} else if %raw(`a instanceof Date`) {
if %raw(`b instanceof Date`) {
Comment on lines +334 to +335
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split the check so it earlier returns false when b is not an instance of Date without running some unexpected code.

!(Js.unsafe_gt(a, b) || Js.unsafe_lt(a, b))
} else {
false
}
} else {
aux_obj_equal(a, b)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken a look at the generated code for the function, and it's not very optimised. Not the goal of the PR, so kept it as it is.

}
}
}
Expand Down
Loading