Skip to content

Fix error related to multiple binary targets #25

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Fix error related to multiple binary targets #25

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf58f48
Fix error related to multiple binary targets
fmdkdd May 7, 2016
5e54256
Update after code review
fmdkdd May 8, 2016
b66ead1
Simplify code after code review
fmdkdd May 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions flycheck-rust.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@

(require 'dash)
(require 'flycheck)
(require 'json)

(defun flycheck-rust-executable-p (rel-name)
"Whether REL-NAME denotes an executable.
Expand Down Expand Up @@ -94,6 +95,32 @@ Return non-nil if PROJECT-ROOT is a binary crate, nil otherwise."
(let ((root-dir (file-name-directory project-root)))
(file-exists-p (expand-file-name "src/main.rs" root-dir))))

(defun flycheck-rust-find-target (file-name)
"Find and return the cargo target associated with the given file.

FILE-NAME is the name of the file that is matched against the
'src_path' value in the list 'targets' returned by 'cargo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emacs docstrings use a different quoting style, namely ``src_path'`. Please update all quotes accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure what to use because those weren't elisp value I was quoting. Good to know.

read-manifest'. If there is no match, the first target is
returned by default.

Return a cons cell (TYPE . NAME), where TYPE is the target
type (lib or bin), and NAME the target name (usually, the crate
name)."
(let* ((manifest (let ((json-array-type 'list))
(json-read-from-string
(shell-command-to-string "cargo read-manifest"))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use shell-command-to-string. Going through the shell comes with all kinds of troubles, from poor error reporting to messy output, etc.

Use (with-temp-buffer (call-process "cargo" nil (current-buffer) nil "read-manifest") (json-read)) instead. That avoids the shell and gives us direct access to the process which is more efficient and easier to debug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, skipping the shell is indeed best.

Out of curiosity, would with-output-to-string work as well, or are there downsides?

(default-target (cadr (assq 'targets manifest)))
;; Assuming there is always at least one target
(target-type (cadr (assq 'kind default-target)))
(target-name (cdr (assq 'name default-target))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at let-alist and use it instead of cdr and friends. This makes code that parses alists much easier to read.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I was wondering if there wasn't something more direct to deal with that.

;; If there is a target that matches the file-name exactly, pick that one
;; instead
(dolist (target (cdr (assq 'targets manifest)))
(when (string= file-name (cdr (assq 'src_path target)))
(setq target-type (cadr (assq 'kind target)))
(setq target-name (cdr (assq 'name target)))))
(cons target-type target-name)))

;;;###autoload
(defun flycheck-rust-setup ()
"Setup Rust in Flycheck.
Expand All @@ -103,7 +130,8 @@ Flycheck according to the Cargo project layout."
(interactive)
(when (buffer-file-name)
(-when-let (root (flycheck-rust-project-root))
(let ((rel-name (file-relative-name (buffer-file-name) root)))
(let ((rel-name (file-relative-name (buffer-file-name) root))
(target (flycheck-rust-find-target (buffer-file-name))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we don't need target itself but only its components, please use pcase-let instead of let to destructure the pair immediately. I think the binding form would be ((,target-type . ,target-name) (flycheck-rust-find-target …))`.

Makes no difference semantically, but it's much easier to read :)

;; These are valid crate roots as by Cargo's layout
(if (or (flycheck-rust-executable-p rel-name)
(flycheck-rust-test-p rel-name)
Expand All @@ -117,9 +145,16 @@ Flycheck according to the Cargo project layout."
;; Check tests in libraries and integration tests
(setq-local flycheck-rust-check-tests
(not (flycheck-rust-executable-p rel-name)))
;; Set the crate type
(setq-local flycheck-rust-crate-type
(if (flycheck-rust-binary-crate-p root)
"bin" "lib")) ;; Set the crate type
(if (string= (car target) "bin")
(progn
;; If it's binary target, we need to pass the binary
;; name
(setq-local flycheck-rust-binary-name
(cdr target))
"bin")
"lib"))
;; Find build libraries
(setq-local flycheck-rust-library-path
(list (expand-file-name "target/debug" root)
Expand Down