erl_parse: Provide friendlier "head mismatch" errors

[jchristgit]

Given the following module:

    %% mismatch.erl
    -module(mismatch).
    -compile([export_all, nowarn_export_all]).

    test_it(ok) -> ok;

    verify_it(true) -> ok;
    verify_it(false) -> {error, was_false}.
    verify_it(false, Reason) -> {error, Reason}.

On master, running erlc mismatch.erl currently reports:

    mismatch.erl:6:1: head mismatch
    %    6| verify_it(true) -> ok;
    %     | ^

The previous clause that the parser is comparing to may not be obvious:
The current compiler warning points to verify_it/1, whose two clauses
are terminated properly. When more code is inbetween these two functions
(think documentation, specs, comments) the time needed to find the
mismatched clause can be a lot higher. I also believe that this will
make the error easier to understand for people new to Erlang.

With this commit, the compiler will output the following:

    mismatch.erl:6:1: head mismatch: previous function test_it/1 is distinct from verify_it/1. Is the semicolon in test_it/1 unwanted?
    %    6| verify_it(true) -> ok;
    %     | ^

For functions that are defined with the same name but distinct arities,
a separate error message is used, since the possibility of missed or
superfluous arguments also arises here. The following module illustrates
it:

    %% mismatch2.erl
    -module(mismatch2).
    -compile([export_all, nowarn_export_all]).

    verify_it(false) -> {error, was_false};
    verify_it(false, Reason) -> {error, Reason}.

The following error message is provided in this case:

    mismatch2.erl:6:1: head mismatch: function verify_it with arities 1 and 2 is regarded as two distinct functions. Are the number of arguments incorrect or is the semicolon in verify_it/1 unwanted?
    %    6| verify_it(false, Reason) -> {error, Reason}.
    %     | ^

For tools matching on the output of the compiler warnings, this is a
breaking change. I have so far only found Erlang LS which matches on
this 1. Adding support for the extended information should be trivial.

[github-actions]

CT Test Results

No tests were run for this PR. This is either because the build failed, or the PR is based on a branch without GH actions tests configured.

Results for commit d74e9c5

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• No HTML docs found
• No Windows Installer found

// Erlang/OTP Github Action Bot

[bjorng]

Thanks for your pull request.

I like the general idea. However, I think you'll need to provide a somewhat different message when the names of the function heads are the same but they have different arities. I often run into this error when I refactor code to remove or add function parameters. For example:

foo({A,B}) ->
    A + B;
foo(Other, State) ->
    {Other, State}.

The message will be:

t.erl:9:1: head mismatch: previous function foo/1 is distinct from foo/2. Is there an unwanted semicolon?
%    9| foo(Other, State) ->
%     | ^

Here the problem is not an unwanted semicolon, but that I have forgotten to add the State variable to the first clause.

Regarding compatibility, we don't promise compatibility of compiler messages between major releases of OTP.

[jchristgit]

Thank you for the feedback! I have updated my commit to include special handling for functions with the same name but different arity: ```erl %% mismatch2.erl -module(mismatch2). -compile([export_all, nowarn_export_all]). verify_it(false) -> {error, was_false}; verify_it(false, Reason) -> {error, Reason}. ``` will result in: ```erl mismatch2.erl:5:1: head mismatch: function verify_it with arities 1 and 2 is regarded as two distinct functions. Are arguments to verify_it/1 missing or is the semicolon unwanted? % 5| verify_it(false, Reason) -> {error, Reason}. % | ^ ``` Perhaps a bit verbose, but let me know what you think. Re: compatibility, good to know. I made a draft PR to Erlang LS to support the new message should it be merged.

[bjorng]

Yes, that's better, but it is still a little bit confusing if the intention in my refactoring was to remove one or more arguments. What about:

mismatch2.erl:5:1: head mismatch: function verify_it with arities 1 and 2 is regarded as two distinct functions. Are the number of arguments incorrect or is there an unwanted semicolon?

(I found "is the semicolon unwanted?" confusing, because it not clear to which semicolon you are referring. "is the semicolon in verify_it/1 unwanted?" is longer but is clear.)

[jchristgit]

I updated the commit to explicitly specify where the unwanted semicolon could be and provide a more generic message for a function with the same name but different arities. The new messages are:

    mismatch.erl:6:1: head mismatch: previous function test_it/1 is distinct from verify_it/1. Is the semicolon in test_it/1 unwanted?

and

    mismatch2.erl:6:1: head mismatch: function verify_it with arities 1 and 2 is regarded as two distinct functions. Are the number of arguments incorrect or is the semicolon in verify_it/1 unwanted?

[jchristgit]

Just realized I've been editing erl_parse.erl locally the entire time, which is generated ... and I was wondering why the tests didn't work in CI 🙂 they should be fixed now. I've also changed "Are the number of arguments [...]" to "Is the number of arguments"

[bjorng]

Excellent! 👍

Added to our daily builds.