Quote source as part of error messages #3020

richcarl · 2021-02-01T12:53:56Z

Provides one or more additional lines of context for compiler messages, showing the relevant source code with the error position indicated by ^, like this:

foo.erl:13:10: variable 'X' is unbound
%   13|     case X of
%     |          ^

foo.erl:18:16: illegal guard expression
%   18| foo(A, B) when is_lit(A), is_list(B) ->
%     |                ^

foo.erl:23:22: function bar/1 undefined
%   23|             {ok, {H, bar(T)}}
%     |                      ^

foo.erl:27:5: record foo undefined
%   26| bar(X, Y) ->
%   27|     #foo{a = baz(X, Y),
%     |     ^
%   28|          b = f(),
%     | ...
%   31|          y = false,
%   32|          z = true}.

These long messages can be suppressed with the 'brief' compiler option.
The message formatting code is moved out to a separate module.

The followup PR #3026 also adds end position to the parser, allowing the error ranges to be underlined.

mheiber · 2021-02-01T14:22:52Z

I'm so excited for this change, I think it will be a huge devX improvement 🎉.

Some small bugs with +deterministic:

With +deterministic, when there is a file with the same basename as the source file in the current directory, lines from the file in the current directory are displayed instead of in the source file. You can see this with an example.erl in .., then yes > example.erl CTRL-C then erlc +deterministic ../example.erl. Any errors in the source file show up with the quoted source line as yes.
With +deterministic, when there is no file with the same name in the current directory, there are no error lines displayed. We hit the error-swallowing case here.

The root cause seems to be that SourceName has two incompatible meanings:

thing used for display, which can be shortened
thing used for reading from the fs, which must be accurate

mheiber · 2021-02-01T14:23:54Z

lib/compiler/src/compile_messages.erl

+quote_source_1(File, Loc1, Line) when is_integer(Line) ->
+    quote_source_1(File, Loc1, {Line, -1});
+quote_source_1(File, {StartLine, StartCol}, {EndLine, EndCol}) ->
+    case file:read_file(File) of


It looks like we are re-reading the source file for each error line. Is there a way to do fewer reads?

Absolutely, this was a simplistic implementation. Considering OS file caches, it's probably not too bad as it is even for a hundred or more errors, but you could implement a caching system (which would have to handle include files as well, not just the main source file).

Seems to be fast enough when compiling the compiler test suite (which has many warnings). In real code, there is probably very few warnings, as many projects use -Werror nowadays.

richcarl · 2021-02-02T09:47:36Z

To make things easier for upstream adoption, I am splitting this into two separate parts. This part now only adds the messages and points out the start position, so it does not affect the parser annotations.

bjorng

compile_SUITE:app_test/1 fails because you have not included compile_messages in the app file.

Found two minor style issues.

Otherwise, looks good to me.

lib/compiler/src/compile_messages.erl

richcarl · 2021-02-03T07:59:37Z

Otherwise, looks good to me.

Any suggestion for a different module name than "compile_messages", or is it ok for you?

garazdawi · 2021-02-03T08:06:16Z

Should there be tests for this? or are the existing tests sufficient?

ranjanified · 2021-02-03T08:25:25Z

Otherwise, looks good to me.

Any suggestion for a different module name than "compile_messages", or is it ok for you?

If you can make it sound like a "object", that will be good. Modules probably are well-suited with a notion of a "subject". "compile_messages" sound more like a verb. Just my thoughts. Honestly haven't gone through the details of the PR, so excuse me for being shallow.

richcarl · 2021-02-03T08:36:30Z

"compile_messages" sound more like a verb.

That's basically why I'm asking (and also because someone out there might be using this name for something else already). The main point with the current name is that it indicates its relation to the "compile" module.

bjorng · 2021-02-03T09:19:22Z

Any suggestion for a different module name than "compile_messages", or is it ok for you?

Traditionally, Robert and Joe use sys_ as a prefix for compiler modules, and it is still used for some compiler modules.

So, perhaps, sys_messages.

richcarl · 2021-02-03T09:53:31Z

Fixed style issues and renamed to sys_messages.

richcarl · 2021-02-03T10:53:57Z

Tried rebasing on master as well to trigger another rebuild, but it's still failing. Docker trouble?

bjorng · 2021-02-03T12:45:37Z

Tried rebasing on master as well to trigger another rebuild, but it's still failing. Docker trouble?

