Unified operators #7057

cometkim · 2024-09-28T01:50:14Z

Trying to solve #6477, but without implicit open

Rationale

As explained in #6477, we have the "infix explosion" problem. To relax it, I added some specialization to both type inference and primitive inspired by F#

https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/symbol-and-operator-reference/arithmetic-operators#operators-and-type-inference

The idea is, to extend the type inference rules for primitive operations defined in Pervasives to specialize them for predefined types but fall back to int if they cannot be inferred.

let t1 = 1 + 2     // => external \"+": (int, int) => int = "%addint"
let t2 = 1. + 2.   // => external \"+": (float, float) => float = "%addfloat" 
let t3 = "1" + "2" // => external \"+": (string, string) => string = "%string_concat"
let t4 = 1n + 2n   // => external \"+": (bigint, bigint) => bigint = "%addbigint"

let fn1 = (a, b) => a + b        // => external \"+": (int, int) => int = "%addint"
let fn2 = (a: float, b) => a + b // => external \"+": (float, float) => float = "%addfloat" 
let fn3 = (a, b: float) => a + b // => external \"+": (float, float) => float = "%addfloat" 

let inv1 = (a: int, b: float) => a + b  // => external \"+": (int, int) => int = "%addint"
//                                   ^ error: cannot apply float here, expected int

This doesn't make a breaking change since the existing operations are already defined on int.

However, we can introduce them as unified operators for the new codebase. They are still safe, as they are always specialized and not polymorphic.

Specialized operators:

Op	Primitive	supported types	ready?
`~+`	`%plus`	`int`, `float`, `bigint`	✔️
`~-`	`%neg`	`int`, `float`, `bigint`	✔️
`+`	`%add`	`int`, `float` , `bigint`, `string`	✔️
`-`	`%sub`	`int`, `float`, `bigint`	✔️
`*`	`%mul`	`int`, `float`, `bigint`	✔️
`/`	`%div`	`int`, `float`, `bigint`	✔️
`mod`	`%mod`	`int`, `float`, `bigint`	✔️
`**`	`%exp`	`int`, `float`, `bigint`
`&`	`%bitand` (formerly `%land`)	`int`, `bigint`
`\|`	`%bitor` (formerly `%lor`)	`int`, `bigint`
`^`	`%bitxor` (formerly `%lxor`)	`int`, `bigint`
`~`	`%bitnot` (formerly `%lnot`)	`int`, `bigint`
`<<`	`%lsl`	`int`, `bigint`
`>>`	`%asr`	`int`, `bigint`
`>>>`	`%lsr`	`int`
`abs`	`%abs`	`int`, `float`, `bigint`
`min`	`%min`	`int`, `float`, `bigint`, `string`
`max`	`%max`	`int`, `float`, `bigint`, `string`

Side note

This PR doesn't attempt to add custom ops or polymorphic ops, but that's the specialized custom ops already possible by opening a module.

Polymorphic ops should be deprecated. They are unpredictable and inefficient, as explained in #7031. So, in future versions, comparisons will behave consistently with this change. It will be a breaking change, but we don't have a better alternative yet.

zth · 2024-10-01T19:44:37Z

@cometkim you waiting on anything from any of us on this one?

cometkim · 2024-10-04T00:23:26Z

@cometkim you waiting on anything from any of us on this one?

No, I just took a break from coding over the Korean holidays.

zth · 2024-10-04T04:45:32Z

@cometkim you waiting on anything from any of us on this one?

No, I just took a break from coding over the Korean holidays.

Enjoy your holidays!

cometkim · 2024-11-04T17:48:22Z

It now works for basic arithmetic operators

tests/build_tests/super_errors/expected/primitives1.res.expected

cristianoc

This approach looks good to me, both the design and the implementation.
Left some minor comments.

@zth @cknitt any comments on the design?

cristianoc · 2024-11-05T09:40:58Z

compiler/ml/translcore.ml

+  match entry with
+  | Some {specialization} -> (
+    match specialization with
+    | {int}


I was wondering if the when clause in this case is complete. But I guess this case is just unnecessary and can be removes as it is already expressed as the last default case?

This always takes precedence int over other types. (Rule 1-2) The last default case is a fallback strategy. (Rule 3)

Can you give an example of any change after removing this first case?
All the other cases seem to have incompatible when clauses, so it would fall back to the last case no matter what.
Unless I'm missing something.

You are right. There is no logical difference in the behavior after removing that case. I was just thinking of the computational difference. In the existing codebase, I assume the first case is hit the most frequently (since it was originally int-only).

It's just a detail anyway. Not much difference. Whatever you think is best.

Yeah, it probably doesn't matter, but I'll leave it as is because it seems easier to understand the intent.

cristianoc · 2024-11-05T09:41:33Z

compiler/ml/translcore.ml

-    if String.length prim_name > 0 && prim_name.[0] = '%' then
-      raise (Error (loc, Unknown_builtin_primitive prim_name));
-    Pccall prim
+    | [arg1] | [arg1; _] -> translate_unified_ops prim env arg1.exp_type


why also [arg1] with only 1 argument?

There is a couple of unary primitive to support, %pos and %neg

Yes I noticed all the other functions don't have unaries, so if they're planned for this PR great.

cristianoc · 2024-11-05T09:43:08Z

compiler/ml/typecore.ml

+      let rhs = type_exp env rhs_expr in
+      let rhs_type = expand_head env rhs.exp_type in
+      let lhs, rhs, result_type =
+        (* rule 1. *)


Some overall comment above should summarise what rule1, rule2, rule3 are, before we get this far into the code.

I can leave a link here to read the unified_ops.ml module first.

Or would you prefer to duplicate the same explanation here?

Link sounds perfect.
Or should the code move too? Not sure.
Just wondering why the description is in one file and the code in an other. Perhaps dependencies or similarity to other code.

Just wondering why the description is in one file and the code in an other. Perhaps dependencies or similarity to other code.

I wanted to keep everything in a single file if possible. Unified_ops should be the only module that manages the list of supported operators.

But I used a utility from typecore.ml like type_exp. It isn't easy to replicate as it is defined recursively.

cristianoc · 2024-11-05T09:50:11Z

* (%mul): While unusual for most data types, * is often supported for strings in some languages (like Python) for repetition. For example, "abc" * 3 results in "abcabcabc" in Python.

seems a pushing it a bit far -- but just reporting here what a quick search returned

cknitt · 2024-11-05T12:35:38Z

This approach looks good to me, both the design and the implementation. Left some minor comments.

@zth @cknitt any comments on the design?

Looks good to me, too! Awesome work @cometkim!

zth · 2024-11-05T13:13:44Z

Agreed, looks great! 👏

cometkim · 2024-11-05T13:14:14Z

* (%mul): While unusual for most data types, * is often supported for strings in some languages (like Python) for repetition. For example, "abc" * 3 results in "abcabcabc" in Python.

seems a pushing it a bit far -- but just reporting here what a quick search returned

Sounds like an abbreviation of String.prototype.repeat

I'm not sure if that will be useful or cleaner.

cometkim · 2024-11-05T15:42:19Z

I haven't added char here (yet?), but it would be confusing.

'a' * 3 looks like be "aaa", but that doesn't match the mechanism (char <> string) and it doesn't match the semantics (char = int) either.

cometkim · 2024-11-05T17:03:50Z

For other operators, I need to implement/rework additional primitives. It seems better to merge this first and then do it later on.

cristianoc

Sounds great!
Would you add a change log before merging?
We’ll also need to set documentation tasks for this.

cometkim · 2024-11-06T14:03:34Z

compiler/ml/unified_ops.ml

+  specialization: specialization;
+}
+
+let builtin x = Primitive_modules.pervasives ^ "." ^ x


Ohhhhh right it's pervasives_mini, not pervasive. tricky 😅

cometkim · 2024-11-06T18:09:15Z

changelog added

cometkim · 2024-11-06T18:15:10Z

documentation issue opened rescript-lang/rescript-lang.org#934

This comment was marked as outdated.

Sign in to view

cometkim changed the title ~~PoC: generic infix operators~~ PoC: Unified infix operators Sep 28, 2024

This comment was marked as resolved.

Sign in to view

cometkim mentioned this pull request Oct 4, 2024

Compiled js min/max value with bigint #7062

Open

cometkim force-pushed the generic-infix branch from 7d0dcee to 6eaf032 Compare October 11, 2024 19:09

cknitt assigned cometkim Oct 24, 2024

cometkim changed the title ~~PoC: Unified infix operators~~ PoC: Unified operators Nov 4, 2024

cometkim force-pushed the generic-infix branch from 604621b to f8a673e Compare November 4, 2024 15:12

cometkim requested review from cknitt, zth and cristianoc November 4, 2024 17:55

cometkim marked this pull request as ready for review November 4, 2024 18:01

cometkim changed the title ~~PoC: Unified operators~~ Unified operators Nov 4, 2024

cometkim commented Nov 5, 2024

View reviewed changes

tests/build_tests/super_errors/expected/primitives1.res.expected Show resolved Hide resolved

cristianoc reviewed Nov 5, 2024

View reviewed changes

cristianoc approved these changes Nov 5, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

cometkim commented Nov 6, 2024

View reviewed changes

PoC: generic infix operators

7205088

cometkim added 15 commits November 7, 2024 03:05

try less diff

0005e2f

wip: unified ops

4bc812d

rewrite type translation

7551045

revert changes on comparison

64d4e15

done implement unified_ops translation

19e01b7

migrate other arithmetic operators

2c05e5a

rename test and more cases

6607973

link comments

cd7aa38

remove unused clause

bcccd76

rename primitive

68fa0dc

sync Pervasives_mini

adc65bf

keep arg labels

efa0058

format

15595d2

note on pervasives_mini

1791379

add changelog

1ef4469

cometkim force-pushed the generic-infix branch from dff2e3f to 1ef4469 Compare November 6, 2024 18:08

cometkim mentioned this pull request Nov 6, 2024

Document unified operators rescript-lang/rescript-lang.org#934

Open

cknitt approved these changes Nov 6, 2024

View reviewed changes

cometkim merged commit b0211f3 into rescript-lang:master Nov 6, 2024
20 checks passed

cometkim deleted the generic-infix branch November 6, 2024 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unified operators #7057

Unified operators #7057

cometkim commented Sep 28, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as resolved.

zth commented Oct 1, 2024

cometkim commented Oct 4, 2024

zth commented Oct 4, 2024

cometkim commented Nov 4, 2024

cristianoc left a comment

cristianoc Nov 5, 2024

cometkim Nov 5, 2024

cristianoc Nov 5, 2024

cometkim Nov 5, 2024

cristianoc Nov 5, 2024

cometkim Nov 5, 2024

cristianoc Nov 5, 2024

cometkim Nov 5, 2024

cristianoc Nov 5, 2024

This comment was marked as resolved.

cristianoc Nov 5, 2024

cometkim Nov 5, 2024

cristianoc Nov 5, 2024

cometkim Nov 5, 2024

cristianoc commented Nov 5, 2024

cknitt commented Nov 5, 2024

zth commented Nov 5, 2024

cometkim commented Nov 5, 2024

cometkim commented Nov 5, 2024 •

edited

Loading

cometkim commented Nov 5, 2024

cristianoc left a comment

This comment was marked as resolved.

This comment was marked as resolved.

cometkim Nov 6, 2024

cometkim commented Nov 6, 2024

cometkim commented Nov 6, 2024

Unified operators #7057

Unified operators #7057

Conversation

cometkim commented Sep 28, 2024 • edited Loading

Rationale

Side note

This comment was marked as outdated.

This comment was marked as resolved.

zth commented Oct 1, 2024

cometkim commented Oct 4, 2024

zth commented Oct 4, 2024

cometkim commented Nov 4, 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

Choose a reason for hiding this comment

This comment was marked as resolved.

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

cristianoc commented Nov 5, 2024

cknitt commented Nov 5, 2024

zth commented Nov 5, 2024

cometkim commented Nov 5, 2024

cometkim commented Nov 5, 2024 • edited Loading

cometkim commented Nov 5, 2024

cristianoc left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

cometkim commented Nov 6, 2024

cometkim commented Nov 6, 2024

cometkim commented Sep 28, 2024 •

edited

Loading

cometkim commented Nov 5, 2024 •

edited

Loading