Skip to content
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

Confusing handling of named arguments #7359

Closed
radeusgd opened this issue Jul 20, 2023 · 19 comments · Fixed by #7629
Closed

Confusing handling of named arguments #7359

radeusgd opened this issue Jul 20, 2023 · 19 comments · Fixed by #7629
Assignees
Labels

Comments

@radeusgd
Copy link
Member

radeusgd commented Jul 20, 2023

Enso reconciles currying, named arguments and argument default values. This however introduces some trade-offs.

TL;DR

There is a specification of named arguments behavior and to quote it:

Named arguments are applied in the order they are given.
This means that if you positionally apply to an argument foo and
then try to later apply to it by name, this will fail due to currying of functions.

@JaroslavTulach interprets that as a confirmation the current behavior is correct and all we can do is to improve error reporting as Radek concludes too.

Original Report

Here I encountered one such issue where the various trade-offs make the behaviour very confusing to the user.

from Standard.Base import all

fun a b c =
    IO.println "a = "+a.to_display_text
    IO.println "b = "+b.to_display_text
    IO.println "c = "+c.to_display_text
    a+b+c

main =
    IO.println '\n1:'
    IO.println <| fun "A" "B" "C"
    IO.println '\n2:'
    IO.println <| fun  b="B" "A" "C"
    IO.println '\n3:'
    IO.println <| fun "A" "C" b="B"
    IO.println '\n4:'
    IO.println <| Panic.recover Any <| (fun "A" "C" b="B").length
    IO.println '\n5:'
    IO.println <| fun "A" "C" b="B" "D"

yields us:

1:
a = A
b = B
c = C
ABC

2:
a = A
b = B
c = C
ABC

3:
named-args.fun[named-args.enso:3-22]

4:
(Error: (No_Such_Method.Error named-args.fun[named-args.enso:3-22] UnresolvedSymbol<length>))

5:
a = A
b = C
c = D
Execution finished with an error: Type error: expected a function, but got ACD.
        at <enso> named-args.main<arg-1>(named-args.enso:19:19-39)
        at <enso> named-args.main(named-args.enso:19:5-39)

Let's unpack what is happening here.

  1. We just provide all arguments positionally - all is well.
  2. We provide the b argument as named - it is correctly reordered.
  3. We provide the 3 arguments, but now the named b argument comes last. This is the most interesting example showing the issue. Due to how our argument resolution works (and this is not fully avoidable if we want to support currying, at least not without some careful limitations), what the system sees: I see argument "A", I pass it to parameter a, then I see "C", I've got a function waiting for an argument named b - OK all looks good - let's take it. Then I see a named argument b="B". OK but I have a function taking a parameter c only - no match - the resolver "suspends" this argument, hoping that once more arguments are provided, maybe our function will return a new function taking more arguments and one of its names will match my pending b? Maybe. For now I return a value containing a Function waiting for one parameter - that is what is printed. Note that this 'suspended pending argument' b="B" is hidden and it is impossible to see that we are waiting for this argument.
  4. Actually I want to come to this in a moment, below.
  5. Here we see the same behaviour as in (3), just continued. What happened is that we did provide another argument "D". So my function from step (3) was waiting for one more parameter (named c). I got an unnamed argument "D" - OK - that sounds good - I provide this argument for the parameter c. This runs my function fun with args a="A", b="C" and c="D" - it executes and returns a Text value "ACD". But the resolution continues - I'm still pending a named argument b="B". Ok but what the function returned is a Text value, not a Function - so a type error is invoked.

The case (5) alone is problematic, as I get a weird type error which does not really tell me what went wrong. That case is actually an instance of #5219.

But (4) shows something even more concerning. What I do is I expect fun to return me a Text, so I call length on it. What happens then is that my resolution is currently suspended. It resolved to a function that is waiting for one parameter named c and is also waiting to apply one named argument b="B". The object returned is a Function holding all that 'state'. I call length on this and I get a message that a Function does not have a method called length. Well, fair - functions don't have those. But I have not expected a Function, I've provided 3 arguments to my function fun and I expect it to return a Text. Instead, I see I'm getting a function - so this looks like a usual case of "not enough arguments" - if I have a function f x y and provide just IO.println (f 1) I get a function. But wait - I look at the code - fun is expecting 3 parameters and I did provide 3 arguments!. The issue is that the argument b="B" did not match and is silently suspended, with no way to see it.

I've just lost about 30-40 minutes trying to track down a case in my code when this was happening. It is a very confusing kind of error message, because it suggests we do not have enough arguments, but we have all the arguments that we need!

@radeusgd
Copy link
Member Author

I'm not sure what would be the proper solution for this.

  1. I think from the user perspective - ideally, we should make sure that named arguments work even if reordered. I'm not sure how viable that is because we are limited by other requirements, like ensuring named arguments propagate through currying.
  2. If we cannot make such reordered named arguments work, we should probably at least try to make sure the error message that is raised is clearer - the current one is really confusing. This would likely be the same issue/solution as Mismatched named arguments could result in a more user-friendly error #5219. Likely if we get something like: "Unexpected named argument b", it could be a bit easier to track down what the issue is.

@radeusgd
Copy link
Member Author

radeusgd commented Jul 20, 2023

This behaviour is consistent with point (2) of the Named Arguments section in the function arguments design: https://enso.org/docs/developer/enso/syntax/function-arguments.html#named-arguments

This probably rules out (1) - as I suspected - it is unlikely possible to reconcile reordering named arguments with currying.

However still, I think we need to look into (2) - currently the user experience of using named arguments can become a rabbit hole.

Maybe actually we could somehow 'group' arguments to user-facing components that do not resort on currying and still have (1)? I'm not a fan of doing this as that would make the language less uniform. However from the UX perspective - we definitely want to have clear error messages and intuitive handling of arguments - and the current state is quite far from that - as shown here or in #5219 - i.e. when you misspell an argument name.

This may not be as high priority though, as the IDE should generally manage arguments of nodes and so it should be much harder to trigger these errors in the IDE. I imagine the only scenario is when the library version has changed between runs of a workflow and an argument was renamed there.

The developer documentation for the Enso compiler, runtime, and IDE..

@GregoryTravis
Copy link
Contributor

For case 3: After we've supplied "A" and "C" (as parameters a and b), we return a function expecting a parameter c. There is a suspended named parameter, but it is the wrong name, so we would expect it to say that the argument to the partially-applied function is the wrong name, right? So just having b="B" there is immediately an error.

So that in case 4, we don't even get to the point of trying to call length, since we already hit the error with b="B".

@radeusgd
Copy link
Member Author

For case 3: After we've supplied "A" and "C" (as parameters a and b), we return a function expecting a parameter c. There is a suspended named parameter, but it is the wrong name, so we would expect it to say that the argument to the partially-applied function is the wrong name, right? So just having b="B" there is immediately an error.

So that in case 4, we don't even get to the point of trying to call length, since we already hit the error with b="B".

Yes, that would be ideal. I think there are some challenges to that (we need to ensure that some function returned further downward the chain will not be taking this parameter).

We can relatively easily check this if the return value evaluates to something that is not a Function anymore. Then, if we still have 'pending' named arguments but have something that is not a Function, instead of a Type error, we could display this more specific error.

But the general case and one shown in (4) is actually not that simple. The issue is currying - at the point where we are, we do not know if after we get the last parameter we are waiting for (c), if the function fun will not return a new function, expecting the parameter b. So we cannot error immediately :/

e.g.

fun2 a b c =
    my_b = b
    if c == "D" then b-> a+my_b+b+c else z-> a+b+c+z

As demonstrated in fun2, the condition cannot be known a-priori:

fun2 "A" "C" (b="B")

Here we don't know what the parameter be and if b will be valid or not:

If we do

fun2 "A" "C" (b="B") "D"

Then b="B" will happily match, we'll get back "ACBD".

But if we do

fun2 "A" "C" (b="B") "X"

then this will reduce to a function waiting for parameters named z but having a pending named argument b="B" - again we do not know anything because we can get one more argument and maybe we will get back a curried function that does take "B" - determining this in general cases is essentially equivalent to the halting problem and also can be non-deterministic if we have side effects.

@radeusgd
Copy link
Member Author

I guess what we can do, is detect cases where a Type error is thrown or whenever a "Function value with pending unresolved named arguments" is encountered, and handle the errors specially.

In particular, I'm not sure that the entity "Function value with pending unresolved named arguments" should be of type Function. I'm not sure if it can be any other type, but at least IMO we should print in its textual representation that there are these pending arguments.

This is really hard, I'm not sure what the right solution should be but we definitely should try to improve this as currently the errors can be very misleading and basically unreadable.

@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Jul 25, 2023
@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to ⚙️ Design in Issues Board Jul 26, 2023
@jdunkerley jdunkerley moved this from ⚙️ Design to 📤 Backlog in Issues Board Aug 22, 2023
@jdunkerley
Copy link
Member

Can we include looking at partially applied constructors within the type checking in this ticket please.

@radeusgd
Copy link
Member Author

radeusgd commented Aug 22, 2023

Can we include looking at partially applied constructors within the type checking in this ticket please.

These seem to be rather completely unrelated issues, shouldn't we create a separate ticket for it?

Or are they actually related?

@JaroslavTulach
Copy link
Member

Or are they actually related?

I think they are related.

all we can do is to improve error reporting

If there is an argument that requires some type, but gets Function, it is a sign of ...

partially applied

... arguments. Of a function or ...

constructor

... at that moment we should look at FunctionSchema and describe what arguments need to be provided or what are the over-saturated arguments and report that to the user. When I heard about the partially applied constructor problem, I immediately thought the fix is going to be at the same place.

@radeusgd
Copy link
Member Author

Or are they actually related?

I think they are related.

I see, ok. It will be great to have both of these issues improved 🙂

@enso-bot
Copy link

enso-bot bot commented Aug 24, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-23):

Progress: - fixing ReplTest & co. error improvements: ea23fc9

Next Day: Python Interop on Mac CI + fixing the x._ problem

@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to 👁️ Code review in Issues Board Aug 24, 2023
@mergify mergify bot closed this as completed in #7629 Aug 24, 2023
mergify bot pushed a commit that referenced this issue Aug 24, 2023
Fixes #7359 by printing more information about the function including partially applied arguments and over-saturated arguments.
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Aug 24, 2023
@radeusgd
Copy link
Member Author

radeusgd commented Sep 1, 2023

The improvements in #7629 are amazing and have improved the issue greatly.

But I think there is still some cases that need some love.

Please see the following example:

from Standard.Base import all

foo x y z=42 =
    x+y+z

main =
    IO.println (foo 1 2 3)
    IO.println (foo 10 20)
    IO.println (foo z=23 y=52)
    IO.println (foo x=100 w=200)
    IO.println (Panic.catch Any (foo 10 20 w=300) (caught-> caught.payload.to_display_text))

it yields:

6
72
bugs123.foo[bugs123.enso:3-4] self=bugs123 x=_ y=52
bugs123.foo[bugs123.enso:3-4] self=bugs123 x=100 y=_ +w=200
Type error: expected a function, but got 72.

I can see two problems with that:

  1. (Rather minor issue) The z argument is not displayed on the third line, even though it has beem set z=23.
    • I think that may be a bit confusing. I thought the representation was giving us a pretty good overview of what arguments are already applied. But here we have an applied argument that is hidden.
  2. (the main issue) If I mistype the name of a default argument (lines 4 and 5) but provide all required arguments (line 5), I do not get any help from the system, I still get a cryptic error saying expected a function but got 72.

The error like described in (2) / line 5 is acceptable if all arguments were filled in - we already computed a value and the user is providing it more arguments. But here not all arguments were filled in. It was only the required ones. I'm wondering if we could somehow be able to tell the user that the argument name was wrong (i.e. got w but expected z).


I'm not exactly sure how we can best solve it (because there are limitations), but I think it may be worth to investigate this further.

One problematic limitation to the example above is, I can have another example like:

from Standard.Base import all

foo x y z=33 =
    case z of
        42 ->
            w-> x + y + z + w
        _ -> 
            x + y + z

main =
    IO.println (foo 10 20 w=300 z=42)
    IO.println (Panic.catch Any (foo 10 20 w=300 z=1000) (caught-> caught.payload.to_display_text))
    IO.println (Panic.catch Any (foo 10 20 w=300) (caught-> caught.payload.to_display_text))
372
Type error: expected a function, but got 1030.
Type error: expected a function, but got 63.

Here, we can see that I cannot 'just' stop at 'expecting argument z but got w' and issue an error, because if I actually continue the evaluation with the oversaturated w argument 'waiting in line', I can actually get to a valid result.

