Update Erlang warnings translation #10694

wojtekmach · 2021-02-08T16:48:45Z

Since erlang/otp@8ecc648 instead of {eval_failure, Reason} we'd get {eval_failure, Call, Reason}.

Since erlang/otp@8ecc648 instead of `{eval_failure, Reason}` we'd get `{eval_failure, Call, Reason}`

ericmj · 2021-02-08T17:32:05Z

Should we format and display Call as the operator / function call that will fail?

wojtekmach · 2021-02-08T17:43:24Z

@ericmj definitely, how would you display it?

today we have:

warning: this expression will fail with ArgumentError
warning: this expression will fail with ArithmeticError

we can mirror the origin erlang error, how about:

warning: this call to atom_to_binary/2 will fail with ArgumentError
warning: this call to +/2 will fail with ArithmeticError

ericmj · 2021-02-08T17:47:31Z

we can mirror the origin erlang error, how about:

That looks great, I would only change "this" to "the".

ericmj · 2021-02-08T21:37:23Z

lib/elixir/src/elixir_erl_compiler.erl

  "this check/guard will always yield the same result";

 %% Handle literal eval failures
+custom_format(sys_core_fold, {eval_failure, {_, {Name, Arity}}, Error}) ->


This will unfortunately show functions according to Erlang operators, import rules, and function names.

You need to match on:

{operator, {Name, Arity}}

{function, {Name, Arity}}

{function, {Module, Name, Arity}}

And then translate it back to Elixir operators and function names. You can use elixir_rewrite:erl_to_ex/3 for this but I have also asked for this to be simplified so we don't have to work around Erlang's rules: erlang/otp@8ecc648#r46899624.

make sense, instead of {operator, {Name, Arity}}, erl compiler would return {Module, Name, Arity}, right?

In fact, I think the compiler should return {Module, Name, Args} (not Arity) so that we can pass it to elixir_rewrite:erl_to_ex/3.

I've created a PR erlang/otp#3051 so that the compiler emits {eval_failure, {Mod, Name, Arity}}. It emits Arity instead of Args because of possible performance issues.

wojtekmach · 2021-02-10T08:41:26Z

I've updated the branch per erlang/otp#3051, the next step is to improve the warnings further.

wojtekmach · 2021-02-10T13:47:17Z

Currently we have this:

defmodule Foo do
  def foo, do: Atom.to_string("abc")
end

# Outputs:
# warning: the call to :erlang.atom_to_binary/2 will fail with ArgumentError
#   lib/foo.ex:2

The idea is to output:

# warning: the call to Atom.to_string/1 will fail with ArgumentError
#   lib/foo.ex:2

but if we always do this translation we'd have:

defmodule Foo do
  def foo, do: :erlang.atom_to_binary("abc")
end

# Outputs:
# warning: the call to Atom.to_string/1 will fail with ArgumentError
#   lib/foo.ex:2

which would be incorrect. But I guess since the vast majority of time Elixir devs would use Elixir API the benefit outweighs this downside?

josevalim · 2021-02-10T13:49:58Z

Yes, there will be false positives always so i’d rather err on the Elixir side. Alternatively, show both (with the Erlang one in parens)

ericmj · 2021-02-10T15:25:02Z

We have false positives in other areas, so I think it's fine to always print the Elixir version.

fertapric · 2021-02-10T15:46:37Z

Alternatively, show both (with the Erlang one in parens)

My vote goes for this one ☝️ warning: the call to Atom.to_string/1 (or :erlang.atom_to_binary/2) will fail with ArgumentError

wojtekmach · 2021-02-10T16:12:47Z

Updated, we now have:

defmodule Foo do
  def f1, do: 1 + nil

  def f2, do: Integer.to_string("bad")

  def f3, do: Atom.to_string("bad")
end

warning: the call to +/2 will fail with ArithmeticError
  lib/foo.ex:2

warning: the call to Integer.to_string/1 will fail with ArgumentError
  lib/foo.ex:4

warning: the call to :erlang.atom_to_binary/2 will fail with ArgumentError
  lib/foo.ex:6

The 3rd one will be improved when we do #9603

Rewrite Atom.to_string/1 to :erlang.atom_to_binary/1

please lmk if we're adding the (or m.f/a) that @fertapric mentioned, happy to do that as well!

josevalim · 2021-02-10T18:11:13Z

lib/elixir/src/elixir_erl_compiler.erl

+  Call = 'Elixir.Exception':format_mfa(ExMod, ExName, length(ExArgs)),
+  Trimmed = case Call of
+              <<"Kernel.", Rest/binary>> -> Rest;
+              _ -> Call
+            end,


Check ExMod and call Exception.format_mfa/3 if not Kernel and then Exception.format_fa/2 if Kernel.

Actually, ignore me. This is good.

wojtekmach · 2021-02-11T12:43:14Z

The PR landed upstream, please rerun tests and they should be green now!

ericmj

Looks good. Let's wait until tomorrow for the new build of master to be available so we can run CI.

ericmj · 2021-02-12T10:51:15Z

OTP master is failing but it's unrelated to the changes in this PR it seems. This should be good to merge.

Thanks @wojtekmach!

josevalim · 2021-02-12T11:24:03Z

💚 💙 💜 💛 ❤️

josevalim · 2021-02-12T11:24:31Z

