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

Use JSON output for rust and rust-cargo checkers #1036

Merged
merged 1 commit into from Aug 25, 2016

Conversation

Projects
None yet
5 participants
@fmdkdd
Member

fmdkdd commented Aug 13, 2016

See #1035 for the background discussion.

Essentially: this pull request restores flycheck for users of rust nightly, while (almost) preserving the functionality for rust stable users.

Almost, because some messages may have been slightly altered (see the tests update).

I've switched the error parsers of rust and rust-cargo to parse the JSON output directly. I've based the code on the tslint JSON parser, but the situation was alas not as simple.

Things to watch out for: the -Z unstable-options might trigger an error in the future. We can revisit it when it does.

Also, I think the JSON output requires at least rust 1.7, meaning we might break some setups. Is there a place where we should state that flycheck now requires a more recent rust version?

@CLAassistant

This comment has been minimized.

CLAassistant commented Aug 13, 2016

CLA assistant check
All committers have signed the CLA.

@mkpankov

This comment has been minimized.

Contributor

mkpankov commented Aug 14, 2016

Maybe we should add an option to work with JSON error format? Or, rather, to go back to plain text parsing that is present currently - like "compatibility mode". Sticking to interface that is both unstable and only available since specific version is scary IMO.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Aug 14, 2016

@mkpankov The JSON output is informally stable (see this discussion), and the PR talks about maintaining backward compatibility, so I would not worry on that front.

I'm also in favor of not dropping support for rust < 1.7, since rust releases may have breaking changes, and this is a legitimate reason to stay on a previous version. However I believe flycheck maintainers would rather have only one parser, so that's why I went with JSON only in this PR. But if that is not the case, I can add the text parser back in.

Keep in mind though that the text parser would only work until rust 1.12, where the new text format is the default.

As one data point, the rust poll indicates most respondents prefer the latest stable and nightly versions. For the moment, flycheck does not work for rust nightly users at all.

@mkpankov

This comment has been minimized.

Contributor

mkpankov commented Aug 15, 2016

Right.

I noticed PR doesn't build on Travis on snapshot Emacs, but that doesn't look like our problem to me. Looks like we'll have to wait for @lunaryorn

@mkpankov

This comment has been minimized.

Contributor

mkpankov commented Aug 15, 2016

As for PR itself: checked the branch locally, works for me with nightly and stable 1.10 :)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 16, 2016

@fmdkdd Thank you for this change.

I must admit, though, that I find some things in the parsing code strange. Why do we need to preprocess the output? What is a JSON "line"? Could you perhaps show an example of this JSON?

flycheck.el Outdated
CHECKER and BUFFER denote the CHECKER that returned OUTPUT and
the BUFFER that was checked respectively."
(let* ((json-array-type 'list)
(json-false nil)

This comment has been minimized.

@lunaryorn

lunaryorn Aug 16, 2016

Contributor

Isn't that the default?

This comment has been minimized.

@fmdkdd

fmdkdd Aug 16, 2016

Member

The default seems to be :json-false from reading the source code.

This comment has been minimized.

@lunaryorn

lunaryorn Aug 16, 2016

Contributor

Ah, interesting, I didn't know that. Good choice, then 👍

flycheck.el Outdated
(dolist (message messages (nreverse errors))
(let-alist message
(let ((error-message .message)
;; Assumption: .level is "error" or "warning"

This comment has been minimized.

@lunaryorn

lunaryorn Aug 16, 2016

Contributor

Please use pcase to explicitly turn the strings into Flycheck levels and signal an explicit error or add an explicit default if an unknown level occurs.

This comment has been minimized.

@fmdkdd

fmdkdd Aug 16, 2016

Member

Right, this is probably a better idea than calling intern on whatever.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Aug 16, 2016

@lunaryorn When passing the JSON flag to rustc, each output line is a JSON object (and not in a JSON array mind you). But when passing the flag through cargo, we have a mixed output of cargo text and JSON lines from rustc.

Here is an example of what you currently get:

$ cargo rustc --bin chipers -- -Z no-trans -Z unstable-options --error-format=json
   Compiling chipers v0.1.0 (file:///home/fmdkdd/proj/chipers)
warning: the option `Z` is unstable and should only be used on the nightly compiler, but it is currently accepted for backwards compatibility; this will soon change, see issue #31847 for more details
warning: the option `error-format` is unstable and should only be used on the nightly compiler, but it is currently accepted for backwards compatibility; this will soon change, see issue #31847 for more details
{"message":"expected `,`, or `}`, found `pub`","code":null,"level":"error","spans":[{"file_name":"src/cpu.rs","byte_start":10872,"byte_end":10875,"line_start":30,"line_end":30,"column_start":3,"column_end":6,"is_primary":true,"text":[{"text":"  pub ram_writes: [u64; RAM_LENGTH],","highlight_start":3,"highlight_end":6}],"label":null,"suggested_replacement":null,"expansion":null}],"children":[{"message":"struct fields should be separated by commas","code":null,"level":"help","spans":[],"children":[],"rendered":null}],"rendered":null}
{"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":null}
error: Could not compile `chipers`.

To learn more, run the command again with --verbose.

There are talks of making cargo output JSON as well in the future. I presume we would still need to get the compiler errors out of this JSON, but the parsing code might be simplified a bit.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 16, 2016

@fmdkdd Oh, that's, uhm, an interesting format 😂 It's rather unusual to print line-wise JSON data, imho. Would you mind to add a (lengthy?) comment that explains this format to the docstring of the parsing function? That'd really help us to be able to maintain the parser in future.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 16, 2016

LGTM, but please add an explanation of the format to the docstring, as said above, and please use pcase to parse the error level as by my inline comment.

Other than that I think it's ready to be merged. @cpitclaudel would you like to take a look?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 16, 2016

@mkpankov Thanks for reviewing, and for testing this pull request.

@fmdkdd fmdkdd force-pushed the rust-json-error-format branch from cfd1657 to 1b70971 Aug 16, 2016

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Aug 16, 2016

@lunaryorn I've added a commit with the following changes:

  • Describe the expected input to the parser.
  • Describe the relations between diagnostics, spans and children.
  • Rename messages to diagnostics
  • Use pcase to catch unexpected error levels.
  • Replace a concat by a more readable format.

Hopefully I did not go overboard with the explanations. The logic for this function was unfortunately less straightforward than expected.

Thanks for the review, and indeed thanks @mkpankov for your input.

flycheck.el Outdated
;;
;; We first iterate over diagnostics and their spans to turn every span into
;; a flycheck error object, that we collect into the `errors' list.
(dolist (diagnostic diagnostics (nreverse errors))

This comment has been minimized.

@lunaryorn

lunaryorn Aug 17, 2016

Contributor

Oh, good intentions 👍, but please don't use the 3rd argument to dolist 😊. I absolutely don't like it; it's awfully confusing, particularly to folks which are not too familiar with Emacs Lisp.

Just stick to returning (nreverse errors) at the very end, like in all the other error parsers.

This comment has been minimized.

@fmdkdd

fmdkdd Aug 17, 2016

Member

Right! It probably did confuse me the first time I saw it too.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 17, 2016

@fmdkdd Thanks a lot, particularly for these lengthy explanations. These are really helpful!

Just two things left:

  • Please don't use this fancy 3rd argument to dolist (see my inline comment).
  • The Travis CI build fails with some compiler warnings on Emacs snapshot that I admittedly don't really understand. Maybe you could take another look and try to get rid of these warnings on Emacs 25? Or find a workaround? We require that our code triggers no warnings on Emacs 25.

If you can't get your head around these warnings just tell me and I'll take a look myself this weekend 😊

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 17, 2016

Forgot one thing: If you're done please squash all commits into a single one 😊

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Aug 17, 2016

@lunaryorn It appears that nesting let-alist calls trigger the warnings. Unnesting definitely remove them anyway.

Once the Travis build confirms this, I will squash everything for the merge.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 17, 2016

@fmdkdd Oh, that's unfortunate. I'm not sure whether your refactoring is the best way out of this, though. We shouldn't use mutable variables too much, especially not to preserve a syntactic form.

I'll think about a better way to structure the code—doesn't stand against merging, though, as I can refactor later.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 17, 2016

@fmdkdd Looks good. I've restarted the failing build, which was a random fluke.

I forgot one thing: Now that we require Rust 1.7 or newer, please mention the new version requirements in the docstrings of rust and rust-cargo, and in doc/languages.rst as well (you may need to rebase onto master first for the latest updates to the latter).

After that I think it's ready to go, but let's have @cpitclaudel opinion first :)

@fmdkdd fmdkdd force-pushed the rust-json-error-format branch from fac3430 to 26066d0 Aug 17, 2016

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Aug 17, 2016

@lunaryorn I agree that the workaround is not ideal; I tried to avoid (cdr (assq. I read that there is an alist-get in Emacs 25 that could alleviate that.

But actually, I think it's a bug/limitation in let-alist. Here is what a nested call expands to:

(cl-prettyexpand
 '(let-alist '((a . 1) (b . 2))
    (let-alist '((c . 3) (d . 4))
      .c)))

(let ((alist '((a . 1) (b . 2))))
  (let ((\.c (cdr (assq 'c alist))))  <-- triggers a warning for the unused binding
    (let ((alist '((c . 3) (d . 4))))
      (let ((\.c (cdr (assq 'c alist))))
        \.c))))

Oviously the first let is premature. The correct thing would be to expand the inner let-alist first, then the outer one would just ignore the \.c and not write the first let.

The docstring acknowledges the fact that you can nest them, with the downside that you won't be able to access the variables of the outer one: we can see that's indeed the case because alist is shadowed. This is a macro hygiene issue.

I can report this and see if the maintainers want to support nested calls at all. This affects flycheck-rust as well.

Otherwise, I've rebased, stated that we require 1.7 in all the right places, and squashed.

flycheck.el Outdated
(json-false nil)
;; Skip the plain text lines in OUTPUT, keep the JSON lines.
(json-lines (seq-filter (lambda (line)
(string-match-p "^\{" line))

This comment has been minimized.

@cpitclaudel

cpitclaudel Aug 25, 2016

Member

I think escaping { is superfluous.

This comment has been minimized.

@fmdkdd

fmdkdd Aug 25, 2016

Member

You are right. I began by splitting the output using sed in my shell, then just used the same expression in elisp :)

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Aug 25, 2016

Looks great, thanks (LGTM)! It's too bad that let-alists can't be nested, but the commit is very readable. I made a tiny comment, but it isn't essential.

@fmdkdd fmdkdd force-pushed the rust-json-error-format branch from 26066d0 to 2c90eee Aug 25, 2016

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 25, 2016

@fmdkdd Thanks, LGTM. Can you add a note about this change to the Breaking changes section of CHANGES.rst before you merge 😊?

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Aug 25, 2016

@lunaryorn Sure. In the same commit, or as a new one?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 25, 2016

@fmdkdd In the same if possible :)

@fmdkdd fmdkdd force-pushed the rust-json-error-format branch 2 times, most recently from a9c63f5 to b420836 Aug 25, 2016

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Aug 25, 2016

@lunaryorn Done. Let me know if that works for you.

CHANGES.rst Outdated
@@ -6,6 +6,8 @@
- Change ``flycheck-eslint-rulesdir`` (string) to
``flycheck-eslint-rules-directories`` (list of strings) [GH-1016]

This comment has been minimized.

@lunaryorn

lunaryorn Aug 25, 2016

Contributor

There's no need for this blank line.

CHANGES.rst Outdated
@@ -23,6 +25,8 @@
- Add default directory for ``haskell-stack-ghc`` and ``haskell-ghc`` checkers
[GH-1007]

This comment has been minimized.

@lunaryorn

lunaryorn Aug 25, 2016

Contributor

There's no need for this blank line.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 25, 2016

@fmdkdd Two minor style nitpicks. Please fix these and merge the pull request. I think you need to rebase onto master first to get the Travis CI build working again: Emacs snapshot is currenlty failing, so I added Emacs pretest to the build and allowed the snapshot build to fail on master.

Use JSON output for rust and rust-cargo checkers
This commit replaces the error parsers for the rust and rust-cargo
checkers to use the JSON output instead.

As a side-effect, we skip the "help" messages which promote the use of
`rustc --explain` in the tests.  I'm assuming this is not critical,
since we also always set the error code now.

Closes GH-1035.

@fmdkdd fmdkdd force-pushed the rust-json-error-format branch from b420836 to 6953ee3 Aug 25, 2016

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Aug 25, 2016

@lunaryorn Nits fixed. Rebased and indeed CI now builds. Should be good to go!

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 25, 2016

Please go ahead and merge 😊

@fmdkdd fmdkdd merged commit d54cafd into master Aug 25, 2016

4 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@fmdkdd fmdkdd deleted the rust-json-error-format branch Aug 25, 2016

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Aug 25, 2016

Yay! Great work :)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 25, 2016

@fmdkdd Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment