GNOME Bugzilla – Bug 746704
Improve support for anonymous functions in Genie
Last modified: 2015-11-26 15:19:33 UTC
Created attachment 300223 [details] patch to support lambda's Currently Genie only support the deprecated lambda syntax for signals (+=). This patch provides lambda support in most constructs, the only requirement is that braces and parens need to be indent-balanced on multiple line constructs. for example: delegate Bla(a:string, b:int):string delegate PlusOne(number:int):int delegate Cb():void struct Bar id:uint name:string cb:Callback class Foo : Object event bla(what:string) construct() bar:Callback = def() print "bar" actions : array of Bar = { Bar() { id=10, name="foo", cb=def() if true print "foo" }, Bar() { id=20, name="bar", cb=bar } } for action in actions action.cb() bla.connect(def(t) if true print "ok" ) cb:Bla = def(a,b) return @"a=$a, b=$b" print cb("foo", 10) bla += def() print "ok, but deprecated" bla.connect(bla_cb) bla.connect(def(t,what) if what == "hello" print what else print "bye" ) callbacks:array of Callback = { def() print "cb1" , def() print "cb2" , def() print "cb3" } for c in callbacks c() cb2:Cb = def() ints:array of int = { 1,2,3 } for i in ints print @"i=$i" cb2() var g = generate() g(); print "constructed" def bla_cb(what:string) print @"method callback $what" def generate():Callback return def() print "generated" def blabla(a:int, b:int, c:int, calc:PlusOne, d:int):void print @"a=$a b=$b c=$c calc="+calc(c).to_string() init var f = new Foo() f.bla("hello") f.blabla( 10, 20, 30, def(what) return what + 1 ,40)
Thanks for sharing your patch. Your examples look pretty good. A few minor points: 1. For clarification to anyone else reading this, "Currently Genie only support the deprecated lambda syntax for signals (+=)." means the += syntax for Genie events (signals in Vala and GLib) is deprecated in favour of the connect() method. The "lambda syntax" isn't deprecated. 2. Personally I prefer to call this syntax "anonymous functions" because they follow the Genie syntax for creating a function, just without an identifier. Vala has the () => { print( "do something" ); } syntax for lambda expressions that could also be included in Genie at some point. 3. Can you resubmit the patch with the following changed:
- your full name, I think this is the convention - change the mime-type from application/mbox to text/plain so a review can be made, e.g. see https://bugzilla.gnome.org/show_bug.cgi?id=620133 I'm hoping using the review process is easier to signal to someone with commit access that the patch is good. - If you agree "anonymous function" is a better phrase then re-title the patch subject from "[PATCH] Genie: include elaborate lambda support." to something like "[PATCH] Genie: improve support for anonymous functions." I believe this will appear in the Vala commit log if your patch is accepted. 4. Your patch brings significant enhancement to the use of Genie. Once I have fully tested it then I think it should probably be accepted as a good step forward. My only doubt is the addition of skip_whitespace() as a solution. There are a number of whitespace problems in Genie, e.g. https://bugzilla.gnome.org/show_bug.cgi?id=611085 and being unable to define an empty namespace when starting out on a project, that a more comprehensive solution may be better in the long term for identifying blocks of code based on the context they appear. That is probably a lot of work though. All the best, Alistair
Created attachment 300306 [details] [review] Genie: improve support for anonymous functions. Updates following remarks of Al Thomas
Hi Al, Thanks for your quick response, I've updated the patch following your requirements. About the skip_whitespace(), currently the scanner is doing this within braces and parens and thus breaking block flow. I've moved the skipping to the parser. the "skip_whitespace()" method is like the other skip_* methods in there. About the bug https://bugzilla.gnome.org/show_bug.cgi?id=611085 I'm experiencing this problem as well, it's on my list to fix next. Thanks again. Best regards, Niek
Created attachment 302758 [details] Test case to check backwards compatibility with multiline support between parentheses
Hi Niek, Is there a part missing to your patch? Your patch removes line continuations between parens and braces from the Genie scanner and presumably moves it to the parser. I have attached a set of tests to check backwards compatibility for this between parens, but I am getting syntax errors. For example "syntax error, expected declaration but got `assert' with previous `dedent'" Also your examples are producing a syntax error "syntax error, expected identifier". If you apply this your patch against a clean copy of Vala master you should see the same. Maybe another commit should have been squashed in to this one? Al
Hi Al, Thanks for the tests. I can compile the examples I wrote without compile errors after applying the patch on the current master. Perhaps there is an indentation issue when copy-pasting from bugzilla? About the tests you wrote, it's true there is a small backwards incompatibility here. To make the anonymous function syntax work the only requirement is that braces and parens need to be indent-balanced on multiple line constructs. Without this things can get ambiguous quickly, f.e.: var foo = bar(def(a,b) b(def(a) return def(c) return c+1)) needs to become: var foo = bar(def(a,b) b(def(a) return def(c) return c+1 ) ) All the Genie code I was able to find did use either single line method calls or this indentation style. It's very hard to solve unbalanced parens without modifying the scanner.
Created attachment 304964 [details] Test anonymous functions when declared as a variable
Created attachment 304965 [details] Test anonymous functions when returned by a function
(In reply to nieka from comment #7) > Thanks for the tests. I can compile the examples I wrote without compile > errors after applying the patch on the current master. Perhaps there is an > indentation issue when copy-pasting from bugzilla? Ah yes, an [indent=4] is needed at the start after copying your examples from bugzilla. They now all work for me with your patch. Thanks! From what I can tell your patch identifies two problems with the current Genie implementation of anonymous functions: 1. there is a conflict between an expression that expects a terminator (semi-colon or end of line) and the parsing of dedent to mark the end of an anonymous function's block of code 2. the current handling of multi-line parentheses and braces strips all white space and that causes a problem because anonymous functions are a block of code with normal Genie indentation The current solution to the first problem is to check current_expr_is_lambda and if it is then don't call expect_terminator(). The current parser does this in the parse_expression_statement function. This is needed to handle the deprecated += syntax for events(Vala signals). The patch adds the current_expr_is_lambda check in three more places: - parse_return_statement, this is needed to allow a function to return an anonymous function. Test cases are attached for this. It also appears closures work. See test5 and test6 of the test cases. As an aside, after looking at https://bugzilla.gnome.org/show_bug.cgi?id=611460 it appears the parser has code to allow an anonymous function without parentheses where the identifier becomes the first parameter. This could probably be removed. - parse_local_variable_declarations, at the beginning where type inference is handled with the var keyword. Functionally this is not necessary as Vala does not currently infer a type for an anonymous function, but returns the message "error: lambda expression not allowed in this context". This is, however, a more useful error message so it is nice to have the check added. Also https://bugzilla.gnome.org/show_bug.cgi?id=750698 moves this section of code into a new function to improve readability. A review of that patch would be welcome. - parse_local_variable_declarations, at the end. The check here enables the test cases for anonymous functions declared as a variable to work. Ideally the check would be in one place in the code. For example an extension of parse_expression function, maybe called parse_expression_with_terminator. This would check the expression returned was not of the type LambdaExpression and call expect_terminator. parse_expression_with_terminator would be called directly by parse_return_statement. parse_expression_statement could also be made to call parse_expression_with_terminator directly. With parse_local_variable_declarations a parameter with default value of false could be added to parse_local_variable, so when called with true parse_expression_with_terminator would be called. In summary the patch resolves the first problem, but would ideally do so with the removal of current_expr_is_lambda. The second problem of declaring anonymous functions as parameters and within structs and arrays is almost there: > About the tests you wrote, it's true there is a small backwards > incompatibility here. To make the anonymous function syntax work the only > requirement is that braces and parens need to be indent-balanced on multiple > line constructs. > Without this things can get ambiguous quickly, f.e.: > var foo = bar(def(a,b) > b(def(a) > return def(c) > return c+1)) > > needs to become: > > var foo = bar(def(a,b) > b(def(a) > return def(c) > return c+1 > ) > ) > > All the Genie code I was able to find did use either single line method > calls or this indentation style. It's very hard to solve unbalanced parens > without modifying the scanner. I think there are two things here. Firstly an anonymous function is defined as a block using Genie indentation. The end of the block is marked with a dedent, which is fine. So in the examples just quoted I understand the closing parentheses have to be on the next line with a dedent. The second thing is the patch requires all closing parentheses or braces to be on the same indentation level as the opening parentheses or braces. For example: const a:array of string = { "a", "b", "c", "d", "e" } init pass will compile, but: const a:array of string = { "a", "b", "c", "d", "e" } init pass will not compile. This affects all multiline function parameters, object instantiations, array assignments and struct assignments. This doesn't feel like an acceptable solution. Obviously this patch is very close and a lot of time has been spent on it in the analysis, development and testing stages. I propose splitting the patch in two. The first should deal with the terminator problem. Getting this in to the mainline parser would atleast extend some useful functionality. A second patch to parse anonymous functions as a value in a parameter, array or struct would probably be best submitted as a separate bug as I think it will require more work. Thanks for your work so far on this, Alistair
Created attachment 305910 [details] [review] improve support for anonymous functions New version of the patch that also modifies the geniescanner to maintain indentation state on opening parens, brackets and braces.
Hi Al, I've updated the patch to deal with the unbalanced parens, brackets and braces issue. What I've done is modify the scanner to keep state of current indentation when opening, then return to this indentation when closing. This seems to fix all affected cases, f.e.: const a:array of string = { "a", "b", "c", "d", "e" } init pass Best regards, Niek
Created attachment 306095 [details] Tests to check listeners added to events with the deprecated += syntax
Created attachment 306096 [details] [review] Enables support for anonymous functions as function return values and assignment to variables This also incorporates the code clean up from https://bugzilla.gnome.org/show_bug.cgi?id=750698 Passes all tests in this bug and https://bugzilla.gnome.org/show_bug.cgi?id=750698
Created attachment 306097 [details] Test multiline support with object constructors
Review of attachment 305910 [details] [review]: Thanks for your continued work on this. Unfortunately a quick test of your patch on a project shows it fails to compile object constructors that have parameters over multiple lines - see new tests attached. As a refactoring of current code the patch should also remove open_parens_count and open_brace_count from the scanner because these are now unused after applying the patch. There are also some minor details to the patch that also need amending. The patch will not apply against the mainline repository, fortunately this can be overcome with --ignore-whitespace. The patch also is missing author details and commit message. I have attached an alternative patch that adds support for anonymous functions as return values and in variable declarations, but does not deal with the parens and braces issue. This patch also moves the current_expr_is_lambda test to one place to make long term maintenance of Genie hopefully easier. I was unable to remove current_expr_is_lambda completely because the += syntax failed when I did. Can you rename this bug to something like "Support anonymous functions as return values and variable declarations", test my patch and if it is OK review the patch as accepted-commit_now. I would prefer for you to have had the credit so I am more than happy for you to amend the patch. Then can you start a new bug titled something like "Support anonymous functions as function arguments and in arrays and structs" to focus on the parens and braces issue. I like your use of state in the scanner and had started to explore that myself, but I was looking to use it as a test to replace the test for open_parens_count or open_braces_count so the whitespace is not stripped when inside an anonymous function. Thanks again, Al
Created attachment 306101 [details] Test multiline support with object constructors
Hi Al, Very sorry for the late reply, I wasn't able to work much on this the past month. I tried your patch and it's very good! I found one small issue: Code like: init a, b, c: int a = 1 b = 2 c = 3 Seems to give a compilation error now. Also I've done some more work on the state in the scanner, and I got most of it working, but still fixing some edge cases when there are pending dedents. More on that later. Thanks and best regards, Niek
Created attachment 309754 [details] Test variable declarations Includes test for multiple declarations before assignment, e.g.: a, b, c:int a = 1 b = 2 c = 3
Created attachment 309756 [details] [review] Enables support for anonymous functions as function return values and assignment to variables Thanks for the review Niek. This patch should fix the problem with multiple variable declarations.
Created attachment 309757 [details] Test variable declarations Includes test for multiple declarations before assignment, e.g.: a, b, c:int a = 1 b = 2 c = 3 (Previous version did not include assert for last test)
Review of attachment 309756 [details] [review]: passes all tests and compiles libsoy.
Thanks I will have a look at this in next few days...
Review of attachment 309756 [details] [review]: This patch seems to work OK. Im happy for it to be committed as is. Thanks for your work on this
commit 9044d84f3b3bde17782723dab23eea8f1eb53fe4 Author: nieka@dsv.nl <nieka@dsv.nl> Date: Wed Nov 25 11:23:03 2015 +0100 genie: anonymous functions as function return values and assignment to variables Fixes bug 746704 This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.