I'm not sure how to solve this.

@JaroslavTulach
Copy link
Member

The z argument is not displayed on the third line, even though it has beem set z=23

As designed. We don't display arguments with default values. You voted +1.

@JaroslavTulach
Copy link
Member

I'm not sure how to solve this.

I believe it could be solved by removing the possibility to re-order arguments. But as @kustosz wrote:

it would be very annoying in practice as the point of named arguments is to not think about order

I am not sure it is the most important point, I believe the point of named+defaulted arguments is to not have to specify them all.

I believe that, if we remove the ability to specify arguments in random order, we can solve these issues. However it is a significant change to the language and needs to be discussed with major stakeholders. I don't think it would affect the IDE experience, so it may be acceptable, but I happily leave the negotiations up to you, Radek.

@somebody1234
Copy link
Contributor

my two cents: specifying w out of place should be an error - default parameters being able to be specified out of order is fine, but they shouldn't be allowed to be specified out of order when the arguments come from different declarations. i think this is especially true if w does not show up in the docs in the first place, in which case i think it would be counterintuitive to accept w before z

... alternatively another possibility may be to add metadata about function parameters to the type, to make the entire call possible to statically analyze?

@radeusgd
Copy link
Member Author

radeusgd commented Sep 2, 2023

The z argument is not displayed on the third line, even though it has beem set z=23

As designed. We don't display arguments with default values. You voted +1.

I though this applies only to argument with default value. Then that's ok. But here the value is overridden with value provided by the user. And in that case not displaying it does not seem ok.

@JaroslavTulach
Copy link
Member

... the value is overridden with value provided by the user...

OK, that can be fixed. You can report a bug for me.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Sep 4, 2023

my two cents: specifying w out of place should be an error - default parameters being able to be specified out of order is fine, but they shouldn't be allowed to be specified out of order when the arguments come from different declarations

One thing to mention is currying. There should be no way to tell a difference between f(a, b, c) and g(a)(b)(c) which shall behave the same if f is curried version of g or vice versa. Named arguments are particularly nasty from this perspective as one can use f c=3 and then that has to work with g c=3.

However g c=3 is an application of an over-saturated argument and the whole runtime is quite complicated because of supporting that. Moreover it creates additional problem of properly reporting an error as g anything=something is valid until g gets saturated & evaluated.

If we want to improve the error messages, then (I believe) we have to restrict the freedom people have when providing arguments to a function. E.g. f c=3 is an error, if a or b is missing. Or (if a and b have default values), then f c=3 is OK, but the values of a and b are immediately set to their default values and can no longer be modified.

@radeusgd
Copy link
Member Author

radeusgd commented Sep 4, 2023

... the value is overridden with value provided by the user...

OK, that can be fixed. You can report a bug for me.

Ok: #7723

@radeusgd
Copy link
Member Author

radeusgd commented Sep 4, 2023

my two cents: specifying w out of place should be an error - default parameters being able to be specified out of order is fine, but they shouldn't be allowed to be specified out of order when the arguments come from different declarations

One thing to mention is currying. There should be no way to tell a difference between f(a, b, c) and g(a)(b)(c) which shall behave the same if f is curried version of g or vice versa. Named arguments are particularly nasty from this perspective as one can use f c=3 and then that has to work with g c=3.

However g c=3 is an application of an over-saturated argument and the whole runtime is quite complicated because of supporting that. Moreover it creates additional problem of properly reporting an error as g anything=something is valid until g gets saturated & evaluated.

If we want to improve the error messages, then (I believe) we have to restrict the freedom people have when providing arguments to a function. E.g. f c=3 is an error, if a or b is missing. Or (if a and b have default values), then f c=3 is OK, but the values of a and b are immediately set to their default values and can no longer be modified.

I like your idea. I think looking at the practical use of the language, it is really important to get good, easy to understand, error messages.

Your PR has already improved this vastly, but I think we are not yet 'there'. What you are proposing here could make it much better.

As you mentioned, this would not change the semantics from the GUI perspective, since there we specify the arguments in the right order 'anyway'. It may make the life of text users slightly harder, but IMHO the benefit of better error messages far exceeds the benefit of being able to reorder named arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants