Function Default Arguments

[assem2002]

fixes #2509 fixes #2109
completed the implementation of default arguments in functions.

Code Flow

• AST::functiondef has arguments object as one of its members, this object has m_defaults and n_defaults which preserve the number of default arguments and AST::expr_t of the default constants.
• When we're building the variables of the function in visit_functiondef( ) , we start pushing the ASR::expr_t of the defaults as a value for the variable when we reach the difference of n_args and n_defaults .
• When we start to call the function, we check if n_pos_args is less than func.n_args , if so, we start pushing the data from varibles in ASR::function_t to Vec<ASR::call_arg_t>,starting from the index n_pos_args.

The code handles keyword arguments.

serious issue to handle

The solution is build upon the idea of starting to make variable for the last number of parameters. If the user writes a code like this

def whatever(first :str ="lpython",second :str):
    print(second)

whatever("foo") # result => lpython

as the lpython would be matched with the last parameter.

This should arise a syntax error like how Cpython does.

[assem2002]

The errors arises from converting the lpython code into "c code" as c doesn't provide default arguments. Should i change the process of converting lpython-> c , so it doesn't make a default arguments assignment?

[assem2002]

If the solution seems to be doing the job correctly. I would start handling the syntax error to make the solution complete without any flaws. I just want to make sure it's the right approach to handle such a problem.

[Shaikh-Ubaid]

If the solution seems to be doing the job correctly. I would start handling the syntax error to make the solution complete without any flaws. I just want to make sure it's the right approach to handle such a problem.

You can start another PR to support the semantic error. I suspect that the implementation might require updates in the parser.

[Shaikh-Ubaid]

Please mark it as "Ready for review" when ready.

[assem2002]

If the solution seems to be doing the job correctly. I would start handling the syntax error to make the solution complete without any flaws. I just want to make sure it's the right approach to handle such a problem.

You can start another PR to support the semantic error. I suspect that the implementation might require updates in the parser.

I think we can handle such a thing on the parser level as you mentioned. we can handle it by using a default_argumets_started_ flag = false to throw an error if ==> default_argumets_started_ flag = true and the current argument being built doesn't have a default argument.

[Shaikh-Ubaid]

we can handle it by using a default_argumetsstarted flag = false to throw an error if ==> default_argumetsstarted flag = true and the current argument being built doesn't have a default argument.

Go ahead and submit a PR with your approach and I will take a look.

[Shaikh-Ubaid]

I suggest implementing the semantic error in a separate PR.

[assem2002]

I suggest implementing the semantic error in a separate PR.

I'll do right after handling this PR

[assem2002]

@Shaikh-Ubaid

I faced a test case which I can't say whether it's a valid test case or not the following:

def default_func(x : str ="Hello" ,y : str = " ", z : str = "World") ->str:
    return x + y + z

default_func(z="lpython")

It results in the following error:

semantic error: default_func() got multiple values for argument 'z'
  --> ../../integration_tests/def_func_01.py:49:20
   |
49 |     test_06 :str = default_func(z="lpython")
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^ 

should I handle this in the parsing phase to prevent user form passing keyword arguments for the last parameters ?, instead the user should pass keyword arguments only if he filled some sequence in the beginning of the parameter list like that:

default_func(y = "///",x="Hi")  # result => "Hi///World"

This prevention still fits the convention that most programming languages use, which states that whenever you pass an argument to a default argument function,you can't pass it arbitrarily but you should follow the following pattern : [passed arguments by user] + [default arguments to be handled by compiler]

and not -> [passed arguments by user] + [default arguments to be handled by compiler] + [passed arguments by user]

[Shaikh-Ubaid]

I faced a test case which I can't say whether it's a valid test case or not

If the test case runs in cpython then its a valid test case.

[assem2002]

I faced a test case which I can't say whether it's a valid test case or not

If the test case runs in cpython then its a valid test case.

Actually it passes this testcase. I will try to find a way to handle it.

[assem2002]

@Shaikh-Ubaid I handled the test case.

def default_func(x : str ="Hello" ,y : str = " ", z : str = "World") ->str:
    return x + y + z

    test_09 :str = default_func(y = "++",z = "LPython")
    print("test_09 is =>",test_09)
    assert test_09 == "Hello++LPython"

    test_10 :str = default_func("Welcome",z = "LPython")
    print("test_10 is =>",test_10)
    assert test_10 == "Welcome LPython"

Solution Flow

• If only positional arguments are passed and aren't matching func->n_args, we start from the last argument in function_t and push back in Vec<args>, if there's no value to push then break.
• If there's keyword arguments passed, We reserve Vec<args> with func->n_args and then visit_expr_list() starts to place set all arguments to nullptr, then pushes back positional arguments, then sets keyword arguments in its desired index in Vec<args> , then we loop onVec<args> to see if there's a nullptr arg that we can have a matching default argument for.

[certik]

Thanks!

To implement default arguments, the following must be done:

• At the function definition time, find the default argument and put it into the argument's Variable symbolic_value into the symbol table
• At the function call site, simply lookup any default arguments in ASR, and if present, use them by explicitly inserting them

The alternative design is to not even insert them and sort it out inside the function, but I would not do this, since it is not explicit. I would always insert them at the call site.

[kmr-srbh]

@assem2002 could you please test for this case?

from lpython import i32

def fn(defarg1: str="default", arg: i32, defarg2: str="default") -> i32:
    return 1

fn()

This should throw an error.

[assem2002]

@assem2002 could you please test for this case?

from lpython import i32

def fn(defarg1: str="default", arg: i32, defarg2: str="default") -> i32:
    return 1

fn()

This should throw an error.

Yeah it should, but unfortunately it doesn't I talked with Shaikh Ubaid about this, We agreed to push another pull request to handle this problem on the parsing stage not in the ASR stage.

[assem2002]

Thanks!

To implement default arguments, the following must be done:

* At the function definition time, find the default argument and put it into the argument's Variable `symbolic_value` into the symbol table

* At the function call site, simply lookup any default arguments in ASR, and if present, use them by explicitly inserting them

The alternative design is to not even insert them and sort it out inside the function, but I would not do this, since it is not explicit. I would always insert them at the call site.

Does that mean the AST of a call to a function with default arguments will include the default arguments passed even if they weren't provided by user? If so, isn't the whole AST is built then ASR?

Or what you mean is to edit the call to 'make_call_helper()' to include the default arguments?

[Shaikh-Ubaid]

The approach that Ondrej shared comprises of the following two steps:

Step 1

For default function arguments, their default value should be inserted in the functions symbol table inside the Variable declaration. Precisely, the default value should be inserted in the variable's symbolic_value and value in its declaration inside the symbol table of each function.

Step 2

For each function call, whenever we need to use the default argument value we can simply lookup the symbol table of the called function, access the symbolic_value (or value) for the variable whose default value we need and insert this default value in the function call.

Please let me know if this makes it clear.

[assem2002]

The approach that Ondrej shared comprises of the following two steps:

Step 1

For default function arguments, their default value should be inserted in the functions symbol table inside the Variable declaration. Precisely, the default value should be inserted in the variable's symbolic_value and value in its declaration inside the symbol table of each function.

Step 2

For each function call, whenever we need to use the default argument value we can simply lookup the symbol table of the called function, access the symbolic_value (or value) for the variable whose default value we need and insert this default value in the function call.

Please let me know if this makes it clear.

I got the first step. does the second step should be similar to that? :

            (for size_t i = index_start_def_arguments; i < func-> n_args; i++){
                ASR::var_t* var  = ASR::down_cast<ASR::Var_t>(func->m_args[i]);
                std::string symbol_name = ASRUtils::symbol_name(arg_Var->m_v);
                ASR::symbol_t* sym = func->m_symtab->getsymbol(symbol_name);
                //then accessing the value, and pushing in call_args vector.
            }

[Shaikh-Ubaid]

does the second step should be similar to that?

Seems in the right direction. Also, note that you can simply get the symbol from Var_t as follows:

ASR::Var_t* var  = ASR::down_cast<ASR::Var_t>(func->m_args[i]);
ASR::symbol_t* sym = var->m_v;

[assem2002]

Seems in the right direction. Also, note that you can simply get the symbol from Var_t as follows:

actually I'm confused at the moment. My PR handles the issue of passing the default arguments by sorting out arguments in Vec<call_args> (logical reasoning about passed arguments by user and the parameters of the function itself),

  for (size_t def_arg = args.size(); def_arg < func->n_args; def_arg++){
      ASR::symbol_t* sym = ASR::down_cast<ASR::Var_t>(func->m_args[def_arg])->m_v;
      ASR::Variable_t* var = ASR::down_cast<ASR::Variable_t>(sym);
      if (var->m_value == nullptr) {
          break;
      }
      ASR::call_arg_t call_arg;
      args.push_back(al,call_arg);
      args.p[def_arg].loc = ((var->m_value)->base).loc;
      args.p[def_arg].m_value = (var->m_value);

which Ondrej thinks isn't explicit, so he is proposing the approach of pushing arguments by looking up their values in the symbol Table and get the value in symbolic value if exist.This approach could be done like that:

(for size_t i = index_start_def_arguments; i < func-> n_args; i++){
    ASR::var_t* var  = ASR::down_cast<ASR::Var_t>(func->m_args[i]);
    std::string symbol_name = ASRUtils::symbol_name(arg_Var->m_v);
    ASR::symbol_t* sym = func->m_symtab->getsymbol(symbol_name);
    //then accessing the value, and pushing in call_args vector.
}

which seems better i can just look up the arguments by their name instead of manipulating the logic of the arguments positions passed the make_call_helper( ). Using previous approach is really more explicit to the reader of the code and it would handle the issue of keyword arguments that appeared in some test cases in more explicit way.

I hope I got what you mean. If I completely lost track of what you want to implement, I hope you can guide me through an article or something.

[Shaikh-Ubaid]

which Ondrej thinks isn't explicit, so he is proposing the approach of pushing arguments by looking up their values in the symbol Table and get the value in symbolic value if exist

As of now, I think the symbolic value for a functions default argument is not inserted. That is, the symbolic value for the default argument is currently null at the ASR level. Even if the user has provided the default value in the AST, we just skip processing it while constructing the ASR.

As a part of this PR, the first step should be to process the default values of function arguments (if any) provided in the AST and ensure that these get appropriately reflected in the symbolic_value of the function argument declaration in the function's symbol table.

Please let me know if this helps.

[assem2002]

processing it while constructing the ASR.

I read about interpreter design in the classic SICP, and I see what you meant to say. Though the PR's current solution is working fine, but still not the convenient way to do such a thing. I think the PR is close to the desired solution,I'll push some edits to meet these requirements.

[assem2002]

@Shaikh-Ubaid @certik Now I fetch the value of the default arguments by looking up the symbol table of the scope of the function that has been called. Let me know if that's the right approach to do so or it needs to get changed.

[Shaikh-Ubaid]

Where is the m_symbolic_value being set in the first place? I couldn't see it in the PR. I left some minor comments.

I think it is not being set. @assem2002 as we shared before, the m_symbolic_value should be first set (it should be visible for the default parameters of the function when we do --show-asr). Then, on function call when sufficient arguments are not provided, we simply look up the m_symbolic_value for the missing arguments and insert it in the function call.

[assem2002]

Where is the m_symbolic_value being set in the first place? I couldn't see it in the PR. I left some minor comments.

I think it is not being set. @assem2002 as we shared before, the m_symbolic_value should be first set (it should be visible for the default parameters of the function when we do --show-asr). Then, on function call when sufficient arguments are not provided, we simply look up the m_symbolic_value for the missing arguments and insert it in the function call.

It does appear in the ASR

def default_func(x : str ="Hello", y : str = " ", z : str = "World") ->str:
    return x + y + z

the symbolic_value of the variable of the function parameters is set in the following line of code

  if (i >= default_arg_index_start){
      size_t default_arg_index = i - default_arg_index_start;
      this->visit_expr(*(x.m_args.m_defaults[default_arg_index]));
      init_expr = ASRUtils::EXPR(tmp);  // <<<<<<<<<<<<<<<<<<<<<<<<<HERE

init_expr is the value that is going to be passed to symbolic_value while building the variable in make_variable( )

and yeah i know this PR isn't much different, but while processing a function call, instead of fetching the values directly we search the scope for the name of the arguments. I think it's a matter of clarity in the code not more than that. @Shaikh-Ubaid

[certik]

Indeed, it seems it is already set in ASR.

[assem2002]

Indeed, it seems it is already set in ASR.

In the current version on lpython repo, The ASR of any function doesn't know any information about the default arguments as it never get passed to the function_t object while building the ASR

[Shaikh-Ubaid]

Please mark as "Ready for review" when ready.

[assem2002]

Please mark as "Ready for review" when ready.

@Shaikh-Ubaid I need your opinion on this link and this link . to handle your suggestions on the code properly.

[Shaikh-Ubaid]

https://github.com/lcompilers/lpython/pull/2618#discussion_r1585139929 is a simple code improvement suggestion. Please try it and share if it works.

For https://github.com/lcompilers/lpython/pull/2618#discussion_r1585109447, I am unsure why we have used break.

[assem2002]

#2618 (comment) is a simple code improvement suggestion. Please try it and share if it works.

For #2618 (comment), I am unsure why we have used break.

For the break thing : I loop through the not-passed function parameters and try to look for their defaults (if exist), once i notice i have a missed parameter with no default for it, i break the loop as there's no need to proceed with the loop (NOTICE : default values are passed sequentially at the end of the parameters list). ............ About the modifications I'll test it and let you know about it.

[Shaikh-Ubaid]

I see. You break the loop and you get an error that the number of arguments do not match as follows:

% cat examples/expr2.py  
from lpython import i32

def default_func(x: i32, y: i32, z: i32) -> i32:
    return x + y + z

res: i32 = default_func(1, 2)
% python examples/expr2.py
Traceback (most recent call last):
  File "/Users/ubaid/Desktop/OpenSource/lpython/examples/expr2.py", line 6, in <module>
    res: i32 = default_func(1, 2)
TypeError: default_func() missing 1 required positional argument: 'z'
% lpython examples/expr2.py
semantic error: Number of arguments does not match in the function call
 --> examples/expr2.py:6:12
  |
6 | res: i32 = default_func(1, 2)
  |            ^^^^^^^^^^^^^^^^^^ (found: '2', expected: '3')

Note: Please report unclear or confusing messages as bugs at
https://github.com/lcompilers/lpython/issues.

Instead of the break, let's throw a SemanticError() saying that "missing required argument 'z'" or something similar.

About the https://github.com/lcompilers/lpython/pull/2618#discussion_r1585139929 I'll test it and let you know about it.

Yes, please try it and let us know.

[Shaikh-Ubaid]

Please mark as "Ready for review" when ready.

[assem2002]

@Shaikh-Ubaid Okay, I'll write such an error message and let you know. but why there wasn't such a message before?

[Shaikh-Ubaid]

but why there wasn't such a message before?

Previously we just checked for mismatch in the number of arguments. With the changes in this PR, I think we can now pinpoint to which particular argument is missing.

[assem2002]

• I raised a semantic error (pushed a diagnostic) instead of breaking.

  
  def func(a : i32, b : str, c : str, d : i32 = 11111) ->i32:
  print (a,b,c,d)
  return 0

func(b = "lpython")

```bash
semantic error: Number of arguments does not match in the function call
 --> ../../../test.py:5:1
  |
5 | func(b = "lpython")
  | ^^^^^^^^^^^^^^^^^^^ missing 2 required arguments :'a' and 'c'

• Edited some typos. @Shaikh-Ubaid

[kmr-srbh]

I think there might be some more changes required with the PR. The following must have failed, but executes normally:

from lpython import i32

def fn(arg1: i32, arg2: i32, arg3: i32=1) -> i32:
    return arg1 + arg2 + arg3

print(fn(arg3=2, 1, 3))

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
6

CPython throws the expected SyntaxError due to positional arguments following keyword arguments. I found the following error when thinking of some more testcases to add.

I think this PR is specifically handling the filling of default arguments. We can focus on this issue later if this PR is not the right place to handle it.

[assem2002]

I think there might be some more changes required with the PR. The following must have failed, but executes normally:

from lpython import i32

def fn(arg1: i32, arg2: i32, arg3: i32=1) -> i32:
    return arg1 + arg2 + arg3

print(fn(arg3=2, 1, 3))

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
6

CPython throws the expected SyntaxError due to positional arguments following keyword arguments. I found the following error when thinking of some more testcases to add.

I think this PR is specifically handling the filling of default arguments. We can focus on this issue later if this PR is not the right place to handle it.

Yeah you're right. We suggest to handle this syntax error in another issue in addition to that problem too link

[Shaikh-Ubaid]

The PR seems to be getting longer. So let's merge this and work iteratively on the rest of the things (if any) in a new PR.