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.

Sign up for GitHub

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

Jump to bottom

WIP better error msg #6751

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

WIP better error msg #6751

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
ea23576
feat: better error msg
dadhi May 14, 2024
2a17e49
Merge branch 'roc-lang:main' into main
dadhi Jun 6, 2024
33be4d1
Merge branch 'roc-lang:main' into main
dadhi Jun 10, 2024
3a4316d
Merge branch 'roc-lang:main' into main
dadhi Jun 22, 2024
0d680dc
Merge branch 'roc-lang:main' into main
dadhi Jul 23, 2024
f6b30a1
Revert "feat: better error msg"
dadhi Jul 23, 2024
5a6de63
Merge branch 'roc-lang:main' into main
dadhi Jul 24, 2024
fbaf992
Merge branch 'roc-lang:main' into main
dadhi Jul 29, 2024
17e15e7
Merge branch 'roc-lang:main' into main
dadhi Jul 30, 2024
76d839e
Merge branch 'roc-lang:main' into main
dadhi Jul 31, 2024
2b4aa97
Merge branch 'roc-lang:main' into main
dadhi Aug 6, 2024
dc18c14
Merge branch 'roc-lang:main' into main
dadhi Aug 12, 2024
6f2f0c2
Merge branch 'roc-lang:main' into main
dadhi Aug 13, 2024
cb1a59c
Merge branch 'roc-lang:main' into main
dadhi Aug 14, 2024
bda678f
Merge branch 'roc-lang:main' into main
dadhi Aug 16, 2024
d37156d
Merge branch 'roc-lang:main' into main
dadhi Aug 19, 2024
31b51bb
Merge branch 'roc-lang:main' into main
dadhi Aug 20, 2024
5f8a41f
Merge branch 'roc-lang:main' into main
dadhi Aug 21, 2024
f6526d3
Merge branch 'roc-lang:main' into main
dadhi Aug 25, 2024
0206f84
Merge branch 'roc-lang:main' into main
dadhi Sep 1, 2024
304cf5d
Merge branch 'roc-lang:main' into main
dadhi Sep 4, 2024
58f2f34
Merge branch 'roc-lang:main' into main
dadhi Sep 9, 2024
e0ae6d7
Merge branch 'roc-lang:main' into main
dadhi Sep 12, 2024
66a90b2
Merge branch 'roc-lang:main' into main
dadhi Sep 21, 2024
be3a580
Merge branch 'roc-lang:main' into main
dadhi Sep 22, 2024
c5644ae
Merge branch 'roc-lang:main' into main
dadhi Sep 24, 2024
5b1c60c
Merge branch 'roc-lang:main' into main
dadhi Sep 27, 2024
e05c544
Merge branch 'roc-lang:main' into main
dadhi Oct 1, 2024
972e218
Merge branch 'roc-lang:main' into main
dadhi Oct 7, 2024
78131d5
Merge branch 'roc-lang:main' into main
dadhi Oct 10, 2024
5f9a4fc
Merge branch 'roc-lang:main' into main
dadhi Oct 15, 2024
0d683ca
Merge branch 'roc-lang:main' into main
dadhi Oct 17, 2024
8bc83a8
Merge branch 'roc-lang:main' into main
dadhi Oct 22, 2024
dc47d1a
Merge branch 'roc-lang:main' into main
dadhi Oct 23, 2024
f227ea2
Merge branch 'roc-lang:main' into main
dadhi Oct 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions crates/compiler/load/tests/test_reporting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1665,7 +1665,7 @@ mod test_reporting {
f 1
"
),
@r"
@r#"
── TOO FEW ARGS in /code/proj/Main.roc ─────────────────────────────────────────

The `f` function expects 2 arguments, but it got only 1:
Expand All @@ -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?


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

test_report!(
Expand Down Expand Up @@ -14214,7 +14218,7 @@ In roc, functions are always written as a lambda, like{}
2 |> (Num.sub 3)
"
),
@r"
@r#"
── TOO FEW ARGS in /code/proj/Main.roc ─────────────────────────────────────────

The `sub` function expects 2 arguments, but it got only 1:
Expand All @@ -14224,7 +14228,11 @@ In roc, functions are always written as a lambda, like{}

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 ", "
"#
);

test_report!(
Expand All @@ -14234,7 +14242,7 @@ In roc, functions are always written as a lambda, like{}
2 |> (Num.sub 3) |> Num.sub 3
"
),
@r"
@r#"
── TOO FEW ARGS in /code/proj/Main.roc ─────────────────────────────────────────

The `sub` function expects 2 arguments, but it got only 1:
Expand All @@ -14244,6 +14252,10 @@ In roc, functions are always written as a lambda, like{}

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 ", "
"#
);
}
8 changes: 6 additions & 2 deletions crates/repl_test/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ fn too_few_args() {
expect_failure(
"Num.add 2",
indoc!(
r"
r#"
── TOO FEW ARGS ────────────────────────────────────────────────────────────────

The add function expects 2 arguments, but it got only 1:
Expand All @@ -652,7 +652,11 @@ fn too_few_args() {

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 ", "
"#
),
);
}
Expand Down
4 changes: 4 additions & 0 deletions crates/reporting/src/error/type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1237,6 +1237,10 @@ fn to_expr_report<'b>(
"Roc does not allow functions to be partially applied. \
Use a closure to make partial application explicit.",
),
alloc.reflow(
r#"For example: ["a", "b"] |> \list -> Str.joinWith list ", ""#,
),
alloc.reflow(r#"or simplier: ["a", "b"] |> Str.joinWith ", ""#),
];

Report {
Expand Down