-
Notifications
You must be signed in to change notification settings - Fork 127
AST-38: Significant parenthesis around tuple patterns in func arguments #178
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
Changes from 32 commits
08e32c3
921c06a
7fcb0a5
67135be
3d3ee75
4fe6e09
2d8eac1
1383827
3f2e405
da9c08b
0d797e6
53cb2ad
d31dcac
b5b9925
71ccf02
de8f3a2
6f4ee64
7ca3979
0d4844b
7daa160
82f4f42
922f757
88d6270
e646c9c
f02396b
02fd5ee
10547cb
f9ccad9
7cbdb57
9c29a48
2688aeb
0b42e5e
92ea6b6
c67a012
1052fbb
f93ecdc
bffcea3
e4e7268
c392ade
81200bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,13 +173,20 @@ and decs ds = | |
| | _ -> (phrase' dec' d) :: (decs ds) | ||
|
|
||
| and dec d = phrase' dec' d | ||
| and dec' at n d = match d with | ||
| and dec' at n d = | ||
| let fix_unary p = match p.it, p.note with | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, it looks fishy to me to only apply such a special case in one place. If the solution is to essentially remove unary tuples then the right place to do this normalisation is in the parser, in the productions for tuples, so that the AST never contains a unary tuple type/exp/pat in the first place.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I wrote this, my intention was not to equate values with the unary tuple formed from them. I was bending to the (empirical) observation that the later stages (i.e. coverage analyser, interpreter) seem to insist that single arguments should not be wrapped by an extra layer of |
||
| | S.TupP [p1], Type.Tup [n] -> { p with it = p1.it; note = n } | ||
| | _ -> p in | ||
| let param p = match p.it with | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: avoid nesting functions unless they actually close over something.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, this seems to be a stylistic issue. Are there any such conventions written down somewhere? My notion used to be "define utilities in the closest possible scope", which I do here. I'll try to lift this out to top-level when folding the two functions into one. I wonder however, if that will export the function from the module? |
||
| | S.ParP p1 -> pat (fix_unary { p with it = S.TupP [p1]; note = Type.Tup [p.note] }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed with @crusso, not sure why there are two levels of special casing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will fold. There is nothing subtle going on besides "separation of concerns". See my previous explanation. |
||
| | _ -> pat (fix_unary p) | ||
| in match d with | ||
| | S.ExpD e -> I.ExpD (exp e) | ||
| | S.LetD (p, e) -> I.LetD (pat p, exp e) | ||
| | S.VarD (i, e) -> I.VarD (i, exp e) | ||
| | S.FuncD (s, i, tbs, p, ty, e) -> | ||
| let cc = Value.call_conv_of_typ n.S.note_typ in | ||
| I.FuncD (cc, i, typ_binds tbs, pat p, ty.note, exp e) | ||
| I.FuncD (cc, i, typ_binds tbs, param p, ty.note, exp e) | ||
| | S.TypD (id, typ_bind, t) -> | ||
| let c = Lib.Option.value id.note in | ||
| I.TypD c | ||
|
|
@@ -199,7 +206,7 @@ and dec' at n d = match d with | |
| T.promote (T.open_ inst rng) | ||
| | _ -> assert false | ||
| in | ||
| I.FuncD (cc, id', typ_binds tbs, pat p, obj_typ, (* TBR *) | ||
| I.FuncD (cc, id', typ_binds tbs, param p, obj_typ, (* TBR *) | ||
| { it = obj at s (Some id') self_id es obj_typ; | ||
| at = at; | ||
| note = { S.note_typ = obj_typ; S.note_eff = T.Triv } }) | ||
|
|
@@ -222,7 +229,8 @@ and pat' = function | |
| | S.TupP ps -> I.TupP (pats ps) | ||
| | S.OptP p -> I.OptP (pat p) | ||
| | S.AltP (p1, p2) -> I.AltP (pat p1, pat p2) | ||
| | S.AnnotP (p, _) -> pat' p.it | ||
| | S.AnnotP (p, _) | ||
| | S.ParP p -> pat' p.it | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rossberg @crusso @nomeata I think this desugaring is buggy. func p(()) {};
func q((),) {};should have identical desugarings, because they both have arity 1 and both match unit. However currently they desugar thus: (FuncD ( 1 -> 0) p (TupP) () (BlockE ()))
(FuncD ( 1 -> 0) q (TupP (TupP)) () (BlockE ()))which means different calling conventions. So this is a no-go. I fixed this here as | S.AnnotP (p, _) -> pat' p.it
| S.ParP p -> pat' (S.TupP [p])and now I get the correct (FuncD ( 1 -> 0) p (TupP (TupP)) () (BlockE ()))
(FuncD ( 1 -> 0) q (TupP (TupP)) () (BlockE ()))I only started looking at the desugarings late yesterday, so this slipped my attention. Then of course,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be fixed in multiple ways, I think. I think the simplest is to only apply your fix above for toplevel parameter patterns, i.e., tweak desugaring such that you invoke an auxiliary: and invoke that in place of directly calling pat for func and class (replacing your fix above).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this is kind of why I was wondering if we could just have a separate production for argument patterns that retains ParP and leaves the existing rules unchanged. The ParP could only occur outermost in argument patterns.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but since this arguably is an issue of typing not parsing, handling it in the parser seems less appropriate to me.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed why don't we define ast node: arg_pat = ParA pat | PatA pat and leave the exisiting pat type ParP free. with arg_pat used for class and function arguments.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rossberg That's what I would have expected in a language with multi-arity functions. (ExpD (CallE ( 1 -> 0) (VarE g) (TupE)))
(ExpD (CallE ( 1 -> 0) (VarE h) (TupE)))where (FuncD ( 1 -> 0) g (TupP (TupP)) () (BlockE ()))
(FuncD ( 1 -> 0) h (VarP u) () (BlockE ()))and the IR-interpreter chokes on the call to At one point @nomeata suggested to consider the function's arity. When 1, we could adapt the convention that there is no wrapping of the argument in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gabor What is the source for g and h? I'm hoping there will be a type error with my recent fix.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @crusso from func g(()) {};
func h(u:()) {};
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This would simplify the backend (it currently matches tuple patterns directly, but has to recreate the tuple if there is a different kind of pattern. Annoying logic to maintain).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This make sense to me, but as a separate PR |
||
|
|
||
| and prog p = (decs p.it, | ||
| { I.has_await = true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ let word_unop fword8 fword16 fword32 fword64 = function | |
| | T.Word16 -> fun v -> Word16 (fword16 (as_word16 v)) | ||
| | T.Word32 -> fun v -> Word32 (fword32 (as_word32 v)) | ||
| | T.Word64 -> fun v -> Word64 (fword64 (as_word64 v)) | ||
| | _ -> raise (Invalid_argument "unop") | ||
| | _ -> raise (Invalid_argument "word_unop") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these unrelated changes that snuck into the branch? I wouldn't bother reverting, just wanted to draw your attention...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer reverting that change, since word_unop is an implementation detail of the module, and only unop is meaningful outside.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am happy to revert. I did this because there are two switch: [tc] [run] [run-low] [run-ir] [wasm]
--- switch.tc (expected)
+++ switch.tc (actual)
@@ -1,6 +1,4 @@
-switch.as:87.33-87.34: warning, this pattern is never matched
-switch.as:75.11-77.2: warning, the cases in this switch do not cover all possible values
-switch.as:81.3-81.14: warning, the cases in this switch do not cover all possible values
-switch.as:87.3-87.40: warning, the cases in this switch do not cover all possible values
-switch.as:92.11-94.2: warning, the cases in this switch do not cover all possible values
-switch.as:97.11-99.2: warning, the cases in this switch do not cover all possible values
+(unknown location): internal error, Invalid_argument("unop")
+
+Last environment:
+It is kind-of hard to figure which function threw, when two are throwing the same thing :-) Anyway, if we go forward (with a distinct PR) to equate unary tuples and values in the parser, then the Also my (not pushed) experiment to always add -coverage.as:5.8-5.14: warning, this pattern does not cover all possible values
+coverage.as:5.7-5.15: warning, this pattern does not cover all possible valuesI could fix that by inheriting the inner srcloc, but I don't see the benefit. |
||
|
|
||
| let num_unop fint fword8 fword16 fword32 fword64 ffloat = function | ||
| | T.Int -> fun v -> Int (fint (as_int v)) | ||
|
|
@@ -37,14 +37,14 @@ let unop t op = | |
|
|
||
| let text_binop ftext = function | ||
| | T.Text -> fun v1 v2 -> Text (ftext (as_text v1) (as_text v2)) | ||
| | _ -> raise (Invalid_argument "binop") | ||
| | _ -> raise (Invalid_argument "text_binop") | ||
|
|
||
| let word_binop fword8 fword16 fword32 fword64 = function | ||
| | T.Word8 -> fun v1 v2 -> Word8 (fword8 (as_word8 v1) (as_word8 v2)) | ||
| | T.Word16 -> fun v1 v2 -> Word16 (fword16 (as_word16 v1) (as_word16 v2)) | ||
| | T.Word32 -> fun v1 v2 -> Word32 (fword32 (as_word32 v1) (as_word32 v2)) | ||
| | T.Word64 -> fun v1 v2 -> Word64 (fword64 (as_word64 v1) (as_word64 v2)) | ||
| | _ -> raise (Invalid_argument "binop") | ||
| | _ -> raise (Invalid_argument "word_binop") | ||
|
|
||
| let num_binop fnat fint fword8 fword16 fword32 fword64 ffloat = function | ||
| | T.Nat -> fun v1 v2 -> Int (fnat (as_int v1) (as_int v2)) | ||
|
|
@@ -82,7 +82,7 @@ let word_relop fword8 fword16 fword32 fword64 = function | |
| | T.Word16 -> fun v1 v2 -> Bool (fword16 (as_word16 v1) (as_word16 v2)) | ||
| | T.Word32 -> fun v1 v2 -> Bool (fword32 (as_word32 v1) (as_word32 v2)) | ||
| | T.Word64 -> fun v1 v2 -> Bool (fword64 (as_word64 v1) (as_word64 v2)) | ||
| | _ -> raise (Invalid_argument "relop") | ||
| | _ -> raise (Invalid_argument "word_relop") | ||
|
|
||
| let num_relop fnat fint fword8 fword16 fword32 fword64 ffloat = function | ||
| | T.Nat -> fun v1 v2 -> Bool (fnat (as_int v1) (as_int v2)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -481,7 +481,10 @@ pat_nullary : | |
| | l=lit | ||
| { LitP(ref l) @! at $sloc } | ||
| | LPAR p=pat RPAR | ||
| { p } | ||
| { match p.it with | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need the case distinction, just do
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I don't quite understand why you only preserve the ParP around TupP - unless you somewhere pattern match on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to get away with the distinction, but it caused the +(unknown location): internal error, Invalid_argument("unop")failure. I have analysed it and it originates from: let x2 : Int = switch (-3) {
case (0) 0;
case (-1) 2; // <<< problem
case (-3) 1;
case (x) x;
};so the (LetD
(VarP x2)
(AnnotE
(SwitchE
(UnE ??? NegOp (LitE (PreLit 3 Nat)))
(case (ParP (LitP (PreLit 0 Nat))) (LitE (PreLit 0 Nat)))
(case (ParP (SignP NegOp (PreLit 1 Nat))) (LitE (PreLit 2 Nat)))
(case (ParP (SignP NegOp (PreLit 3 Nat))) (LitE (PreLit 1 Nat)))
(case (ParP (VarP x)) (VarE x))
)
(VarT Int)
)
)Update: cracked it.
The other reason for not using |
||
| | TupP _ -> { p with it = ParP p; at = at $sloc } | ||
| | _ -> p | ||
| } | ||
| | LPAR ps=seplist1(pat_bin, COMMA) RPAR | ||
| { TupP(ps) @! at $sloc } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| func p((),) {}; | ||
| func p1(()) {}; | ||
| func p2((())) {}; | ||
| func p3(((()))) {}; | ||
|
|
||
| let ps : [((),) -> ()] = [p, p1, p2, p3]; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,87 @@ | ||
| func f() {}; | ||
| func g(()) {}; | ||
| func g1((())) {}; // only top-level parenthesis signifies arity | ||
| func h(u:()) {}; | ||
| func h1((u:())) {}; // dito, intermediate parentheses are redundant | ||
|
|
||
| let _ = f : () -> (); | ||
| let _ = g : () -> (); | ||
| let _ = g : (()) -> (); | ||
| let _ = h : (()) -> (); | ||
|
|
||
| let _ : [() -> ()] = [f]; | ||
| let gh : [(()) -> ()] = [g, g1, h, h1]; | ||
|
|
||
| f(); | ||
| g(); | ||
| h(); | ||
| g(); // accepted, because "At calls there is an implicit coercion | ||
| // between tuples and n-ary arguments" | ||
| h(); // dito | ||
| gh[0](); // dito | ||
| gh[1](); // dito | ||
|
|
||
| // Correct: | ||
| g(()); | ||
| h(()); | ||
| h1(()); | ||
| gh[0](()); | ||
| gh[1](()); | ||
|
ggreif marked this conversation as resolved.
|
||
| gh[2](()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice tests! Would it make sense to add a couple of negative ones in test/fail too, eg distinguishing unary functions on pairs from binary functions etc
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do a little follow-on for this, as I actually have a few stashed tests that fail lying around. |
||
| gh[3](()); | ||
|
|
||
| func j(a:Int) {}; | ||
| func k(a:(Int)) {}; | ||
| func l(a:((Int))) {}; | ||
|
|
||
| let jkl : [Int -> ()] = [j, k, l]; | ||
|
|
||
| j(5); | ||
| k(8); | ||
| l(13); | ||
| jkl[1](21); | ||
|
|
||
| func m((a:Int, b:Bool)) {}; | ||
| func m1((a:Int, b:Bool,)) {}; | ||
| let ms : [((Int, Bool)) -> ()] = [m, m1]; | ||
|
|
||
| func n(a:Int, b:Bool) {}; | ||
| func n1(a:Int, b:Bool,) {}; | ||
|
|
||
| let ns : [(Int, Bool) -> ()] = [n, n1]; | ||
|
|
||
| func o((a:Int,)) {}; | ||
| let os : [((Int,)) -> ()] = [o]; | ||
|
|
||
| // class tests, these have the same argument regime | ||
| class F() {}; | ||
| class G(()) {}; | ||
| class H(a:()) {}; | ||
| class G1((())) {}; | ||
| class H1((u:())) {}; | ||
| let _ : [() -> F] = [F]; | ||
| let _ : [(()) -> G] = [G]; | ||
| let _ : [(()) -> G1] = [G1]; | ||
| let _ : [(()) -> H] = [H]; | ||
| let _ : [(()) -> H1] = [H1]; | ||
|
|
||
| func annih<A,B>(f : A -> B) : (A -> ()) { func (a : A) { ignore (f a) } }; | ||
| let _ : [(()) -> ()] = [annih<(),G> G, annih<(),G1> G1, annih<(),H> H, annih<(),H1> H1]; | ||
|
|
||
|
|
||
| // test that parens are not significant deeper into the pattern, | ||
| // even around tuples | ||
|
|
||
| func p(a:Int, () ) {}; | ||
| func q(a:Int, (())) {}; | ||
| func r(a:Int, (b:Int,) ) {}; | ||
| func s(a:Int, ((b:Int,))) {}; | ||
| let _ : [(Int, ()) -> ()] = [p, q]; | ||
| let _ : [(Int, (Int,)) -> ()] = [r, s]; | ||
|
|
||
| // redundant parens for anonymous functions | ||
|
|
||
| let _ = (func (a:Nat, ()) : Nat { a }) (42, ()); | ||
| let _ = (func ((a:Nat, (()))) : Nat { a }) (42, ()); | ||
|
|
||
| // test that switch expressions also behave correctly with redundant parens | ||
|
|
||
| let _ = switch (42,) { case (3,) 3; case ((5,)) 5; case (a,) a}; | ||
| let _ = switch (42,) { case (3,) 3; case ((5,)) 5; case (((a,))) a}; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace only