Polymorphic equality #1267
Polymorphic equality #1267
Conversation
This reverts commit e8a22ac. quit change DH.re
|
@GuoDCZ let's better handle functions:
|
|
@cyrus- just to make sure I got the idea. When we refer to static error, does it mean something like: |
|
Yes. I'm not sure you need a new synthesis mode, but a new error definitely makes sense. |
…into polymorphic-equal-test merge to push
|
I think equality with holes should probably not evaluate - e.g. |
|
Remaining type inconsistency checker issue. Can be solved by fixing #1370 |
…into polymorphic-equal-test
|
@GuoDCZ is this ready to review? if so go ahead and click that button |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1267 +/- ##
==========================================
- Coverage 53.25% 52.37% -0.88%
==========================================
Files 103 103
Lines 10583 10814 +231
==========================================
+ Hits 5636 5664 +28
- Misses 4947 5150 +203
🚀 New features to boost your workflow:
|
|
@GuoDCZ can you add some unit tests for this functionality |
There was a problem hiding this comment.
Playing around I found a statics bug:
(5 == 3.0): doesn't have an error, when I think it should
Likewise this message is slightly strange
Also to echo Cyrus' comment, iirc you put a lot of effort in to dealing with edge cases (playing around with it now it does feel pretty robust to weird sum constructor/cast situations) it would be helpful if you could add some tests to stop anyone else from later breaking those edge cases.
src/haz3lcore/dynamics/DHExp.re
Outdated
| | Match(_) | ||
| | Dot(_) | ||
| | Cast(_) => false | ||
| | Fun(_) |
There was a problem hiding this comment.
can you add a comment explaining why fun is comparable? - or if I'm understanding it right, maybe this function could be named something like is_value?
| @@ -529,6 +529,44 @@ module Transition = (EV: EV_MODE) => { | |||
| kind: BinBoolOp(Or), | |||
| is_value: false, | |||
| }); | |||
| | BinOp(Int((Equals | NotEquals) as op), d1, d2) => | |||
There was a problem hiding this comment.
I wonder whether we should rename BinOp(Int(Equals)) to BinOp(Equals) or BinOp(Poly(Equals))
src/haz3lcore/dynamics/Transition.re
Outdated
| let expr: t = | ||
| if (!DHExp.ty_consistent(d1', d2')) { | ||
| DynamicErrorHole( | ||
| BinOp(Int(op), d1', d2') |> rewrap, |
There was a problem hiding this comment.
is there a reason we need to put fully-cast-stripped expressions here? It might be nice if we could keep the inner casts on output.
|
I am trying to add a test case. But I get: where an error of comparing arrow is expected. I am sure this is a static error tested from frontend. Having no idea why It does not appear in testing. test_case("Polymorphic equal compare arrow 2", `Quick, () => {
annotated_tree_test(
{|let a : +A+B(Int->Int) = in (a == a)|},
FIError.(
Exp.(
bin_op(
Int(Equals),
empty_hole(),
empty_hole(),
~ann=
Some(
FTemp.Typ.(
Exp(
Common(
Inconsistent(
CompareArrow(
sum([
Variant("A", [], None),
Variant("B", [], Some(arrow(int(), int()))),
]),
),
),
),
)
),
),
)
)
),
)
}), |
#1252.
Enable polymorphic equal / not equal comparison for:
Int,Bool,Float,String: Compare valueTuple&List: Pairwise comparison. ForList, also check internal type consistencySum: Same as OcamlWe disallow comparison of inconsistent types and types contain arrow:
Inconsistentwhen inconsistent types of both side is detected.CompareArrowwhen either a primitive is an arrow or a compound type contains an arrowSumtype raiseCompareArrowif any entry contains an arrow argument.Front-end quick tests: