JSON errors parsing enhancement #62

alygin · 2016-09-17T20:04:52Z

I've improved support for JSON error format and made it default.

All spans are now parsed and added to trace. No missing information anymore.
Label of the primary span is added to the main message. For instance, before: "mismatched types", after: "mismatched types (expected u32, found i32)".
Error code is added to the main message when possible. Before: "field a bound multiple times in the pattern", after: "field a bound multiple times in the pattern [E0027]".
For an errors within a macro, the macro definition is added to the trace. A special message type Macro is created for that.
A list of redundant messages is created to prevent displaying span labels that repeat the main message text in different wording. For instance, it's "struct Foo does not have a field named bar" instead of "struct Foo does not have a field named bar (struct Foo does not have field bar)". The list is safe to use because it checks exact message templates, so no useful information will be removed even if the error messages will be changed in the future. List is filled with three dozens of such messages, but can be easily expanded when redundancy is noticed.
If a trace item has the same location as the main message, this location is omitted in such an item to remove unnecessary noise.
A special case messages can be handled now before displaying. For instance, the compiler doesn't emit location for the error "main function not found". I add "src/main.rs" to such messages. It's not the ideal solution, but works better than what we have now.
Parsers (for JSON messages and for panics) are moved to their own files to simplify further maintenance.
JSON output format is default now.

I've checked it on several dozens of error messages and found no problems with missing useful information from the original JSON message. If you notice something, please, let me know.

I left the standard output parsing intact, just made a simple fix to prevent it from displaying broken messages when the new error format is used. So, it now works for the current error format but is effectively switched off for the new error format (I've provided a caveat in the "JSON format" setting description).

I'm intended to get back to the new error format parsing shortly. But it will be a separate PR.

There's an issue to discuss: what should we do with the existing settings? Should we forcefully switch all the users to the JSON format while upgrading? Since the standard output has been default so far, many use it and might lose linter messages after switching to the new error format if we do nothing. I'm not sure I'll have time to implement new parser quickly (after a closer look, I'm not even sure that it might be properly implemented for all errors).

Fixes #61

colin-kiegel · 2016-09-17T20:37:22Z

lib/errors.js

+    return true;
+  }
+  if (msg.message === 'main function not found') {
+    msg.file = buildWorkDir + '/src/main.rs';	// TODO: When running an example, set the path to the example file.


that path can be changed in Cargo.toml ... why not file it as a rustc bug, that the file is not reported in the json error represntation instead?

I tried pointing it to Cargo.toml but then switched back because it's very unlikely that Cargo.toml is the file in which the developer will be fixing this problem. It's usually either src/main.rs or one of the examples. Making difference between main.rs and an example file requires passing a wider building context to this function. Since we don't do it now, I just left main.rs.

I'm not sure what solution is better. Cargo.toml is universal, but seems useless from the practical point of view.

Regarding filing a bug, I'm going to do that after studying it a bit closer to be able to come up with some practical suggestions. I told Jonathan about it but to no avail so far.

I suggest not to guess the filename in atom, if it is not specified. Instead you could display all errors with missing filenames as error pop-ups, as you did with the runtime errors.

Ultimately this should be fixed in rustc instead.

I don't like guessing filenames either, but we only use it for a particular message for which it's really possible to guess the exact file. There's another exceptional error message: error: aborting due to previous error that is simply ignored now because it doesn't relate to the source code.

I would suggest the following:

Ignore the aborting due to previous error error.

Keep a special case for the main function not found error (provide the filename ourselves) until it's fixed in Rust (here's the issue).

Display notifications for all other messages with no location in case we encounter something unknown at this time.

The thing is that the path can be customised, with the following Cargo.toml it is src/foo.rs and not src/main.rs.

[[bin]] name = "foo" path = "src/foo.rs"

You can completely overwrite this behaviour and even define multiple binaries. In these edge cases the guess will be wrong and point to a nonexistent or not the right file. Pointing to the wrong binary might be especially confusing if that one has a main function...

The question is, is it worth to have better diagnostics in 97℅ of the cases if that means getting 3℅ completely wrong? These numbers are just my personal estimate and may be far off.

You're right. Given such configuration possibilities, it's better not to divine. I'll remove this special case and add notifications.

colin-kiegel · 2016-09-17T20:41:52Z

I think there is good reason to change the setting to json for everyone after the update

alygin · 2016-09-18T12:53:50Z

Errors without locations are displayed as notifications now.
The "JSON error format" setting will be set to true for all users after the upgrade.

…rors

oli-obk

Just some nits. This is great work!

We should probably start bugging rustc devs to add a compiler option to generate json-producing panic code.

oli-obk · 2016-09-19T08:21:09Z

lib/json-parser.js

+    && span.column_end === msg.col_end;
+}
+
+// Appends spans's lable to the main message. It only adds the lable if:


oli-obk · 2016-09-19T08:23:40Z

lib/json-parser.js

+  if (!span.label || msg.message.indexOf(span.label) >= 0) {
+    return;
+  }
+  for (let idx = 0; idx < redundantLabels.length; idx++) {


for (var l in redundantLabels)

In JS, for (var l in redundantLabels) iterates through indices, not through the array elements. At first glance, it looked counterintuitive to me, so I decided to leave the explicit iteration through indices.

redundantLabels.forEach() also doesn't work well here because it becomes too wordy when you need to return from the main function within a forEach-callback.

oh... that's very confusing. redundandLabels.some() is too unreadable, so lets stay with this.

oli-obk · 2016-09-19T08:29:54Z

lib/panic-parser.js

+'use babel';
+
+//
+// Panics and stack backtrades parser.


nit: backtraces

oli-obk · 2016-09-19T10:44:54Z

lib/json-parser.js

 // - the main message doesn't contain exactly the same phrase
 // - the main message doesn't contain the same information but uses different wording
 function appendSpanLabel(span, msg) {
  if (!span.label || msg.message.indexOf(span.label) >= 0) {
    return;
  }
-  for (let idx = 0; idx < redundantLabels.length; idx++) {
+  for (idx in redundantLabels) {


missing var

It's not only var, Travis requires an additional if within the loop because it thinks that I iterate through the simple object properties and must check if those properties are internal or not. With such check the loop becomes more wordy. It seems the original loop was more concise than any other option.

alygin added 3 commits September 17, 2016 22:23

Improved support for JSON error format

3b50312

Fixed formatting

785079f

Fixed formatting

4276bde

colin-kiegel reviewed Sep 17, 2016

View reviewed changes

alygin added 2 commits September 18, 2016 15:38

Display errors without locations as notifications

efd03e7

Reset "JSON error format" setting

6d55f2a

Fixed suppressing of the compilation abortion message for multiple er…

0116901

…rors

oli-obk suggested changes Sep 19, 2016

View reviewed changes

Minor refactoring and typo fixes

af1fcee

oli-obk reviewed Sep 19, 2016

View reviewed changes

Fixed failed Travis check

9b241fd

oli-obk approved these changes Sep 19, 2016

View reviewed changes

oli-obk merged commit d79fd62 into AtomBuild:master Sep 19, 2016

alygin deleted the json-parsing-enhancement branch September 20, 2016 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON errors parsing enhancement #62

JSON errors parsing enhancement #62

alygin commented Sep 17, 2016

colin-kiegel Sep 17, 2016

alygin Sep 17, 2016

colin-kiegel Sep 17, 2016

alygin Sep 18, 2016

colin-kiegel Sep 18, 2016

alygin Sep 18, 2016

colin-kiegel commented Sep 17, 2016

alygin commented Sep 18, 2016

oli-obk left a comment

oli-obk Sep 19, 2016

oli-obk Sep 19, 2016

alygin Sep 19, 2016

oli-obk Sep 19, 2016

oli-obk Sep 19, 2016

oli-obk Sep 19, 2016

alygin Sep 19, 2016

JSON errors parsing enhancement #62

JSON errors parsing enhancement #62

Conversation

alygin commented Sep 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-kiegel commented Sep 17, 2016

alygin commented Sep 18, 2016

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment