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

Improve illegal pattern error for accidental map associations #8555

Conversation

jchristgit
Copy link
Contributor

Given a module such as this:

-module(test).
-export([pattern/1]).

pattern(#{foo => bar}) -> ok.

We presently report an error as:

test.erl:4:15: illegal pattern
%    4| pattern(#{foo => bar}) -> ok.
%     |               ^

Since we have information in erl_lint on the bad pattern being a map field update, we can be a bit more helpful by extending the error with a hint instructing the user that := is most likely wanted instead:

test.erl:4:15: illegal pattern, did you mean to use `:=`?
%    4| pattern(#{foo => bar}) -> ok.
%     |               ^

Copy link
Contributor

github-actions bot commented Jun 9, 2024

CT Test Results

    2 files     95 suites   36m 29s ⏱️
2 142 tests 2 094 ✅ 48 💤 0 ❌
2 451 runs  2 401 ✅ 50 💤 0 ❌

Results for commit 6db7d96.

♻️ This comment has been updated with latest results.

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

// Erlang/OTP Github Action Bot

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Jun 10, 2024
@kikofernandez
Copy link
Contributor

Hi! @jchristgit this is a good improvement. Given that you have previously suggested new improvements (#7383), do you have more ideas to add to this PR for better error messages?
Just checking if there are more places that could benefits from your suggestions :)

@kikofernandez
Copy link
Contributor

BTW, can you rebase this on maint, so that changes these improvements are available on the next patch of OTP 27?

Given a module such as this:

    -module(test).
    -export([pattern/1]).

    pattern(#{foo => bar}) -> ok.

We presently report an error as:

    test.erl:4:15: illegal pattern
    %    4| pattern(#{foo => bar}) -> ok.
    %     |               ^

Since we have information in `erl_lint` on the bad pattern being a map
field update, we can be a bit more helpful by extending the error with a
hint instructing the user that `:=` is most likely wanted instead:

    test.erl:4:15: illegal pattern, did you mean to use `:=`?
    %    4| pattern(#{foo => bar}) -> ok.
    %     |               ^
@jchristgit jchristgit force-pushed the improve-illegal-pattern-error-for-map-problems branch from 22e7e15 to 6db7d96 Compare June 10, 2024 15:48
@jchristgit jchristgit changed the base branch from master to maint June 10, 2024 15:49
@jchristgit
Copy link
Contributor Author

@kikofernandez thanks for the feedback!
I usually try to make PRs for things I catch on the fly - the last PR was from my own confusion, this one from a user of a library I maintain :-) I had a quick look around an "illegal expression" stood out but I'm unsure if 1. the illegal expression is always of the form atom:atom (as seen in the test case) 2. we should assume that an existing function is being called and suggest to call it. I've also never encountered it personally before, but in case somebody expects that functions can be called directly (without ()) it might help. What do you think?
I also rebased the branch on maint.

@kikofernandez kikofernandez merged commit 56ca964 into erlang:maint Jun 14, 2024
16 checks passed
@kikofernandez
Copy link
Contributor

I think we should let that be for now. Thanks!

@jchristgit jchristgit deleted the improve-illegal-pattern-error-for-map-problems branch June 14, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants