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

Named arguments #250

Merged
merged 14 commits into from May 19, 2015

Conversation

Projects
None yet
4 participants
@danielpetisme
Copy link
Contributor

danielpetisme commented Mar 3, 2015

First of all a bit of vocabulary, a parameter (or formal parameter) representents the identifier used during the function definition

function min = |a, b| -> ...

a and b are parameters
An argument (or actual parameter) represents the value used during the function invocation

min(1,2)

1 and 2 are arguments

So far, Golo runtime relied on the arguments order to find the target function/method to invoke. With named parameters, the developers do not have to care about the argument order, plus it permits to increase the verbosity of the function invocations.

function join = |delimiter, values...| ->

#change argument order + varargs support
join(values = array["Foo", "Bar"], delimiter=",")

Show me more!

@yloiseau

This comment has been minimized.

Copy link
Contributor

yloiseau commented Mar 4, 2015

👍

[source,golo]
----
function dummy = |who, when, what| {
return who + " " when + " " + what

This comment has been minimized.

@yloiseau

yloiseau Mar 4, 2015

Contributor

a + is missing between the " " and the when

This comment has been minimized.

@artpej

artpej Mar 4, 2015

Contributor

when is a reserved keywork and can't be a parameter name

@danielpetisme danielpetisme force-pushed the danielpetisme:wip/named-arguments branch 3 times, most recently from c3ab0b1 to 05a527d Mar 4, 2015

@danielpetisme

This comment has been minimized.

Copy link
Contributor

danielpetisme commented Mar 4, 2015

Doc fixed according to @yloiseau @artpej comments.
Thank you guys

@jponge

This comment has been minimized.

Copy link
Member

jponge commented Mar 4, 2015

This sounds awesome @danielpetisme 😃

I will dive into the pull-request soon and make a proper code review.

@danielpetisme

This comment has been minimized.

Copy link
Contributor

danielpetisme commented Mar 4, 2015

The aim is to have a first code review. Once this PR merged, I will have a look on optional parameters

@@ -48,7 +48,7 @@
static {
String bootstrapOwner = "fr/insalyon/citi/golo/runtime/FunctionCallSupport";
String bootstrapMethod = "bootstrap";
String description = "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;I)Ljava/lang/invoke/CallSite;";
String description = "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;";
FUNCTION_INVOCATION_HANDLE = new Handle(H_INVOKESTATIC, bootstrapOwner, bootstrapMethod, description);

This comment has been minimized.

@artpej

artpej Mar 4, 2015

Contributor

I think that BSM method doesn't allow parameters of type Object ( reference )

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

You can have trailing objects in a bootstrap method, and they can be of Object type. The only thing is that they must be compatible with constant pool entries, so primitives, classes, strings and field/method references.

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

So the I argument gets into the objects array, right?

This comment has been minimized.

@artpej

artpej Mar 4, 2015

Contributor

@jponge Ok! nice! could be usefull for my futur experiments 😉

@jponge

This comment has been minimized.

Copy link
Member

jponge commented Mar 4, 2015

@danielpetisme Haven't checked the code yet, but does the resolution checks parameters order for all call sites, or just the call sites where parameter names have been used?

@danielpetisme

This comment has been minimized.

Copy link
Contributor

danielpetisme commented Mar 4, 2015

During the compilation

Function definition
a Java @GoloFunction annotation is generated to decore the Java method (for ALL the golo functions, could be the entry point for more metadata)

function plop = |foo, bar| 

generates

@GoloFunction(parameters = {"foo", "bar"})
public static Object plop(Object, Object)

Function invocation
if the invocation is flagged as using name parameters, then the Arguments visitor build the list of the argument names used in the current invocation. These arguments are passd to the indy BSM.

Runtime

The Function Call Support looks for a method which match the actual arguments constraints (number + order + type). The magic part is that Golo function only use Object has type, so that's why we also find the method even if we permute arguments, the validation is made on the number of arguments only.

Then, if the call site store argument names (ie. not empty), we first check if the method is annotated @GoloFunction (ie. we know the formal parameters names) and then generate a permuteArguments handler
Voilà

I hope it answers your question @jponge

@jponge

This comment has been minimized.

Copy link
Member

jponge commented Mar 4, 2015

@danielpetisme That answers 200% my question 😄 🍻


NOTE: If you name one argument then you must name all the remaining argument. Otherwise, a compilation error will be raised.

Once you are using named parameters in your function call, the order doesn't matter anymore.

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Should be a note, too.

String typeDef = goloFunctionSignature(functionInvocation.getArity());
Handle handle = FUNCTION_INVOCATION_HANDLE;
List<Object> bootstrapArgs = new ArrayList<>();
bootstrapArgs.add(functionInvocation.isConstant() ? 1 : 0);

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Ok, got my answer for the point above 😄

type.toMethodDescriptorString(),
CLOSURE_INVOCATION_HANDLE,
functionInvocation.isConstant() ? 1 : 0);
} else {

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Haven't we lost a branch here?

This comment has been minimized.

@danielpetisme

danielpetisme Mar 4, 2015

Contributor

Refactoring
the visitInvokeDynamicInsn statement was used twice with different arguments. Now, the arguments are initialized for a standard function invocation and overrided if and only the above conditions are met.
The consequence is less code and only one visitInvokeDynamicInsn.

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

👍

@@ -60,6 +61,14 @@ public void setConstant(boolean constant) {
this.constant = constant;
}

public boolean useNamedArguments() {

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

usesNamedArguments() would be more appropriate.


package fr.insalyon.citi.golo.compiler.ir;

public class NamedArgument extends ExpressionStatement{

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Space before the {


@Override
public void accept(GoloIrVisitor visitor) {

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Not going through the expression?

This comment has been minimized.

@danielpetisme

danielpetisme Mar 4, 2015

Contributor

Currently no. May it helps to remove the named arguments list building.
https://github.com/golo-lang/golo-lang/pull/250/files#diff-135e9a72efe5f924909a57f244b00de1R472
Plus, change the return type from List<String> to void

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Ok, got it.

public static CallSite bootstrap(Lookup caller, String name, MethodType type, Object... bsmArgs) throws IllegalAccessException, ClassNotFoundException {
boolean constant = ((int)bsmArgs[0]) == 1;
String[] argumentNames = new String[bsmArgs.length - 1];
for (int i = 0; i < bsmArgs.length -1; i++) {

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Couldn't we use an intrinsic Arrays.copy(...) method?

This comment has been minimized.

@danielpetisme

danielpetisme Mar 5, 2015

Contributor

I have to redo my tests but the original array is an instance of Object[] and the target one is String[]. I'm sure I've tested it and it failed in the casting.
With the loop I'm sure to cast every elements.

I'll check again.

boolean constant = ((int)bsmArgs[0]) == 1;
String[] argumentNames = new String[bsmArgs.length - 1];
for (int i = 0; i < bsmArgs.length -1; i++) {
argumentNames[i] = (String) bsmArgs[i+1];

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

I like space around operators.

@@ -108,11 +118,15 @@ public static Object fallback(FunctionCallSite callSite, Object[] args) throws T
Method method = (Method) result;
checkLocalFunctionCallFromSameModuleAugmentation(method, callerClass.getName());
types = method.getParameterTypes();
if (method.isVarArgs() && isLastArgumentAnArray(types.length, args)) {
//TODO improve varargs support on named arguments. Matching the last param type + according argument

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Sounds interesting, but could you elaborate on that?

This comment has been minimized.

@danielpetisme

danielpetisme Mar 4, 2015

Contributor

a function call w/ named args + varargs found actually a varargs method but this test also check that the last arg is an array... In a named args context, the varargs arry could be at any place in the function call.
I thought about flagging the varargs parameter in the @GoloFunction but don't know if it's worthless.
So far all the tests run successfully so decided the good enough impl. (aka. Quick&Dirty 😄 )

This comment has been minimized.

@danielpetisme

danielpetisme Mar 4, 2015

Contributor

But I agree is not perfect.

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

I have a feeling that vararg arguments in presence of named arguments do not mix well... @yloiseau @artpej what do you think?

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Or else we should support something like f(a=1, b=2, 3, 4) but I'm not sure it's great.

This comment has been minimized.

@danielpetisme

danielpetisme Mar 5, 2015

Contributor

The array[] approach for named varargs parameter does the job. I mean everybody know that the ... is replaced by Object[] so it makes sense.
I'm not sure f(a = 1,2,3, b = 4, c = 5) would be clearer, plus from a parsing standpoint is way harder (at least for me 😄 )

On this particular line of code, the main issue is that currently only method flagged w/ varargs + having an array as last arg are trasnformed to collect all the trailing args into an array (is the job of asFixedArity). Without the test I added, the varargs value is an array of arrays (a first one obtained to the varargs flag on the method, and a second coming from the array[] notation.

Now in that case

function joiner = |delim, values...| -> ...
joiner(values = array[1,2,3], delim = "-")

The joiner method will be flagged as varargs but the collector is in the first position not the last...
The best option I can see is to add metadata in the generated annotation. Basically, flag the varags collecor then during the runtime, the above if will check that the argument corresponding to the flagged parameter is an instanceof Object[].

This comment has been minimized.

@yloiseau

yloiseau Mar 5, 2015

Contributor

The point of named varargs as in Python is not to get a array of values, but a map [arg name -> value], allowing to function to take named args that are not specified in the function signature (useful e.g. for HOF wrapping other functions)

This comment has been minimized.

@danielpetisme

danielpetisme Mar 5, 2015

Contributor

The map approach was my favorite but harder to implement. You create a type mismatch between the callsite target types and the target function definition. I'm sure it's possible (basically exploding the map into an array with the ordered arguments) but quid about perfs (and I know @jponge is picky on that topic :p).

This comment has been minimized.

@jponge

jponge Mar 5, 2015

Member

Passing arguments in a map blows performance. The approach that you have now (annotation + argument permutation combinator) is both elegant and fast 😃

This comment has been minimized.

@yloiseau

yloiseau Mar 5, 2015

Contributor

The map in python is only for variable named args, just as vararg in Java creates an array. But indeed, dealing with that can introduce an overhead.

@@ -144,6 +158,27 @@ public static Object fallback(FunctionCallSite callSite, Object[] args) throws T
}
}

private static MethodHandle reorderArguments(Method method, MethodHandle handle, String[] argumentNames) {

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Lovely

@@ -244,6 +279,7 @@ private static Object findClassWithStaticMethodOrField(Class<?> callerClass, Str
}

private static Object findStaticMethodOrField(Class<?> klass, String name, Object[] arguments) {

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

Not sure that blank line has much sense 😉

@@ -1618,4 +1610,32 @@ public void banged() throws Throwable {
assertThat(result, not((Object)42));

}

@Test
public void test_named_parameters() throws Throwable {

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

I would also add a test case for the cases where some arguments are named, and some are not.

This comment has been minimized.

@danielpetisme

danielpetisme Mar 4, 2015

Contributor

https://github.com/golo-lang/golo-lang/pull/250/files#diff-d6d7e547b440d72444c214f0442a6b1aR1642

Maybe throw a GoloCompilationException would be cleaner but witch type of problem is this? A PARSING problem?

This comment has been minimized.

@jponge

jponge Mar 4, 2015

Member

I would create a new enum entry specifically for that problem, and indeed throw a GoloCompilationException.

This comment has been minimized.

@danielpetisme

danielpetisme Mar 5, 2015

Contributor

There are only two hard things in Computer Science: cache invalidation and naming things.

-- Phil Karlton

Any idea ? is not a parsing issue, an IR issue ?

This comment has been minimized.

@jponge

jponge Mar 5, 2015

Member

Problem.Type.INCOMPLETE_NAMED_ARGUMENTS_USAGE? (or something like that)

@jponge

This comment has been minimized.

Copy link
Member

jponge commented Mar 4, 2015

This sounds really really good.

@jponge

This comment has been minimized.

Copy link
Member

jponge commented Mar 4, 2015

In terms of scheduling I would put this in 3.0.0, along with the Java 8 shift + functional interfaces integration.

@jponge jponge added the feature label Mar 4, 2015

@danielpetisme danielpetisme force-pushed the danielpetisme:wip/named-arguments branch 2 times, most recently from 22aa1e6 to 603ed6b Mar 4, 2015

@danielpetisme

This comment has been minimized.

Copy link
Contributor

danielpetisme commented Mar 4, 2015

To be franck, I didn't looked for the impact on the runtime performance (JMH newbie 😄 ). the 2 main updates on the runtime is the permuteArguments wrapping of the handler and the argument reordering.
Warning: so far the named args feature works only for golo functions not for closures (ClosureCallSupport hasn' t been modified)

In terms of roadmap, if Golo 3.0.0 will be based on Java8, wouldn' t be easier/cleaner to use the named argument feature ?
One merged, I' ll try to have a look on optional parameters (and why not default param.)

@jponge

This comment has been minimized.

Copy link
Member

jponge commented May 5, 2015

There are clever ways to deal with null ;-)

@danielpetisme

This comment has been minimized.

Copy link
Contributor

danielpetisme commented May 6, 2015

For me the PR is "feature complete". I'm all ears for your feedback.

NOTE: Once you are using named parameters in your function call, the order doesn't matter anymore.

CAUTION: If you name one argument then you must name all the remaining argument. Otherwise, a compilation error will be raised.

This comment has been minimized.

@yloiseau

yloiseau May 7, 2015

Contributor

Maybe add a note / explain more precisely how to use named arguments:

  • can I post("the title", "the body", tags=array[...], promoted=false) since you said “all the remaining argumentS”
  • in the presence of varargs (clear from the example, but a explaining note may be worth)

This comment has been minimized.

@danielpetisme

danielpetisme May 7, 2015

Contributor

Such snippet can't work.
Either you name all or none of your arguments.

For varargs, you expect a note explaining that you have to create an array to box the arguments?

This comment has been minimized.

@yloiseau

yloiseau May 7, 2015

Contributor

Yep, your last commit make the point

private List<String> visitInvocationArguments(AbstractInvocation invocation) {
List<String> argumentNames = new ArrayList<>();
for (ExpressionStatement argument : invocation.getArguments()) {
if (invocation instanceof FunctionInvocation && ((FunctionInvocation) invocation).usesNamedArguments()) {

This comment has been minimized.

@yloiseau

yloiseau May 7, 2015

Contributor

Can't we use named arguments on method invocation?

This comment has been minimized.

@danielpetisme

danielpetisme May 7, 2015

Contributor

Have a look to the java-interop
https://github.com/golo-lang/golo-lang/pull/250/files#diff-73d861b2e4cdd14514d7975925c2b003R65

Basicaly if the bytecode contains the parameters info it will work.

This comment has been minimized.

@yloiseau

yloiseau May 7, 2015

Contributor

I mean you put the named argument stuf on ir.FunctionInvocation not on ir.AbstractInvocation. So, I wonder if given e.g.

augment java.lang.String {
  function wrap = |this, prefix, suffix| -> prefix + this + suffix
}

such call

"world": wrap(suffix="!", prefix="Hello")

will work.

This comment has been minimized.

@yloiseau

yloiseau May 7, 2015

Contributor

Tested... Blam!

module Test

augment java.lang.String {
  function bar = |this, pref, suf| -> pref + " " + this + " " + suf
}

function main = |args| {
  println("world": bar(suf="!", pref="hello"))
}
$ golo golo --files test.golo
[error] Encountered " "=" "= "" at line 9, column 27.

This comment has been minimized.

@yloiseau

yloiseau May 8, 2015

Contributor

👍

@@ -1689,10 +1718,41 @@ public void banged() throws Throwable {

}

@Test
public void lambda8_interop() throws Throwable {

This comment has been minimized.

@yloiseau

yloiseau May 7, 2015

Contributor

no more a test?

This comment has been minimized.

@danielpetisme

danielpetisme May 7, 2015

Contributor

My bad, fixed

@jponge

This comment has been minimized.

Copy link
Member

jponge commented May 7, 2015

I'm adding doing a full review in my backlog :-)

@danielpetisme danielpetisme force-pushed the danielpetisme:wip/named-arguments branch from 0098fd4 to 5a82c91 May 7, 2015

@danielpetisme danielpetisme force-pushed the danielpetisme:wip/named-arguments branch from 5a82c91 to 6e80313 May 7, 2015

@@ -0,0 +1,49 @@
/*
* Copyright 2012-2014 Institut National des Sciences Appliquées de Lyon (INSA-Lyon)

This comment has been minimized.

@jponge

jponge May 15, 2015

Member

2012-(...)2015 :-)

@jponge

This comment has been minimized.

Copy link
Member

jponge commented May 15, 2015

The merge with #265 will require some conflict resolution I'm afraid, but it should not be too complex.

@danielpetisme

This comment has been minimized.

Copy link
Contributor

danielpetisme commented May 18, 2015

Indeed there are some conflicts. I gave an eye on them and it's really not big deal.

These are the differences git can not auto-merge when I try to merge #265 on top of #250.
I tried to analyse the differnces to find a way to permit a nice auto-merge 😉 .

1- In src/main/jjtree/Golo.jjt
Create an Argument token for the lexer + Changing the return type of AnonymousFunctionInvocation() from void to ASTAnonymousFunctionInvocation

<<<<<<< HEAD
void Argument():
{
  Token parameterId = null;
  Token expression;
}
{
  (LOOKAHEAD(2) (parameterId=<IDENTIFIER> "="))? ExpressionStatement()
  {
    if (parameterId != null) {
      jjtThis.setName(parameterId.image);
      jjtThis.setNamed(true);
    }
  }
}

void Arguments() #void: {}
{
  Argument() ("," (BlankLine())? Argument())*
}

void AnonymousFunctionInvocation():
=======
ASTAnonymousFunctionInvocation AnonymousFunctionInvocation():
>>>>>>> Fix LL(k) operators priority

I can change the return type on my branch it will works (and permit an auto-merge) but it would be inconsistent with the function usage (in my branch).
For my personal comprehesnsion what this change permits ?

2- In src/main/java/fr/insalyon/citi/golo/doc/ModuleDocumentation.java
The word Argument is for the expression used in the function invocation and Parameter for function declaration. Plus, there is an Indentation issue

@jponge did you change your IDE spacing settings ? The current master has a continuation indent of 2 spaces, your branch has 4 spaces.

<<<<<<< HEAD
          .arguments(node.getParameters())
          .varargs(node.isVarargs());
=======
            .arguments(node.getArguments())
            .varargs(node.isVarargs());
>>>>>>> Fix LL(k) operators priority

3- In src/main/java/fr/insalyon/citi/golo/compiler/ParseTreeToGoloIrVisitor.java , s/Parameter/Argument/ + improve the expression parsing

<<<<<<< HEAD
    for(String parameter : function.getParameters()) {
      ASTCommutativeExpression commutativeExpression = new ASTCommutativeExpression(0);
      functionInvocation.jjtAddChild(commutativeExpression, functionInvocation.jjtGetNumChildren());
      ASTAssociativeExpression associativeExpression = new ASTAssociativeExpression(0);
      commutativeExpression.jjtAddChild(associativeExpression, 0);
      ASTReference ref = new ASTReference(0);
      ref.setName(parameter);
      associativeExpression.jjtAddChild(ref, 0);
=======
    for (String argument : function.getArguments()) {
      ASTReference ref = new ASTReference(0);
      ref.setName(argument);
      functionInvocation.jjtAddChild(ref, functionInvocation.jjtGetNumChildren());
>>>>>>> Fix LL(k) operators priority

Don't know how we could re-arrange the code to auto-merge this one... :😖

The other solution is to merge one PR and fix the issue on the second one. Even if I would prefer an auto-merge, the differences are really no big deal to merge.

@jponge

This comment has been minimized.

Copy link
Member

jponge commented May 18, 2015

  1. It's because I needed to capture a reference to the element, so no big deal for you (in the end it's just like calling a non-void method and ignoring the return value if you don't need it).
  2. Perhaps just the nested indent has changed, otherwise I'm on 2-spaces.
  3. Basically ASTCommutativeExpression and ASTAssociativeExpression go away with my changes.

@jponge jponge merged commit 326b5ae into eclipse:master May 19, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielpetisme danielpetisme deleted the danielpetisme:wip/named-arguments branch May 22, 2015

@danielpetisme danielpetisme removed their assignment Jun 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment