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

WIP better error msg #6751

Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open

WIP better error msg #6751

wants to merge 51 commits into from

Conversation

dadhi
Copy link

@dadhi dadhi commented May 15, 2024

TL;DR;

The PR adds the usage examples into the error message for TOO FEW ARGS and adjusts the tests.

Explanation

The PR is improving the error message for the too few arguments to a function by adding the examples into the message, specifically in the piping context.

I am learning the Roc language and trying to find analogs from my background (C# and a bit of F#, Java, Scala).
Trying the pipe operator to replicate C# LINQ (collection processing), I was confused by the error message:

Roc does not allow functions to be partially applied. Use a closure to
make partial application explicit.

I know what the closure is generally, but what does it mean to make partial application explicit.
There are too many arbitrary words glued together with the meaning lost on me, sorry :)
And what should I do practically to move on?

So the examples may help me to unblock and illustrate explicit closures/lambdas and partial application.
Like this:

    Roc does not allow functions to be partially applied. Use a closure to
    make partial application explicit.

    For example: ["a", "b"] |> \list -> Str.joinWith list ", "

    or simplier: ["a", "b"] |> Str.joinWith ", "

@@ -1675,7 +1675,11 @@ mod test_reporting {

Roc does not allow functions to be partially applied. Use a closure to
make partial application explicit.
"

For example: ["a", "b"] |> \list -> Str.joinWith list ", "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea! What if we give an example that doesn't have a simplified version, so there's no need for the second line? For example:

List.map nums \num -> num + 3

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rtfeldman Thanks for replying!

In regard to your proposal:

In your example, the closure (lambda) means a different thing comparing to the source of error.

Here is the original error message taken from the tests:

── TOO FEW ARGS ────────────────────────────────────────────────────────────────

The add function expects 2 arguments, but it got only 1:

4│      Num.add 2
        ^^^^^^^

Roc does not allow functions to be partially applied. Use a closure to
make partial application explicit.

My first attempt at providing the example would be:

For example: \n -> Num.add n 2

It is a good illustration, but if I substitute it into Num.add 2 it does not make sense.

That's why it may be helpful to provide the valid context for the closure, e.g. the pipe operation.

The new idea:

I think we don't need a context if we demonstrate the problem and solution side-by-side.

For example instead of `Num.add 2` use `\n -> Num.add n 2`

@rtfeldman What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting!

Thinking about it some more, it occurs to me that people who encounter this message might be in one of a few different scenarios:

  • They forgot an argument and are going to add it
  • They're used to languages where arguments can be omitted (e.g. in JavaScript you can leave off an argument and the argument will be undefined in the function body)
  • They're used to curried languages and expected this to result in partial application

I think a lot of people reading the error message in the first two scenarios won't know what "partial application" means, and would actually have a better experience if we didn't bring that up at all - because they might wonder "wait, is partial application something I should be doing here?" when actually they just forgot an argument.

Here's an idea for an error message that might be clearer for people in different scenarios.

Today:

The `f` function expects 2 arguments, but it got only 1:

   7│      f 1
           ^

Roc does not allow functions to be partially applied. Use a closure
to make partial application explicit.

Idea:

The `f` function expects 2 arguments, but it got only 1:

   7│      f 1
           ^

Calling a Roc function requires providing all of its arguments.

I think this wording would be clear for people in all 3 scenarios:

  • Forgot an argument: it's clear that they forgot an argument and need to provide it, and it doesn't bring up new terminology that might lead them down the wrong path
  • Thought arguments might be optional: makes it clear that all arguments are required
  • Expected partial application: makes it clear that all arguments are required

I think the number of people who know what partial application is but don't know how to do it by hand will be very low, so I think this might end up being the most helpful message to people encountering it from different backgrounds!

What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I agree that the words of partial application should be set aside and more "based" explanation provided: all arguments should be provided looks good to me.

But we still have 2 different contexts for this error: standalone function call and piping.
The problem that the solution hint for the error is different depending on context:

  • Provide all arguments for function call
  • Use the closure syntax to adapt the function call to the required number of arguments

Idea 1 - Split the error in two

Pros:

  • It is fewer words to read
  • Focused on the right solution for the usage context

Cons:

  • Complicates the compiler implementation
  • Reverse of the focused - it probably useful to see the (only) 2 ways of solving such problems in general

Idea 2 - Keep a single error but provide 2 possible context+solutions

Pros:

  • Complete info package
  • Just change the error message

Cons:

  • Too long to read (is it really a problem ???)
  • Requires from the user to select appropriate context (hey, there are only 2)

Ok. I am biased :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we still have 2 different contexts for this error: standalone function call and piping.
[...]
Use the closure syntax to adapt the function call to the required number of arguments

Ah, so the current advice of "Use a closure to make partial application explicit" is not about pipelines; it's intended to be a message for people who expect functions to be curried.

In other words, in curried languages I can do List.map nums (Num.add 1) and Num.add would be partially applied with 1, but in Roc I would need to "Use the closure syntax to adapt the function call to the required number of arguments" - in other words, write it as List.map nums \num -> Num.add 1 num instead of List.map nums (Num.add 1) (which would produce an error like this today).

Does that make sense?

Copy link

Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked Closed are not deleted, so no matter what, the PR will still be right here in the repo. You can always access it and reopen it anytime you like!

@JRI98
Copy link
Contributor

JRI98 commented Nov 23, 2024

This PR has no changes. Maybe close?

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

Successfully merging this pull request may close these issues.

3 participants