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

Add cargo clippy #1385

Merged
merged 1 commit into from Feb 11, 2018

Conversation

Projects
None yet
3 participants
@fmdkdd
Member

fmdkdd commented Jan 15, 2018

This adds clippy as another checker for rust. I've picked up #1156, where most of the heavy-lifting had already been done by @dzamlo.

I've tweaked the rust parser to handle a corner case with suggested replacement by Clippy.

Adding a third checker on the same syntax-checker:: line in doc/languages.rst broke the tests, as the regexp did not handle this case. I've extended it for 3 lines, but... I'm sure there is a more definitive solution waiting to be found ;)

The branch is based on #1381 and #1289, so we should probably merge these before this one.

(cl-pushnew (intern match) matches))))
matches))
(defconst flycheck/checker-re
(rx bol " .. syntax-checker:: " (group (1+ nonl)) "\n"
(? " " (group (1+ nonl)) eol)))
(? " " (group (1+ nonl)) "\n")
(? " " (group (1+ nonl))) eol))

This comment has been minimized.

@cpitclaudel

cpitclaudel Jan 16, 2018

Member

As an alternative, would replacing the original ? with a * work?

This comment has been minimized.

@fmdkdd

fmdkdd Jan 16, 2018

Member

The regexp would still match, but the trouble is in extracting the result with match-string. The group in the star * will contain only the last match (here, rust-clippy).

This comment has been minimized.

@cpitclaudel

cpitclaudel Jan 16, 2018

Member

Ah, yes, of course. Then a single ? with a + inside the capturing group? The calling code would have to then further split the match (string-split on spaces)

This comment has been minimized.

@cpitclaudel

cpitclaudel Jan 16, 2018

Member

I think this is just fine as it is, though — we can deal with this problem later, if/when we need an extra addition

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Feb 10, 2018

Rebased on master, now that both previous PR have been merged.

flycheck.el Outdated
;; Use the line/column from the first span if there is one, or
;; fallback to the line/column information from the primary span of
;; the diagnostic.
(or (and (car .spans)

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 10, 2018

Member

I wonder if this and is needed: if (car .spans) is nil, then assq will return nil as well.
Hence maybe (let-alist (car .spans) .column_start) works?

This comment has been minimized.

@fmdkdd

fmdkdd Feb 10, 2018

Member

We can't nest let-alist without running into byte-compile warnings. But we can certainly get rid of the and since nil is safe.

After programming in stricter languages than Elisp, you tend to put guards defensively ;)

This comment has been minimized.

@cpitclaudel

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 10, 2018

Member

Which warnings do you get? I don't get any for the following in Emacs 24.5, nor on master:

;;; test --- test
;;; Commentary:
;;; Code:

(require 'let-alist)

(defun ~/access (x)
  "X."
  (let-alist x
    (let-alist (car .x) .xx)))

(~/access '((x . (((xx . 1) (yy . 2)) ((xx . 4) (yy . 5))))))

(provide 'test)
;;; test.el ends here
flycheck.el Outdated
(cdr (assq 'line_start (car .spans))))
primary-line)
(or (and (car .spans)
(cdr (assq 'column_start (car .spans))))

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 10, 2018

Member

Same question/suggestion here

flycheck.el Outdated
;; these cases.
(-if-let* ((first-span (car .spans))
(replacement
(cdr (assq 'suggested_replacement first-span))))

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 10, 2018

Member

and here :)

flycheck.el Outdated
(-if-let* ((first-span (car .spans))
(replacement
(cdr (assq 'suggested_replacement first-span))))
(format "%s: `%s`" .message replacement)

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 10, 2018

Member

It might be better to use `…' instead of `…` here?

This comment has been minimized.

@fmdkdd

fmdkdd Feb 10, 2018

Member

I believe rustc uses the Markdown convention, so it might be more coherent to follow that in the displayed message. It's just text, so it doesn't really matter. Unless someone tries to prettify that down the road.

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 10, 2018

Member

That's what I'm afraid about (prettification). I think popups do get prettified, though arguably it's a bug on the Emacs side.

flycheck.el Outdated
@@ -9375,6 +9390,24 @@ Relative paths are relative to the file being checked."
(flycheck-error-message err)))
errors))
(defun flycheck-rust-manifest-directory ()
"Return the neaerst directory holding the Cargo manifest.

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 10, 2018

Member

typo neaerst

flycheck.el Outdated
"A Rust syntax checker using clippy.
See URL `https://github.com/rust-lang-nursery/rust-clippy'."
:command ("cargo" "\+nightly" "clippy" "--message-format=json")

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 10, 2018

Member

That \ looks suspicious

This comment has been minimized.

@fmdkdd

fmdkdd Feb 10, 2018

Member

It does! But the command didn't work without it.

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 10, 2018

Member

Huh. That is strange. AFAICT this \ doesn't do anything: it escapes a non-special character, which is the same as not escaping it (I mean that (string= "\+test" "+test") evaluates to t). Am I missing something?

This comment has been minimized.

@fmdkdd

fmdkdd Feb 11, 2018

Member

Hmm. I could not reproduce the issue on this machine, so I removed the \. Hopefully it was a fluke.

@fmdkdd fmdkdd force-pushed the add-cargo-clippy branch from 6bc9310 to c7a2ca0 Feb 10, 2018

@fmdkdd fmdkdd force-pushed the add-cargo-clippy branch from c7a2ca0 to 2f29e2f Feb 11, 2018

@fmdkdd fmdkdd merged commit f61a9dc into master Feb 11, 2018

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
license/cla Contributor License Agreement is signed.
Details

@fmdkdd fmdkdd deleted the add-cargo-clippy branch Feb 11, 2018

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