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 `--message-format=json` flag for rust-cargo checker #1201

Merged
merged 1 commit into from Feb 7, 2017

Conversation

Projects
None yet
3 participants
@fmdkdd
Member

fmdkdd commented Feb 3, 2017

To get JSON output, rust-cargo used the --error-format=json flag of rustc.
This had the downside of getting JSON output only from the last invocation of
rustc by cargo. Crucially, when compiling dependencies with errors, these
errors would be reported as plain text rather than JSON, resulting in a
suspicious checker state for rust-cargo.

In the 1.13 release of Rust, Cargo added the --message-format=json to ensure
all invocations of rustc would return JSON messages. Unfortunately, the
--message-format flag was not compatible with the -Zno-trans option that
cuts down on compilation time by not generating an
executable (rust-lang/cargo#3390). This issue is fixed by release 1.15 of Rust.

This commit changes rust-cargo to pass the --message-format flag instead of
--error-format, ensuring that errors from dependencies are correctly turned
into flycheck-error objects.

Since Cargo merely encapsulates the JSON messages from rustc, this commits
extracts the parsing of rustc diagnostics into its own function,
flycheck-parse-rustc-diagnostic, to allow the new flycheck-parse-cargo-rustc
and the existing flycheck-parse-rust (renamed flycheck-parse-rustc) to parse
individual diagnostics.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Feb 3, 2017

Note: this is a pre-requisite for handling different targets (benches, examples, tests) with rust-cargo. Dropping Rust < 1.15 is unfortunate, but using --message-format is the correct way to make this work. This change will also benefit #1191.

CHANGES.rst Outdated
@@ -1,6 +1,10 @@
31-cvs (in development)
=======================
- **Breaking changes**
- ``rust-cargo`` now requires Rust 1.15 or newer

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 5, 2017

Member

Do we typically document these? (Not saying its a bad thing)

This comment has been minimized.

@fmdkdd

fmdkdd Feb 6, 2017

Member

We did the previous time for rust, and also for flake8.

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 6, 2017

Member

Cool, thanks. I should have checked myself 😊

@@ -357,6 +357,10 @@ check that the buffer has all ERRORS, and no other errors."
(should (= (length errors)
(length (flycheck-overlays-in (point-min) (point-max))))))
(put 'flycheck-ert-suspicious-checker 'error-message
"Suspicious state from checker.")
(put 'flycheck-ert-suspicious-checker 'error-conditions '(error))

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 5, 2017

Member

Did you want define-error here?

This comment has been minimized.

@fmdkdd

fmdkdd Feb 6, 2017

Member

I just copied the way it was done for flycheck-ert-syntax-check-timed-out and flycheck-ert-skipped higher in the same file. But it looks like define-error is a more direct way. Want me to refactor the others using define-error as well?

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 6, 2017

Member

I think it looks better; what do you think?

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 Feb 5, 2017

Member

I tend to prefer string-prefix-p for this

flycheck.el Outdated
(string-match-p "^{" line))
(split-string output "\n"))))
;; Each JSON line is a JSON object.
(seq-map #'json-read-from-string json-lines)))

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 5, 2017

Member

Could this work better with repeated calls to json-read in a temp buffer? (Not sure; just suggesting)

This comment has been minimized.

@fmdkdd

fmdkdd Feb 6, 2017

Member

Ah, because json-read advances point? I think it would work just as well. Not sure which would be faster though. Since output is already a string, I did not think of writing it to a buffer.

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 6, 2017

Member

json-read-from-string uses a temporary buffer, so I'd bet on json-read being a bit faster. But whether that's worth the extra complexity, I don't know :) I'm thinking of something like this:

(defun ~/read-str (str)
  (let ((objects nil))
    (with-temp-buffer
      (insert str)
      (goto-char (point-min))
      (while (not (eobp))
        (when (memq (char-after) '(?\{ ?\[))
          (push (json-read) objects))
        (forward-line)))
    (nreverse objects)))

This comment has been minimized.

@fmdkdd

fmdkdd Feb 6, 2017

Member

In the interest of science, I conducted a micro-benchmark using benchmark-run-compiled. On a large buffer (300 JSON error objects mixed with plain text), running your version takes 0.05 seconds, while the seq-map version takes 0.06 seconds. Going to 100 iterations shows the same difference (5.34 vs 6.9), with slightly more variance on the seq-map version on a few runs.

I don't think the performance difference would matter in practice. But since I do like your version more anyway – very raw elisp, while still being readable – I'll go with it :)

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 7, 2017

Member

Cool. Another potential benefit of this approach is that it should work fine fine for pretty-printed JSON, too (the following should work, for example:)

Some text here
[1, 2, 3, {"a": 1,
           "b": 2},
 4, 5, 6, {"c": 3,
           "d": 4}]
More text, wrapped to
announce another object:
[1, 2, 3, {"a": 1,
           "b": 2},
 4, 5, 6, {"c": 3,
           "d": 4}]
flycheck.el Outdated
;; unwrap them, and delegate the actual construction of `flycheck-error'
;; objects to `flycheck-parse-rustc-diagnostic'.
(when (string= .reason "compiler-message")
(setq errors (nconc errors (flycheck-parse-rustc-diagnostic .message checker buffer))))))

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 5, 2017

Member

Does this need to be rewrapped?
Also, this looks a bit inefficient (quadratic?) Can you accumulate all lists of errors first, and nconc them all with (apply #'nconc lists-of-errors) at the end?

This comment has been minimized.

@fmdkdd

fmdkdd Feb 6, 2017

Member

Good catch. And here I was using nconc because append would have been quadratic... but somehow I thought setting the cdr in nconc was constant, not linear. But looking at the source it's definitely not.

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 6, 2017

Member

Cool, thanks. The new version looks great.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 5, 2017

Awesome work. I left some minor comments :)

flycheck.el Outdated
(nreverse errors)))
(defun flycheck-parse-rust-filter-json (output)

This comment has been minimized.

@Simplify

Simplify Feb 6, 2017

Member

Can this be renamed to more generic name? typescript-tslint can use this function to remove plain text configuration deprecation notices before JSON output. Upcoming ruby-reek checker also.

This comment has been minimized.

@fmdkdd

fmdkdd Feb 6, 2017

Member

I see that tslint returns an array of JSON objects, while rustc returns JSON objects on different lines. So we have to match on different prefixes.

But I'll try to make it more generic so at least the filtering can be used by other checkers.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 6, 2017

Thanks for the quick answers! I left a few replies, but things are looking great — feel free to merge when you think things are ready :)

Use `--message-format=json` flag for rust-cargo checker
To get JSON output, rust-cargo used the `--error-format=json` flag of rustc.
This had the downside of getting JSON output only from the *last* invocation of
rustc by cargo.  Crucially, when compiling dependencies with errors, these
errors would be reported as plain text rather than JSON, resulting in a
suspicious checker state for rust-cargo.

In the 1.13 release of Rust, Cargo added the `--message-format=json` to ensure
all invocations of rustc would return JSON messages.  Unfortunately, the
`--message-format` flag was not compatible with the `-Zno-trans` option that
cuts down on compilation time by not generating an
executable (rust-lang/cargo#3390).  This issue is fixed by release 1.15 of Rust.

This commit changes rust-cargo to pass the `--message-format` flag instead of
`--error-format`, ensuring that errors from dependencies are correctly turned
into `flycheck-error` objects.

Since Cargo merely encapsulates the JSON messages from rustc, this commits
extracts the parsing of rustc diagnostics into its own function,
`flycheck-parse-rustc-diagnostic`, to allow the new `flycheck-parse-cargo-rustc`
and the existing `flycheck-parse-rust` (renamed `flycheck-parse-rustc`) to parse
individual diagnostics.

This commit also adds a test for the behavior that `--message-format` fixes over
`--error-format`.  This required to instruct the flycheck-ert helpers to fail
when a flycheck reported a suspicious checker state.

Previously, a suspicious status would be printed out while running the tests,
but would not mark the test as failed.

This commits adds a hook to check for change in checker status and fails the
test if the checker reports a suspicious status.

@fmdkdd fmdkdd force-pushed the rust-cargo-use-message-format branch from 9afa320 to 3f05317 Feb 6, 2017

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Feb 6, 2017

@Simplify I added flycheck-parse-json which parses JSON from a mix of plain text, and should work for JSON arrays as well. Since it parses every line and put them in a list, if eslint/tslint outputs a single array, you will have to car it out to get its contents. Hope it works for you.

@fmdkdd fmdkdd merged commit e46c04e into master Feb 7, 2017

3 checks passed

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-cargo-use-message-format branch Feb 7, 2017

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Feb 7, 2017

Oh and thank you @cpitclaudel for a great review, as always 👍

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 7, 2017

Thanks to you for all your great work :)

@Simplify

This comment has been minimized.

Member

Simplify commented Feb 12, 2017

@fmdkdd thanks a lot, works as intended indeed 👍

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