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.

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

Types are pushed into operators incorrectly #72

Closed
UlfNorell opened this issue Nov 5, 2018 · 6 comments
Closed

Types are pushed into operators incorrectly #72

UlfNorell opened this issue Nov 5, 2018 · 6 comments
Labels
Milestone

Comments

@UlfNorell
Copy link
Collaborator

Currently the rules for pushing types into arithmetic operator applications in type_check_arith_op_in says (the other operator rules are similar):

  subtype(R, number)
  check(A, R)
  check(B, R)
  ------------------
  check(A op B, R)

This subtype constraint goes the wrong way, as shown in this example:

-spec f(integer(), integer()) -> integer() | divide_by_zero.
f(_X, 0) -> divide_by_zero;
f(X, Y)  -> X div Y.
%% The operator 'div' on line 8 is given a non-integer argument of type integer() | divide_by_zero

The error message is a bit strange as well, since it's the subtype(R, number) constraint that fails and not anything to do with the arguments.

The reason for having the subtyping this way around (I suspect) is to make this fail:

-spec g(number(), number()) -> integer().
g(X, Y) -> X + Y.
%% The variable 'X' on line 12 has type integer() | float() but is expected to have type integer()

I believe the correct rule is

  S = infer(A)
  T = infer(B)
  U = compat_arith_type(S, T)
  subtype(U, R)
  --------------------
  check(A op B, R)
@UlfNorell
Copy link
Collaborator Author

A problem is that inferring the type using type_check_expr is likely to return any() for non-trivial arguments, which will make the whole thing vacuously true.

@zuiderkwast
Copy link
Collaborator

More type inference can be done using type_check_expr(Env#env{infer = true}, Expr). Do you see any problem in doing inference in this case with respect to the graduality property AKA gradual guarrantee from the papers?

@josefs
Copy link
Owner

josefs commented Nov 5, 2018

Thanks for the thorough analysis. I suspect that there are a few similar problems lurking in the code.

The solution might be to compute the greatest lower bound of the result type and number() (in the case of addition). Something like this:

L = glb(number, R)
check(A, L)
check(B, L)
------------------
check(A + B, R)

Note that the glb-operator would be a partial operation and if it doesn't find a lower bound it would result in a type error.

Your thoughts on this solution, @UlfNorell?

EDIT: The glb-operator would technically not be partial, as it can always return none(); the bottom of the subtyping hierarchy. But if glb returns none() we should report a type error because it better matches programmer expectation.

@UlfNorell
Copy link
Collaborator Author

I think this makes sense, although you can construct examples where one could argue that the "right" thing to do is not failing on none():

-spec much_type_very_code(none()) -> none().
much_type_very_code(X) -> X + X.

@josefs
Copy link
Owner

josefs commented Nov 6, 2018

Yes, there would have to be a special case for when the result is expected to be none().

@josefs josefs added the bug label Nov 6, 2018
@josefs josefs added this to the Beta release milestone Nov 6, 2018
UlfNorell added a commit to UlfNorell/Gradualizer that referenced this issue Nov 12, 2018
First step for josefs#72. Currently handles numeric types and not
much else.
UlfNorell added a commit to UlfNorell/Gradualizer that referenced this issue Nov 12, 2018
UlfNorell added a commit to UlfNorell/Gradualizer that referenced this issue Nov 12, 2018
See josefs#72

Changes the typing rules for 'andalso' and 'orelse' to:

```
A : boolean()
B : Ty
---
A andalso B : boolean() | Ty
```

Whether this is the right choice can be discussed.
UlfNorell added a commit to UlfNorell/Gradualizer that referenced this issue Nov 12, 2018
@UlfNorell UlfNorell mentioned this issue Nov 12, 2018
7 tasks
UlfNorell added a commit to UlfNorell/Gradualizer that referenced this issue Nov 12, 2018
UlfNorell added a commit to UlfNorell/Gradualizer that referenced this issue Nov 18, 2018
First step for josefs#72. Currently handles numeric types and not
much else.
UlfNorell added a commit to UlfNorell/Gradualizer that referenced this issue Nov 18, 2018
UlfNorell added a commit to UlfNorell/Gradualizer that referenced this issue Nov 18, 2018
See josefs#72

Changes the typing rules for 'andalso' and 'orelse' to:

```
A : boolean()
B : Ty
---
A andalso B : boolean() | Ty
```

Whether this is the right choice can be discussed.
UlfNorell added a commit to UlfNorell/Gradualizer that referenced this issue Nov 18, 2018
UlfNorell added a commit to UlfNorell/Gradualizer that referenced this issue Nov 18, 2018
zuiderkwast pushed a commit that referenced this issue Nov 18, 2018
First step for #72. Currently handles numeric types and not
much else.
zuiderkwast pushed a commit that referenced this issue Nov 18, 2018
See #72

Changes the typing rules for 'andalso' and 'orelse' to:

```
A : boolean()
B : Ty
---
A andalso B : boolean() | Ty
```

Whether this is the right choice can be discussed.
zuiderkwast pushed a commit that referenced this issue Nov 18, 2018
@UlfNorell
Copy link
Collaborator Author

Forgot to tag any of the commits with a "fix" message.

berbiche pushed a commit to berbiche/Gradualizer that referenced this issue Feb 9, 2021
First step for josefs#72. Currently handles numeric types and not
much else.
berbiche pushed a commit to berbiche/Gradualizer that referenced this issue Feb 9, 2021
berbiche pushed a commit to berbiche/Gradualizer that referenced this issue Feb 9, 2021
See josefs#72

Changes the typing rules for 'andalso' and 'orelse' to:

```
A : boolean()
B : Ty
---
A andalso B : boolean() | Ty
```

Whether this is the right choice can be discussed.
berbiche pushed a commit to berbiche/Gradualizer that referenced this issue Feb 9, 2021
berbiche pushed a commit to berbiche/Gradualizer that referenced this issue Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants