-
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 12 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 |
|---|---|---|
|
|
@@ -203,6 +203,7 @@ and pat' = function | |
| | S.OptP p -> I.OptP (pat p) | ||
| | S.AltP (p1, p2) -> I.AltP (pat p1, pat p2) | ||
| | S.AnnotP (p, _) -> pat' p.it | ||
| | 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 |
|---|---|---|
|
|
@@ -435,6 +435,7 @@ and declare_pat pat : val_env = | |
| | OptP pat1 -> declare_pat pat1 | ||
|
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. this could be folded into the lines 437,438 |
||
| | AltP (pat1, pat2) -> declare_pat pat1 | ||
|
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. Not my change, but this looks a bit weird,
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. It's probably ok because neither pat1 nor pat2 is allowed to bind any variables... In fact, it could probably just return () without declaring pat1 either (although you'd need to double check that's the case….)
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. Both patterns concurrently bind the same set of variables (currently empty), so it would be wrong to declare them twice. |
||
| | AnnotP (pat1, _typ) -> declare_pat pat1 | ||
| | ParP pat1 -> declare_pat pat1 | ||
|
|
||
| and declare_pats pats ve : val_env = | ||
| match pats with | ||
|
|
@@ -464,6 +465,7 @@ and define_pat env pat v = | |
| | _ -> assert false | ||
| ) | ||
| | AnnotP (pat1, _typ) -> define_pat env pat1 v | ||
| | ParP pat -> define_pat env pat v | ||
|
|
||
| and define_pats env pats vs = | ||
| List.iter2 (define_pat env) pats vs | ||
|
|
@@ -509,8 +511,9 @@ and match_pat pat v : val_env option = | |
| | None -> match_pat pat2 v | ||
| | some -> some | ||
| ) | ||
| | AnnotP (pat1, _typ) -> | ||
| match_pat pat1 v | ||
| | AnnotP (pat1, _) | ||
| | ParP pat1 -> | ||
| match_pat pat1 v | ||
|
|
||
| and match_pats pats vs ve : val_env option = | ||
| match pats, vs with | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -475,18 +475,34 @@ exp_field : | |
|
|
||
| (* Patterns *) | ||
|
|
||
| pat_nullary : | ||
| (* pat_top is needed because top-level parentheses around tuples (unit, 1-ary, pair, etc.) | ||
| are significant in function arguments, signifying a unary function which matches on a | ||
| first-class (i.e. consed) tuple *) | ||
| pat_top : | ||
|
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. Explain why we do this
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. Done.
ggreif marked this conversation as resolved.
Outdated
|
||
| | LPAR p=pat RPAR | ||
| { match p.it with | ||
| | TupP _ -> { p with it = ParP p } | ||
| | _ -> p | ||
| } | ||
| | p=pat_simple | ||
| { p } | ||
|
|
||
| pat_simple : | ||
| | UNDERSCORE | ||
| { WildP @! at $sloc } | ||
| | x=id | ||
| { VarP(x) @! at $sloc } | ||
| | l=lit | ||
| { LitP(ref l) @! at $sloc } | ||
| | LPAR p=pat RPAR | ||
| { p } | ||
| | LPAR ps=seplist1(pat_bin, COMMA) RPAR | ||
| { TupP(ps) @! at $sloc } | ||
|
|
||
| pat_nullary : | ||
| | LPAR p=pat RPAR | ||
| { p } | ||
| | p=pat_simple | ||
| { p } | ||
|
|
||
| pat_un : | ||
| | p=pat_nullary | ||
| { p } | ||
|
|
@@ -535,7 +551,7 @@ dec_nonvar : | |
| { (fd s (xf "func" $sloc)).it @? at $sloc } | ||
| | TYPE x=con_id tps=typ_params_opt EQ t=typ | ||
| { TypD(x, tps, t) @? at $sloc } | ||
| | s=obj_sort_opt CLASS xf=id_opt tps=typ_params_opt p=pat_nullary xefs=class_body | ||
| | s=obj_sort_opt CLASS xf=id_opt tps=typ_params_opt p=pat_top xefs=class_body | ||
| { let x, efs = xefs in | ||
| let efs' = | ||
| if s.it = Type.Object Type.Local | ||
|
|
@@ -572,7 +588,7 @@ dec : | |
| LetD(p, ObjE(s, x, efs') @? r) @? r } | ||
|
|
||
| func_dec : | ||
| | tps=typ_params_opt p=pat_nullary rt=return_typ? fb=func_body | ||
| | tps=typ_params_opt p=pat_top rt=return_typ? fb=func_body | ||
| { let t = Lib.Option.get rt (TupT([]) @! no_region) in | ||
| (* This is a hack to support local func declarations that return a computed async. | ||
| These should be defined using RHS syntax EQ e to avoid the implicit AsyncE introduction | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,63 @@ | ||
| 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(()); | ||
| gh[0](()); | ||
| gh[1](()); | ||
|
ggreif marked this conversation as resolved.
|
||
|
|
||
| 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]; | ||
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