Elixir: Improve no function clause error messages

[josevalim]

Today we say:

iex(1)> Regex.split "dsadsa", 321
** (FunctionClauseError) no function clause matching in Regex.split/3
    (elixir) lib/regex.ex:397: Regex.split("dsadsa", 321, [])

I believe we could improve this message. One proposal:

iex(1)> Regex.split "dsadsa", 321
** (FunctionClauseError) there is no definition of Regex.split/3 that expects the arguments at positions:

  1) "dsadsa"
  2) 321
  3) []

    (elixir) lib/regex.ex:397: Regex.split/3

The idea is to move the arguments themselves up to the error message and remove them from the stacktrace. I am not sure if the positions syntax above is good enough™ but it should be a good starting point for the discussion.

We could use asterisks instead of a numbered list as well:

iex(1)> Regex.split "dsadsa", 321
** (FunctionClauseError) there is no definition of Regex.split/3 that expects the following arguments:

  * "dsadsa"
  * 321
  * []

    (elixir) lib/regex.ex:397: Regex.split/3

Or repeat the function name:

iex(1)> Regex.split "dsadsa", 321
** (FunctionClauseError) there is no definition of Regex.split/3 that expects the following arguments:

  Regex.split("dsadsa", 321, [])

    (elixir) lib/regex.ex:397: Regex.split/3

Repeating the function name works well for small examples but it is problematic for large arguments.

This proposal is a small subset of #4171.

[dimitarvp]

I feel the last form is the best. It directly shows the full call that can't be matched. Makes it easier to parse when you're scanning a stack trace.

[josevalim]

@dimitarvp but how do you suggest us to handle the case where we have large arguments such a big map, multiple maps, large lists? Then it works the opposite, it makes it quite hard to parse.

[dimitarvp]

@josevalim Valid point. Is there a shortener for big variable values? I know IEx shortens maps on occasion (not really sure when exactly, I do remember seeing it putting "..." at the back of a map or large struct though).

[josevalim]

@dimitarvp we do that for stacktraces as well but it won't help on this case. As soon as any of the arguments spawns more than one line, which would not be uncommon, the last approach is going to break apart.

[dimitarvp]

@josevalim I agree but how is that going to be helped by the other approaches? If you have a map spanning 20 lines then using (1) (2) etc. (or asterisks) is gonna be lost to the reader pretty quickly as well.

Is it possible to infer the types (via the is_* functions I presume) and instead report the warning through them and not via the values?

[josevalim]

@dimitarvp the choice of using 1), 2), etc is to have something easier for the eyes to scan. It is easier than finding a comma in between large data structures. I don't think we should do anything specific with the type in here, we don't have this information on function clauses errors yet.

[josevalim]

To be a bit more specific. Compare:

** (FunctionClauseError) no function clause matching in Regex.split/3
    (elixir) lib/regex.ex:397: Regex.split([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42,
43, 44, 45, 46, 47, 48, 49, 50], [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18,
19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42,
43, 44, 45, 46, 47, 48, 49, 50], [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45,
46, 47, 48, 49, 50])

with

** (FunctionClauseError) there is no definition of Regex.split/3 that expects the arguments at positions:

  1) [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
      26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50]
  2) [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
      26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50]
  3) [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
      26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50]

    (elixir) lib/regex.ex:397: Regex.split/3

Those cases are common in many places, such as plug applications, ecto queries, etc.

[Eiji7]

@josevalim: I have a proposal related to this issue:

iex(1)> MyModule.my_method 123
** (FunctionClauseError) there is no definition of &MyModule.my_method/1.

Expected one of:
# all specs for all definitions here

Found:
my_method(integer)

with arguments at positions:

1) 123

# stacktrace here

I would like to compare specs with passed arguments types before I analyse their values. Imagine that we are passing charlist instead of String or Integer instead of Float and we could miss it, so specs could be helpful in that cases. What do you think about it?

[josevalim]

@Eiji7 this is the proposal linked on the issue above as #4171. It is a much larger effort, this is meant to be something simpler.

[dimitarvp]

@josevalim I understand. Your variant is better.

Plus, the first line ("there is no definition of...") is the most informative anyway and it gives you a better hint at what you're doing wrong (edit: clarify: compared to before, not compared to the lines that follow).

[josevalim]

@dimitarvp maybe we could mix and do this:

** (FunctionClauseError) there is no definition of Regex.split/3 that expects the arguments at positions:

  Regex.split(
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50],
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50],
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50]
  )

    (elixir) lib/regex.ex:397: Regex.split/3

But I am not sure if it is better or worse.

[dimitarvp]

@josevalim That actually looks really good, I like it.

[whatyouhide]

I prefer the * bulleted list over multiline call as in my experience multiline calls are quite rare in Elixir code (or at least, I've encountered them quite seldom). Also, If we show the function call like Regex.split(..., then we're trying to make the error resemble the code, but maybe in my code I wrote Regex.split(...) on one line and then the error message can feel a bit alien. But overall I agree that we need arguments on different lines, let's let them breathe :D

[michalmuskala]

The argument for multiline call is that you can easily copy it to IEx and try tweaking the arguments to see what's wrong. With bulleted list it's a bit harder.

[whatyouhide]

@michalmuskala good point. Small nitpick though: if I copy a multiline call to IEx to find out why I got a FunctionClauseError, then I probably want to modify the arguments to arrive to a non-erroring version, and I can't modify them since they're on different lines and I can't edit them in IEx once pasted. I'd have to up-arrow my way to rebuilding the multiline call. As I said, silly thing maybe ¯\_(ツ)_/¯

[josevalim]

@michalmuskala that's a very good point. Even if you can't change on IEx, the multiline approach is more friendly to copying to an editor and changing it.

[whatyouhide]

@josevalim right, editors. :)

[lexmag]

I like "1)" version, to me it is the easiest one to read, "multiline call" version looks clumsy and hard to follow (look how many commas 😵).
Speaking about copying, it's likely that those huge arguments are already defined in variables in IEx session (or as snippet in editor), "1)" version makes you easier to see what needs to be adjusted.

[OvermindDL1]

For such warnings, it would be nice to add in the typespecs in the error message if the function has one, it could be quite illuminating it what it would accept?

[josevalim]

@OvermindDL1 this has already been suggested and replied to above.

[OvermindDL1]

@OvermindDL1 this has already been suggested and replied to above.

Ah sorry, long thread and I searched for 'typespec' with no results after the first and last few messages read. ^.^

[kelvinst]

@lexmag maybe adding a comment on top of each param could make it easier to read?

** (FunctionClauseError) there is no definition of Regex.split/3 that expects the arguments at positions:

  Regex.split(
    # 1
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50],
    # 2
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50],
    # 3
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50]
  )

    (elixir) lib/regex.ex:397: Regex.split/3

Or maybe at the end of the line, IDK.

[lexmag]

@kelvinst unfortunately it still looks clumsy, and, perhaps, won't look nice with smaller arguments.
I think the main issue with "multiline call" version is that we're trying to make two things at the same time: show what were the call arguments and allow copying.

[michalmuskala]

So maybe the ideal solution would be to both show the bulleted arguments and the full call in the stacktrace, like we do today? One allows easily discerning between arguments, the other allows copying.

[josevalim]

I really like the comments idea. For small functions+args, we won't break
into new lines and we won't introduce the comments. It should be a

straight-forward case to handle.

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

[lexmag]

I'd like to propose the following format:

** (FunctionClauseError) there is no definition of Regex.split/3 that expects the arguments at positions:

    # 1
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50],
    # 2
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50],
    # 3
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50]

    (elixir) lib/regex.ex:397: Regex.split/3

It still allows copying, but we don't mention Regex.split, it is already in the message on first line and in the stacktrace.
_Kudos to @kelvinst for comments suggestion._

[whatyouhide]

IMHO, the bulleted/numbered list approach, while being not super-friendly to being copied, is still the only one that reads nicely and clearly. All the other solutions look a bit clumsy to me :(. And just to add my personal experience, I rarely copy this stuff out of errors, and if I do, removing some *s would take a pretty short amount of time given my average Elixir function takes 0-4 arguments. If we go with the copyable format though, I appreciate comments as well.

[josevalim]

I like @lexmag format above all because it is like the bulleted list, which was the original proposal, but it is also valid Elixir code. The problem with the bulleted list is that newcomers may confuse it with part of Elixir syntax but # 1 remove most of its ambiguity.

[ciaran]

I think it’s worth considering if we can show some type information for each argument in this error message, as in many cases this is a more important and shorter piece of information than the value itself.

E.g. "Regex.split(_binary_, _integer_, _list_)", with the real arguments detailed below as discussed.

[josevalim]

@ciaran it is the third time this has been suggested in this issue. Please read the existing discussion before jumping in as it saves us the effort from explaining something over and over again. :)

[ciaran]

I had just read through before I commented – I am not proposing the more complex option of trying to infer typespecs or guards etc. from function definitions, rather just showing the types of the arguments that were provided in this call and that we are discussing printing of.

Perhaps just this change even:

** (FunctionClauseError) there is no definition of Regex.split/3 that expects the arguments at positions:

    # 1 (list)
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50],
    # 2 (list)
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50],
    # 3 (list)
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50]

    (elixir) lib/regex.ex:397: Regex.split/3

[josevalim]

@ciaran oh, I see! Sorry for misunderstanding!

[whatyouhide]

The problem I can see with @ciaran's solution is that patterns are so much more complex than just types. We can show types, yeah, but in the pattern-matching world types are just one of the many shapes/attributes one can pattern match on, so it would be useful for some cases but may be completely useless for others.

[josevalim]

@whatyouhide right. It won't matter for cases like {:ok, atom} and you were expecting {:error, atom}.

[whatyouhide]

@josevalim exactly, and in those cases knowing that the argument I have/expected is "tuple" is just pointless 😕

[ciaran]

I’m not proposing we remove any information though, only that we add it, which I think won’t harm as we are including the actual value alongside it.

I think there are cases where seeing the type at a glance might help people who are confused by a clause like this, consider binary vs charlist for example.

[adkron]

Since we have the ability to color the output of the error could we do the copyable multiline proposal, but color each argument a different color? That seems like it would have the best of the bulleted list but still have the advantages of the multiline copy paste.

[josevalim]

@adkron we cannot use colors in error messages because they are shown in many mediums and not only in the terminal.

[whatyouhide]

I’m not proposing we remove any information though, only that we add it

I still am 👎 on that. Adding information is not necessarily a good thing just because we are not removing any information. I think the number of cases where adding the "type" (again, just one subset of things that can be pattern matched on) is small and not worth covering. :)

[bkudria]

Per @lexmag, temporary variables allow us to preserve evaluability, while allowing sensible multi-line presentation, like so:

** (FunctionClauseError) there is no definition of Regex.split/3 that expects the arguments at positions:

  list1 =
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48,
     49, 50]

  list2 =
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48,
     49, 50]

  list3 =
    [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
     26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48,
     49, 50]

  Regex.split(list1, list2, list3)

(Or even perhaps Regex.split(regex, string, options).

[josevalim]

Closing in favor of #6127.