The other failures also seem to be related to improved error messages on latest OTP, we just need to adapt accordingly. :)

wojtekmach · 2021-02-12T11:26:49Z

Im happy to look into these as well!

Since erlang/otp@8ecc648 instead of `{eval_failure, Reason}` we'd get `{eval_failure, Call, Reason}`

* Update Erlang warnings translation (#10694) Since erlang/otp@8ecc648 instead of `{eval_failure, Reason}` we'd get `{eval_failure, Call, Reason}` * Fix translating erl compiler errors to diagnostics on OTP 24 (#10719) The compiler previously emitted just `line` and now emits `{line, column}`. The diagnostic struct accepts as position either: nil line {start_line, start_col, end_line, end_col} so we could have used the `column` to create the 4-tuple but we don't have enough information to do that correctly, we don't know where the line ends. * Fix remaining warning translations for OTP 24 (#10720) Yecc error message changed so we need to loosen up our assertion: OTP 23 iex> iex Erlang/OTP 23 [erts-11.1.7] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] Interactive Elixir (1.11.3) - press Ctrl+C to exit (type h() ENTER for help) iex(1)> :yecc.file('f.yrl') f.yrl:1: syntax error before: '.' :error OTP 24 iex> iex Erlang/OTP 24 [DEVELOPMENT] [erts-11.1.7] [source-802d2c5083] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] Interactive Elixir (1.12.0-dev) - press Ctrl+C to exit (type h() ENTER for help) iex(1)> :yecc.file('f.yrl') f.yrl:1:5: syntax error before: . :error * Support 16bit floats in bitstrings (#10740) On OTP 24: iex> <<x::float-16>> = <<60, 0>> iex> x 1.0 iex> <<x::float-16>> <<60, 0>> Before OTP 24 we'd get errors or wouldn't match: iex> <<1.0::float-16>> ** (ArgumentError) argument error while evaluating iex at line 1 <<x::float-16>> = <<60, 0>> ** (MatchError) no match of right hand side value: <<60, 0>> iex> (fn <<x::float-16>> -> x; _ -> :nomatch end).(<<60, 0>>) :nomatch * Handle :compile.file/2 returning :error on OTP 24 (#10742) Before OTP 24 when given invalid args, :compile.file/2 would return: iex(1)> :compile.file('a.erl', [{:d, 'foo', 'bar'}, :report]) {:error, :badarg} On OTP 24: iex(1)> :compile.file('a.erl', [{:d, 'foo', 'bar'}, :report]) *** Internal compiler error *** exception error: bad argument in function io_lib:format/2 called as io_lib:format("badly formed '~s'",[{"foo","bar"}]) in call from sys_messages:list_errors/3 (sys_messages.erl, line 53) in call from lists:foreach/2 (lists.erl, line 1342) in call from compile:comp_ret_err/1 (compile.erl, line 546) in call from compile:'-internal_fun/2-anonymous-0-'/2 (compile.erl, line 229) in call from compile:'-do_compile/2-anonymous-0-'/1 (compile.erl, line 219) :error

Update Erlang warnings translation

bac3e36

Since erlang/otp@8ecc648 instead of `{eval_failure, Reason}` we'd get `{eval_failure, Call, Reason}`

Improve error message

fd2b45d

ericmj reviewed Feb 8, 2021

View reviewed changes

Update warning

fc355dc

Rewrite messages

71aaf91

josevalim reviewed Feb 10, 2021

View reviewed changes

ericmj approved these changes Feb 11, 2021

View reviewed changes

josevalim merged commit 6437a1a into elixir-lang:master Feb 12, 2021

wojtekmach added a commit to wojtekmach/elixir that referenced this pull request Feb 20, 2021

Update Erlang warnings translation (elixir-lang#10694)

23c9595

Since erlang/otp@8ecc648 instead of `{eval_failure, Reason}` we'd get `{eval_failure, Call, Reason}`

wojtekmach mentioned this pull request Feb 20, 2021

Backport OTP 24 compatibility patches to v1.11 #10743

Merged

wojtekmach deleted the wm-fix-warnings branch February 20, 2021 12:56

Update Erlang warnings translation #10694

Update Erlang warnings translation #10694

Uh oh!

Conversation

wojtekmach commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericmj commented Feb 8, 2021

Uh oh!

wojtekmach commented Feb 8, 2021

Uh oh!

ericmj commented Feb 8, 2021

Uh oh!

ericmj Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

wojtekmach Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

wojtekmach Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

wojtekmach commented Feb 10, 2021

Uh oh!

wojtekmach commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josevalim commented Feb 10, 2021

Uh oh!

ericmj commented Feb 10, 2021

Uh oh!

fertapric commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wojtekmach commented Feb 10, 2021

Uh oh!

josevalim Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

josevalim Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

wojtekmach commented Feb 11, 2021

Uh oh!

ericmj left a comment

Choose a reason for hiding this comment

Uh oh!

ericmj commented Feb 12, 2021

Uh oh!

josevalim commented Feb 12, 2021

Uh oh!

josevalim commented Feb 12, 2021

Uh oh!

wojtekmach commented Feb 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

wojtekmach commented Feb 8, 2021 •

edited

Loading

wojtekmach commented Feb 10, 2021 •

edited

Loading

fertapric commented Feb 10, 2021 •

edited

Loading