Draft: Adding restriction to LPython generics #921

ansharlubis · 2022-08-05T18:10:55Z

No description provided.

certik · 2022-08-05T18:19:01Z

Awesome, great job!

Let's add some tests for error messages when a wrong type is provided.

certik · 2022-08-21T06:29:36Z

(Ping me once tests pass and this is ready for review, or if you need help fixing anything.)

ansharlubis · 2022-08-21T06:59:25Z

@certik It passed the tests. Currently, there are four restrictions: SupportsPlus, SupportsZero, Divisible, and Any. The operations supported are still only addition and division on integers and reals.

The generic mean also worked in both CPython and LLVM.

certik · 2022-08-21T08:03:18Z

src/libasr/pass/instantiate_template.cpp

-                    case (ASR::binopType::Sub): { result = left_value - right_value; break; }
-                    case (ASR::binopType::Mul): { result = left_value * right_value; break; }
+                    // case (ASR::binopType::Sub): { result = left_value - right_value; break; }
+                    // case (ASR::binopType::Mul): { result = left_value * right_value; break; }


Why is this commented out?

I haven't made any tests with subtraction and multiplication and there is no restriction defined that corresponds to these operations yet.

certik · 2022-08-21T08:04:02Z

src/libasr/ASR.asdl

@@ -346,6 +350,7 @@ cast_kind
    | IntegerToReal
    | LogicalToReal
    | RealToReal
+    | TemplateToReal


When is this cast used?

It's used in generics_list_01.py. The type of sum in the division sum/k is generic, it may or may not have to be cast into a real number for the division.

I see. It's casting to real, but we don't know from "what". Good for now. We can iterate on the design later.

certik · 2022-08-21T08:07:41Z

tests/errors/restriction_error_01.py

+T = TypeVar('T', bound=Any)
+
+def f(x: T, y: T) -> T:
+    return x + y


We need to brainstorm the name for this --- and whether to even allow it at all.

I believe in Rust (or Go?) "Any" means "any type" can be used, which means you are not allowed to do almost anything on it, specifically you are not allowed to do "a+b", because that does not work with any type, only with types that support it.

Also, how is this checked --- it seems that here we would still need to go into the template to check that the type can be used?

It seems to me we should not allow this at all. That is, "Any" could mean "any user type will work here", i.e., "no restrictions", but then you can't use "+" in the function, so the above should not compile. You would have to specify restrictions here.

Yes, that's an example of a restriction error. It wouldn't pass the type check. Maybe the name Any is a bit inappropriate and Assignable would be better for it.

The check is very simple, the binary operation x + y would require both x and y to have SupportsPlus in their bounds if they are type parameters.

Ah, I forgot that this is an "error", this is good. There is no problem.

certik

Excellent job on this!

There are a few design questions here, that we should brainstorm.

It seems to me that with this PR, we should always require restrictions. That is, when compiling the templated code, we either give an error message right away, or if not, then this can can always be instantiated without errors for any user type that satisfies the restrictions, and whether it satisfies the restrictions is determined at the call site.

@czgdp1807 let me know what you think as well.

certik · 2022-08-21T08:10:39Z

Also should we call it restrictions or requirements? There are other possible names too.

ansharlubis · 2022-08-21T08:25:38Z

Yes, I'm following the original idea of having it strongly-typed, so that every type parameter has to be given the appropriate restrictions. Thinking from the way I normally program in Python, it would feel a bit unfriendly towards the users to have to specify every single restriction. Then again, I'm not really sure how much the trade-off of having to type check every function instantiation is.

certik · 2022-08-21T08:35:55Z

Another word is "constraint".

To make it friendly, we have to create predefined restrictions like Number, or String, for the most used cases. I would keep the feature that you can fully check everything at the call site, without going into the function. It's called "strong concepts". The question is whether to allow "weak concepts" or "no concepts", where you are required to go into the templated function at each call site, and I would not allow that at all.

I am looking into how Go now does it:

https://go.dev/doc/tutorial/generics
https://go.dev/blog/intro-generics

They call it "constraint". It seems you cannot use generics without constraints (restrictions).

certik

I think this is good to go in.

After the git history is polished.

@czgdp1807, can you have a look too?

There are small things that we can fix up later. Is there anything major that needs fixing before we can merge this?

certik · 2022-08-21T13:20:17Z

(I resolved the conflicts with master, hopefully correctly.)

certik · 2022-08-21T13:45:14Z

I screwed it up with my "conflicts resolve":

/home/runner/work/lpython/lpython/lpython-0.3.0-702-g953fa7bc8/src/lpython/semantics/python_ast_to_asr.cpp:3000:60: error: ‘value’ was not declared in this scope
 3000 |             ASR::ttype_t *value_type = ASRUtils::expr_type(value);
      |                                                            ^~~~~

certik · 2022-08-21T13:45:40Z

@ansharlubis do you want to push in a fix? I think just the variable needs to be renamed. I don't have time right now.

czgdp1807 · 2022-08-21T14:02:20Z

integration_tests/generics_list_01.py

+from ltypes import TypeVar, SupportsPlus, SupportsZero, Divisible
+from ltypes import f64, i32
+
+T = TypeVar('T', bound=SupportsPlus|SupportsZero|Divisible)


What is the meaning of SupportsPlus, SupportsZero and Divisible?

Can we write these using expressions as well? I will make the meaning of my question more clear once I know the answer to the above comment.

It says that the template T can do such operations. SupportsPlus means you can do T+T -> T. SupprtsZero means you can do T = 0. Divisible means you can do T / 5.

Okay. T + T -> T makes sense. Why do we want to specify T = 0 statement and T/5. Why can't we do T = TypeVar('T', bound=T + T -> T | T/T -> T)? How many more such specifications would be needed. Will we support SupportsMul, SupportsDivide, SupportsStringConcat or in general, Supports[Op] where [Op] can be replaced the operation the type T will support .

Yes, that is the plan to just list all the operations: #984, but that will come later. First we wanted an initial constraints operations done, which this PR delivers. Then we will refine it, figure out the syntax and all the details how to just specify operations (and it seems that yes, you need "T = 0" as well), etc.

This PR just needs to be good enough, no bad choices that would be hard to improve upon. But it does not implement the final design, just like previous merged PRs did not implement the final design either, it just moved us towards the final design.

@czgdp1807 It's still the prototype for the restriction, I got it working with some hard-coded restrictions, but I believe that we will change the design later so that restriction can be freely defined by users.

@czgdp1807 is this ok to merge?

(We still need to polish the history.)

Okay. Makes sense. Feel free to merge.

ansharlubis · 2022-08-22T02:40:36Z

@certik I just saw the problem, I'll handle it today.

certik · 2022-08-22T06:47:06Z

@ansharlubis would you mind polishing the git history (feel free to send a new PR). I'll do a final review and we can merge.

certik · 2022-08-23T07:51:38Z

I restarted the macOS test, which failed for an unrelated reason.

ansharlubis added 8 commits August 6, 2022 02:23

Adding restriction to the frontend

6532b4c

Modify generic test and ltypes to support restriction

57823e1

Added minimal restriction check on binary addition

ecde93b

Added Any restriction

776ee8b

Simplify getting name from type parameter

da4e820

Fixes restriction checking

5940f5e

Update reference tests

5d37517

Modified frontend

74dea01

ansharlubis added 20 commits August 8, 2022 08:57

Added type error tests

6c72681

Initial work on restriction check

f3db4d1

Modified restrictions into list

8fcada1

Added new integration tests

625b877

Update reference tests

86e40fe

Refactoring

3444989

Support type substitution for other types

73688ba

Refactoring

2711ff1

Allowed multiple traits

0336657

Update tests

632db53

Function renaming

e8ec98a

Refactoring

457f6ed

Update integration tests

d7a6236

Simplified implementation

6770c79

Refactoring and updating tests

86d54a8

Refactoring

a447a1a

Resolve conflicts and merge with main

dd60316

Added iterable trait

b001c5e

Added generics mean, does not compile to LLVM yet

f50970b

Update generic list test

b5b6c34

ansharlubis mentioned this pull request Aug 15, 2022

Fmean, mean overload functions implemented #877

Merged

ansharlubis added 4 commits August 21, 2022 14:41

Fixed errors

8e80642

Resolve conflicts

716880b

Update tests

8136801

Comment out binary operations we haven't considered yet

62922de

ansharlubis added 2 commits August 21, 2022 15:30

Comment out and remove currently unsupported operations

fc5221d

Simplified implementation

9ea842a

certik reviewed Aug 21, 2022

View reviewed changes

certik approved these changes Aug 21, 2022

View reviewed changes

certik requested a review from czgdp1807 August 21, 2022 13:16

Merge branch 'main' into add-restriction

bee6db7

czgdp1807 reviewed Aug 21, 2022

View reviewed changes

Fixed errors

ac06897

ansharlubis added 2 commits August 22, 2022 18:58

Merge branch 'master' into add-restriction

234bcea

Update tests

e62ebd4

ansharlubis closed this Aug 24, 2022

ansharlubis deleted the add-restriction branch August 24, 2022 07:59

Draft: Adding restriction to LPython generics #921

Draft: Adding restriction to LPython generics #921

Uh oh!

Conversation

ansharlubis commented Aug 5, 2022

Uh oh!

certik commented Aug 5, 2022

Uh oh!

certik commented Aug 21, 2022

Uh oh!

ansharlubis commented Aug 21, 2022 • 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!

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!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

certik commented Aug 21, 2022

Uh oh!

ansharlubis commented Aug 21, 2022

Uh oh!

certik commented Aug 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

certik commented Aug 21, 2022

Uh oh!

certik commented Aug 21, 2022

Uh oh!

certik commented Aug 21, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ansharlubis commented Aug 22, 2022

Uh oh!

certik commented Aug 22, 2022

Uh oh!

certik commented Aug 23, 2022

Uh oh!

Uh oh!

ansharlubis commented Aug 21, 2022 •

edited

Loading

certik commented Aug 21, 2022 •

edited

Loading