Yes, seems to be. It builds fine on our Jenkins server, though.

bjorng · 2021-02-04T05:36:32Z

There are failed test cases in erlc_SUITE (system) and escript_SUITE (stdlib).

Consider this module: -module(w). -export([ll/0, sum/0, tt/0, ignored/2]). ll() -> L = {a,b,c}, length(L). sum() -> A = a, B = b, A + B. tt() -> (try nan after whatever end) + 42. ignored(X, Y) -> X + Y, ok. When compiling this module using erlang#3020, the following warnings are emitted: w.erl:6:5: Warning: this expression will fail with a 'badarg' exception % 6| length(L). % | ^ w.erl:11:7: Warning: this expression will fail with a 'badarith' exception % 11| A + B. % | ^ w.erl:14:34: Warning: this expression will fail with a 'badarith' exception % 14| (try nan after whatever end) + 42. % | ^ w.erl:17:7: Warning: the result of the expression is ignored (suppress the warning by assigning the expression to the _ variable) % 17| X + Y, % | ^ The last three warnings are slightly confusing, because the warnings say "this expression" or "the expression", but the column position points out the `+` operator in the middle of the expresson instead of the beginning of the expression. For the second warning, it would be better to point out the variable `A`: w.erl:11:5: Warning: this expression will fail with a 'badarith' exception % 11| A + B. % | ^ However, to produce that warning would need a major rethinking of the compiler pass, `sys_core_fold`, that produces that kind of warning. `sys_core_fold` is an optimization pass that opportunistically emits warnings while optimizing the code. When the warning for line 11 is emitted, the original code has been lost and the line and column number for the variable `A` is no longer available. The third warning does not seem possible to fix by moving the position of the caret: w.erl:14:34: Warning: this expression will fail with a 'badarith' exception % 14| (try nan after whatever end) + 42. % | ^ Pointing to the atom `nan` would be confusing. Considering that the second warning would need a lot of work and that there is no good solution for the third warning, I instead chose to rephrase the warnings like this: w.erl:6:5: Warning: the call to length/1 will fail with a 'badarg' exception % 6| length(L). % | ^ w.erl:11:7: Warning: evaluation of operator '+'/2 will fail with a 'badarith' exception % 11| A + B. % | ^ w.erl:14:34: Warning: evaluation of operator '+'/2 will fail with a 'badarith' exception % 14| (try nan after whatever end) + 42. % | ^ w.erl:17:7: Warning: the result of an expression is ignored (suppress the warning by assigning the expression to the _ variable) % 17| X + Y, % | ^ I did not come up with a great solution for the fourth warning, so I only changed "the expression" to "an expression".

Consider this module: -module(w). -export([ll/0, sum/0, tt/0, ignored/2]). ll() -> L = {a,b,c}, length(L). sum() -> A = a, B = b, A + B. tt() -> (try nan after whatever end) + 42. ignored(X, Y) -> X + Y, ok. When compiling this module using erlang#3020, the following warnings are emitted: w.erl:6:5: Warning: this expression will fail with a 'badarg' exception % 6| length(L). % | ^ w.erl:11:7: Warning: this expression will fail with a 'badarith' exception % 11| A + B. % | ^ w.erl:14:34: Warning: this expression will fail with a 'badarith' exception % 14| (try nan after whatever end) + 42. % | ^ w.erl:17:7: Warning: the result of the expression is ignored (suppress the warning by assigning the expression to the _ variable) % 17| X + Y, % | ^ The last three warnings are slightly confusing, because the warnings say "this expression" or "the expression", but the column position points out the `+` operator in the middle of the expresson instead of the beginning of the expression. For the second warning, it would be better to point out the variable `A`: w.erl:11:5: Warning: this expression will fail with a 'badarith' exception % 11| A + B. % | ^ However, to produce that warning would need a major rethinking of the compiler pass, `sys_core_fold`, that produces that kind of warning. `sys_core_fold` is an optimization pass that opportunistically emits warnings while optimizing the code. When the warning for line 11 is emitted, the original code has been lost and the line and column number for the variable `A` is no longer available. The third warning does not seem possible to fix by moving the position of the caret: w.erl:14:34: Warning: this expression will fail with a 'badarith' exception % 14| (try nan after whatever end) + 42. % | ^ Pointing to the atom `nan` would be confusing. Considering that the second warning would need a lot of work and that there is no good solution for the third warning, I instead chose to rephrase the warnings like this: w.erl:6:5: Warning: the call to length/1 will fail with a 'badarg' exception % 6| length(L). % | ^ w.erl:11:7: Warning: evaluation of operator '+'/2 will fail with a 'badarith' exception % 11| A + B. % | ^ w.erl:14:34: Warning: evaluation of operator '+'/2 will fail with a 'badarith' exception % 14| (try nan after whatever end) + 42. % | ^ w.erl:17:7: Warning: the result of evaluating operator '+'/2 is ignored (suppress the warning by assigning the expression to the _ variable) % 17| X + Y, % | ^

garazdawi · 2021-02-04T10:24:01Z

foo() ->
    A = <<"a𐐀b"/utf16>>, <<"hello">>,
    A.

On my machine this prints:

t.erl:14:26: Warning: a term is constructed, but never used
%   14|     A = <<"a\x{10400}b"/utf16>>, <<"hello">>,
%     |                          ^

bjorng · 2021-02-04T12:38:16Z

@garazdawi The problem you found is a problem with erlc. I have known about it, but haven't got around to fix it. I might try to fix it for OTP 24.

If you compile your function with the erl shell, it will work:

1> c(t).
t.erl:5:26: Warning: a term is constructed, but never used
%    5|     A = <<"a𐐀b"/utf16>>, <<"hello">>,
%     |                          ^

There should be no need take any coding into account, because the column position is the number of graphemes. Here is another example:

bar() ->
    A = <<"привет"/utf8>>, <<"hello">>,
    B = <<"abcdef"/utf8>>, <<"hello">>,
    {A,B}.

The carets are placed in the correct place:

t.erl:9:28: Warning: a term is constructed, but never used
%    9|     A = <<"привет"/utf8>>, <<"hello">>,
%     |                            ^

t.erl:10:28: Warning: a term is constructed, but never used
%   10|     B = <<"abcdef"/utf8>>, <<"hello">>,
%     |                            ^

garazdawi · 2021-02-04T12:54:48Z

Great!

bjorng · 2021-02-04T13:20:28Z

@richcarl We found that int_SUITE:init_per_suite/1 in the debugger applications fails with a timetrap timeout on some of our test machines. That test case compiles a huge file with many warnings. On my Mac it needs 25 seconds. Some of our test machines needs more than one minute, which is the current timetrap limit.

Could you fix this in one of two ways?

Raise the timetrap limit, for example to ten minutes.

otp/lib/debugger/test/int_SUITE.erl

Line 57 in c28f301

{timetrap,{minutes,1}}].
Implement some sort of caching, for example by spawning off a caching process the first time an error or warning is printed.

richcarl · 2021-02-04T13:33:46Z

@richcarl We found that int_SUITE:init_per_suite/1 in the debugger applications fails with a timetrap timeout on some of our test machines. That test case compiles a huge file with many warnings. On my Mac it needs 25 seconds. Some of our test machines needs more than one minute, which is the current timetrap limit.

Bumped to 10 min. Caching can wait until later.

richcarl · 2021-02-04T18:17:33Z

Also, does this need to take %% coding: latin-1 into consideration?

Found that the underlining was too naive, and could get out of sync with tabs in combination with utf-8 multibytes. Now treats lines as sequences of codepoints, respecting the file encoding. This should always correspond to the column number computed by the scanner. (Forced edit: handle broken encodings in a sensible way instead of crashing.)

bjorng · 2021-02-05T05:06:15Z

Found that the underlining was too naive, and could get out of sync with tabs in combination with utf-8 multibytes. Now treats lines as sequences of codepoints, respecting the file encoding. This should always correspond to the column number computed by the scanner. (Forced edit: handle broken encodings in a sensible way instead of crashing.)

Do you think you could add a test case to test those things you just fixed? To avoid have to constantly update that test case, it would probably be best to call sys_messages directly with a faked list of errors and warnings so that the output will not change if other parts of the compiler are updated.

richcarl · 2021-02-05T13:54:16Z

Do you think you could add a test case to test those things you just fixed?

Added test case. Running the full compiler to ensure that encodings are checked properly.

bjorng · 2021-02-08T07:31:52Z

There are still two failing test cases in erlc_SUITE (make_dep_options).

richcarl · 2021-02-08T08:46:07Z

See if you like this solution better before I squash. Also added a fix for the hardcoded paths preventing you from running erlc_SUITE directly from make under erts.

uabboli · 2021-02-08T08:48:28Z

The followup PR #3026 also adds end position to the parser, allowing
the error ranges to be underlined.

I think all traces of ranges (#3026) should be removed in this PR.
erl_lint should not be updated at all, and the code in sys_messages,
starting with

StartLoc = erl_anno:location(Loc).

should not try to be forward compatible with ranges. It breaks the
erl_anno abstraction (the current code will crash if the erl_anno
module is compiled with DEBUG=true).

richcarl · 2021-02-08T09:52:59Z

I think all traces of ranges (#3026) should be removed in this PR.
erl_lint should not be updated at all, and the code in sys_messages,
starting with

StartLoc = erl_anno:location(Loc).

should not try to be forward compatible with ranges. It breaks the
erl_anno abstraction (the current code will crash if the erl_anno
module is compiled with DEBUG=true).

I agree, so I'll try to remove stuff that shouldn't have been in this part to begin with.

Something is weird with the types, though. in compile.erl, a comp_ret(), which is the return type for compile:file() and compile:forms(), is defined as reporting errors and warnings that contain err_info() :: {erl_anno:line() | 'none', module(), term()}. Note that it says line(), which is defined in erl_anno as non_neg_integer(), and this should be changed to location() to reflect the new support for column numbers, which might fix the DEBUG compilation problem.

Meanwhile, internally in compile.erl the errors and warnings fields of the #compile{} record use the type err_warn_info() which is just defined as tuple(). The messages_per_file() function takes the internal err_warn_info() representation and produces the err_info() version.

I don't really want to start on a type annotation whack-a-mole to try to clean up that stuff myself, since it might cause some cascading type issues, at least not until I know what you think how it should look.

bjorng · 2021-02-08T10:57:36Z

See if you like this solution better before I squash. Also added a fix for the hardcoded paths preventing you from running erlc_SUITE directly from make under erts.

Looks good to me, and the test cases now run clean.

bjorng · 2021-02-08T11:54:39Z

I think you can squash it now. (I would prefer down to a single commit.) If everything goes well in our nightly tests, I intend to merge this PR tomorrow.

uabboli · 2021-02-08T13:24:59Z

I think all traces of ranges (#3026) should be removed in this PR.
erl_lint should not be updated at all, and the code in sys_messages,
starting with

I see that you've reverted erl_lint. Looks OK.

StartLoc = erl_anno:location(Loc).

should not try to be forward compatible with ranges. It breaks the
erl_anno abstraction (the current code will crash if the erl_anno
module is compiled with DEBUG=true).

You've haven't fixed this, as far as I can see.

Note that it says line(), which is defined in erl_anno as
non_neg_integer(), and this should be changed to location() to reflect
the new support for column numbers, which might fix the DEBUG
compilation problem.

No, the erl_anno-abstraction is broken in a more fundamental way.

The quoted line,

StartLoc = erl_anno:location(Loc).

breaks the erl_anno abstraction.

format_messages(F, P, [{Loc, Mod, E} | Es], Opts) ->
    Src = quote_source(F, Loc, Loc, Opts),

should suffice.

lists_errors also breaks the erl_anno-abstraction:

list_errors(F, [{StartLoc, Mod, E} | Es], Opts) ->
    list_errors(F, [{{StartLoc, StartLoc}, Mod, E} | Es], Opts);

I suppose you can keep the EndLoc in quote_source as long it's
correct, although it's not needed in this PR.

richcarl · 2021-02-08T13:43:46Z

Squashed. Working on a cleanup of type usages, but I can put that in a separate PR after this is merged.

richcarl · 2021-02-08T15:02:12Z

@uabboli Check out #3045 for a patch that tries to unify the type annotations for errors/warnings over the compiler modules.

Can be suppressed with the 'brief' compiler option. Moves message formatting code to a separate module.

richcarl · 2021-02-08T16:25:36Z

Accidentally deleted a clause that was actually used. Also finally saw what @uabboli meant, so I removed the calls to erl_anno on a location type.

bjorng · 2021-02-08T16:46:41Z

Thanks, I can confirm that it works with erl_anno debugging activated (by uncommenting the defines for DEBUG in erl_anno, erl_parse, and erl_pp). Added to our daily builds.

bjorng · 2021-02-10T04:59:53Z

Thanks for your pull request.

mheiber reviewed Feb 1, 2021

View reviewed changes

rickard-green added the team:VM Assigned to OTP team VM label Feb 1, 2021

uabboli self-assigned this Feb 2, 2021

richcarl force-pushed the quote-source branch from 484beb9 to 5170eff Compare February 2, 2021 09:44

richcarl mentioned this pull request Feb 2, 2021

Add end_location annotations for parse trees #3026

Open

bjorng self-assigned this Feb 3, 2021

bjorng requested changes Feb 3, 2021

View reviewed changes

lib/compiler/src/compile_messages.erl Outdated Show resolved Hide resolved

lib/compiler/src/compile_messages.erl Outdated Show resolved Hide resolved

uabboli removed their assignment Feb 3, 2021

richcarl force-pushed the quote-source branch from 5170eff to deadadc Compare February 3, 2021 09:52

richcarl force-pushed the quote-source branch from deadadc to f0df03b Compare February 3, 2021 10:31

bjorng added enhancement testing currently being tested, tag is used by OTP internal CI labels Feb 3, 2021

bjorng approved these changes Feb 3, 2021

View reviewed changes

bjorng removed the testing currently being tested, tag is used by OTP internal CI label Feb 4, 2021

bjorng mentioned this pull request Feb 4, 2021

Reduce confusion by rephrasing some compiler warnings #3030

Merged

bjorng added the testing currently being tested, tag is used by OTP internal CI label Feb 4, 2021

richcarl force-pushed the quote-source branch from ce1ea49 to 7c79850 Compare February 4, 2021 18:13

richcarl force-pushed the quote-source branch from bd1675b to 9a7d62a Compare February 8, 2021 13:39

richcarl mentioned this pull request Feb 8, 2021

Error message types #3045

Merged

bjorng removed the testing currently being tested, tag is used by OTP internal CI label Feb 8, 2021

richcarl force-pushed the quote-source branch from 9a7d62a to 4451621 Compare February 8, 2021 16:17

Quote source as part of error messages

3253259

Can be suppressed with the 'brief' compiler option. Moves message formatting code to a separate module.

richcarl force-pushed the quote-source branch from 4451621 to 3253259 Compare February 8, 2021 16:24

bjorng added the testing currently being tested, tag is used by OTP internal CI label Feb 8, 2021

bjorng merged commit d3c315b into erlang:master Feb 10, 2021

richcarl deleted the quote-source branch February 10, 2021 08:19

Quote source as part of error messages #3020

Quote source as part of error messages #3020

Conversation

richcarl commented Feb 1, 2021 • edited Loading

mheiber commented Feb 1, 2021 • edited Loading

mheiber Feb 1, 2021

Choose a reason for hiding this comment

richcarl Feb 1, 2021

Choose a reason for hiding this comment

bjorng Feb 3, 2021

Choose a reason for hiding this comment

richcarl commented Feb 2, 2021

bjorng left a comment

Choose a reason for hiding this comment

richcarl commented Feb 3, 2021

garazdawi commented Feb 3, 2021

ranjanified commented Feb 3, 2021

richcarl commented Feb 3, 2021

bjorng commented Feb 3, 2021

richcarl commented Feb 3, 2021

richcarl commented Feb 3, 2021

bjorng commented Feb 3, 2021

bjorng commented Feb 4, 2021 • edited Loading

garazdawi commented Feb 4, 2021

bjorng commented Feb 4, 2021 • edited Loading

garazdawi commented Feb 4, 2021

bjorng commented Feb 4, 2021

richcarl commented Feb 4, 2021

richcarl commented Feb 4, 2021

bjorng commented Feb 5, 2021

richcarl commented Feb 5, 2021

bjorng commented Feb 8, 2021

richcarl commented Feb 8, 2021

uabboli commented Feb 8, 2021

richcarl commented Feb 8, 2021

bjorng commented Feb 8, 2021

bjorng commented Feb 8, 2021

uabboli commented Feb 8, 2021

richcarl commented Feb 8, 2021

richcarl commented Feb 8, 2021

richcarl commented Feb 8, 2021

bjorng commented Feb 8, 2021

bjorng commented Feb 10, 2021

richcarl commented Feb 1, 2021 •

edited

Loading

mheiber commented Feb 1, 2021 •

edited

Loading

bjorng commented Feb 4, 2021 •

edited

Loading

bjorng commented Feb 4, 2021 •

edited

Loading