-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat: Remove func
keyword
#3430
base: main
Are you sure you want to change the base?
Conversation
Removing dependence on a keyword for multi-line declaration is cool, but there was another motive for func keyword. I've re-added func keyword because there were cases where parsing was very close to being ambiguous. I was even thinking about making it required. A case of ambiguity with param defaults:
A case of ambiguity with returning a function:
The general problem (and potential performance sink) is that when the parser encounters a func call Because of precedence, it first has to assume that it is a func definition and only when it does not find a trailing |
Could we instead make it right-associative? e.g. with: let f = x y -> x z -> x + y + z If let f = x y -> (x z -> x + y + z) If it's left associative, it would be parsed as: let f = (x y -> x) z -> x + y + z ...which seems wrong. To take your examples: let my_func = a b c:x -> (x -> a + c b) # this applies with no parentheses too
vs.
let my_func = a b -> (c:x -> a + c b) and let my_func = a -> x -> a + x # this applies with no parentheses too
vs.
let my_func = a -> (x -> a + x) ? |
Oh, we could absolutely make it left or right associative (one of these is the current behavior), but my point is that parsing this is much harder and less performant than it needs to be. This applies to all function calls: when parsing I'm all for minimal grammar, but I do think having a (even with the keyword, we still need to decide on left/right-associativity) |
I was less keen on the I agree it slows down the parsing a bit. But the parsing is still pretty fast! And at least how I understand it, it's not ambiguous to humans? Though possibly that's because we so rarely see nested function definitions. I also really like the clean aesthetics of the current form, and only having one form of function definition. Though aesthetics aren't that good a reason, and the latter point would hold if we mandated |
Presumably a language such as Haskell is similar? i.e. |
I don't know haskell, but I don't think that's the case. Aren't all terms before |
Yes but same as with PRQL — until we get to the In particular |
I've looked into Haskell and it does not have this problem. That's because a = f x y ... For anonymous functions, Haskell does have a leading denomination: a = \x y -> x + y I know that this might seem like a detail, but I can have major performance implications. An extreme example would be:
Here, parser would first try to parse this as a function declaration - all of the parameters and all of their default values, only to encounter an end of the line, conclude that this is not a function declaration, throw this all away and start again, parsing as a function call. I wish we could reuse the parsed args, but they do a slightly different syntax (type annotations), so this would be hard to pull off. |
But |
In Haskell, I don't think so, no. (I have a very limited knowledge of Haskell, correct me if I'm wrong). |
@max-sixty This discussion so far has been highly technical, targeted at people who're deeply familiar with PRQL innards. As someone who might wind up writing/tweaking documentation for this, I would enjoy more explanation:
Update: A few sentences of context would be helpful. Thanks, as always. |
Sorry to belabor the point, but to ensure we're sharing understanding on this example. IIUC (also not an expert), this is valid Haskell: f :: Int -> Int -> Int
f a b = a + b -- Function definition ...but so is... x = 10
y = 20
f x y -- Function application (even though that final statement isn't doing anything, it's still valid IIUC) The point re before the Does that make sense? (If I'm correct it doesn't mean that we have remove |
It broadly has! But it's still valid, and allows multi-line function definitions (check out PR for examples). So now we have line wrapping, one proposal is to remove |
I'm super confused by this. The only change I made is to comment out the lexing of"func"
. But as a result, a huge number of tests fail; most queries can no longer be compiled. I don't see how that's possible — the AST that's being passed back fromprql-parser
surely doesn't change. (As expected, the only test inprql-parser
fails is one that containedfunc
, which I've commented out — which suggests that the AST likely doesn't change). Am I missing something very obvious? Is there dark magic in our codebase?OK it was because stdlib was using it!
What do folks think about removing
func
now that we have line continuations? Exactly one way to do things etc etc...