Expand symbolic funs correctly

[richcarl]

If a fun f/n used the same name and arity f/n as a BIF erlang:f/n, for example max/2, the symbolic fun would get unnecessarily rewritten as fun (X1, ..., Xn) -> f(X1, ..., Xn) end.

In addition, the rewritten fun body was then processed as a normal call, so if max/2 was declared as an import from some module m, it got expanded to a call to m:max/2, making imports "work" for symbolic funs but only if their names look like BIF names in order to sneak past the linter (albeit with a warning).

This change makes the expander do the right thing for all imports, auto or explicit, creating symbolic remote funs, and leaves to the linter to decide what is allowed. In the linter, it avoids marking f/n as a used local if it is listed as an import, instead reporting an error if you have a fun f/n that refers to an explicit import, and furthermore checks the reference f/n just like a call f(X1, ... Xn) so that we get the same warnings for deprecated functions etc., for example for fun now/0.

If desired, one could simply drop the check for imports in fun f/n, making the behaviour uniform whether the function is auto-imported or explicitly imported. That rule seems rather arbitrary.

[github-actions]

CT Test Results

    4 files    448 suites   1h 25m 50s ⏱️
3 238 tests 3 192 ✅ 46 💤 0 ❌
8 469 runs  8 420 ✅ 49 💤 0 ❌

Results for commit e88ee26.

♻️ 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

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[bjorng]

If I update the primary bootstrap, the build fails:

erl_types.erl:579:20: ambiguous call of overridden pre Erlang/OTP R14 auto-imported BIF is_boolean/1 --
use erlang:is_boolean/1 or \"-compile({no_auto_import,[is_boolean/1]}).\" to resolve name clash
%  579|   structural(Type, fun is_boolean/1).
%     |                    ^

erl_types.erl:612:22: ambiguous call of overridden pre Erlang/OTP R14 auto-imported BIF is_binary/1 --
use erlang:is_binary/1 or \"-compile({no_auto_import,[is_binary/1]}).\" to resolve name clash
%  612|     structural(Type, fun is_binary/1).
%     |                      ^

erl_types.erl:821:20: ambiguous call of overridden pre Erlang/OTP R14 auto-imported BIF is_number/1 --
use erlang:is_number/1 or \"-compile({no_auto_import,[is_number/1]}).\" to resolve name clash
%  821|   structural(Type, fun is_number/1).
%     |                    ^

(If I don't update the primary bootstrap, Dialyzer fails instead: https://github.com/erlang/otp/actions/runs/12495954151/job/34867261463?pr=9246)

[richcarl]

erl_types.erl:579:20: ambiguous call of overridden pre Erlang/OTP R14 auto-imported BIF is_boolean/1 --
use erlang:is_boolean/1 or "-compile({no_auto_import,[is_boolean/1]})." to resolve name clash
% 579| structural(Type, fun is_boolean/1).

I see. It's the check I added:

furthermore checks the reference f/n just like a call f(X1, ... Xn) so that we get the same warnings for deprecated functions etc.

Since funs have never been checked for this before (which should be considered a bug), I'm not quite sure what to do here. Making existing code not compile, like the erl_types module in dialyzer, is obviously not nice, but if it's a compiler bug the code shouldn't have been legal to begin with. I.e., erl_types defines and exports a local function is_boolean(X) -> ..., and if there had been a local call B = is_boolean(X) in erl_types without declaring -compile({no_auto_import,[is_boolean/1]})., it would have been an error. But currently the only use is as fun is_boolean/1, which was not checked.

Specifically, these things only become errors for functions f/a which are "old BIFs" (pre-R14) according to erl_internal:old_bif/2. For other auto-imports it's just a warning. It's not clear to me if these old-bif checks still serve a purpose.

[bjorng]

It's not clear to me if these old-bif checks still serve a purpose.

We just had a long discussion and decided that they no longer serve any useful purpose. That is, there is no longer any need to check whether the BIF is old (and erl_internal:old_bif/2 can be removed), and the same warning is generated for all BIFs. Feel free to do that change.

[bjorng]

The test case error_SUITE:bif_clashes/1 for the compiler fails:

4:18: ambiguous call of overridden auto-imported BIF length/1 --
use erlang:length/1 or "-compile({no_auto_import,[length/1]})." to resolve name clash
                 length([a,b,c]).
                 ^

Test bif_clashes1 failed. Expected
  {error,[{{4,18},erl_lint,{call_to_redefined_old_bif,{length,1}}}],[]}
but got
  {warning,[{{4,18},erl_lint,{call_to_redefined_bif,{length,1}}}]}

Otherwise it looks good to me.

[bjorng]

Could you do some squashing of the commits to avoid problems if we'll need to do a git bisect? At least I think that the last two commits should be combined, and that the dialyzer commit should be combined with the previous commit.

[richcarl]

Squashed.

[bjorng]

Thanks for your pull request.