Extract build settings and project layout from Cargo itself

[swsnr]

Don't guess, use Cargo's own settings. Blocked by rust-lang/cargo#1434

Specifically: Extract the layout, the crate roots, the dependencies, and probably compiler flags as well. Will probably require changes to Flycheck's rust checker, to pass additional flags.

Fixes: #5, #7, probably #11

[ane]

For #7, I have a tentative fix that covers sub-crates like docopt_macros and works with the lib subtree of the cargo project, but the src/bin still doesn't work because there's no definite way to know what the actual binary file is (in cargo's case it's src/bin/cargo.rs). But I tested with libs (mio) and binaries (racer), and it seemed to work fine.

It's not what I'd call "good", but it's definitely better than it was.

[swsnr]

@ane As I said, feel free to open a pull request. It's just that I won't spend any of my time on this anymore until Cargo gets an interface to export its project settings.

[ane]

Yeah. Well, that should be hopefully soon, as @winger seems to be working on it.

[tomjakubowski]

Now that cargo rustc is a thing, is it even necessary to extract this information? I'll experiment with that I guess.

[tomjakubowski]

Well, that was shockingly straightforward. I defined a new flycheck syntax checker and it seems to just work:

(flycheck-define-checker cargo-rust
  "A Rust syntax checker using cargo rustc.
This syntax checker needs Rust 1.1 or newer.
See URL `http://www.rust-lang.org'."
  :command ("cargo" "rustc" "--" "-Z" "no-trans") ;; <---- the big change
  :error-patterns
  ((error line-start (file-name) ":" line ":" column ": "
          (one-or-more digit) ":" (one-or-more digit) " error: "
          (or
           ;; Multiline errors
           (and (message (minimal-match (one-or-more anything)))
                " [" (id "E" (one-or-more digit)) "]")
           (message))
          line-end)
   (warning line-start (file-name) ":" line ":" column ": "
            (one-or-more digit) ":" (one-or-more digit) " warning: "
            (message) line-end)
   (info line-start (file-name) ":" line ":" column ": "
         (one-or-more digit) ":" (one-or-more digit) " " (or "note" "help") ": "
         (message) line-end))
  :modes rust-mode
  :predicate (lambda ()
               (flycheck-buffer-saved-p))) ;; <--- another important change

I think this design only supports the "check on save" model of syntax checking, which is unfortunate, but I think a reasonable tradeoff given that everything else works perfectly, as far as I can tell, with no painful extraction and guessing at library paths and crate roots and the like.

I'm unsure if it makes sense to have a separate checker for cargo rustc, or to make the single rust checker configurable enough to support this. If going the single checker route, these things would need to be configurable to support cargo rustc:

• Whether a filename (of the current buffer, of a temporary file created by flycheck, whatever) is passed as a command argument. cargo rustc already passes the path to the crate root to the compiler, and rustc complains if you run something like cargo rustc -- -Z no-trans path/to/some_mod.rs because it sees multiple source filenames.
• The command used to invoke the compiler. Obviously cargo rustc needs to be able to start the invocation with cargo rustc -- instead of just rustc.

[tomjakubowski]

From cargo rustc --help:

This command requires that only one target is being compiled. If more than one
target is available for the current package the filters of --lib, --bin, etc,
must be used to select which target is compiled.

So there's another thing that needs to be configurable, for some projects.

[blaenk]

@tomjakubowski is correct, we just need to call cargo rustc -- --Zno-trans which is the correct way to do somethign like this. In the meantime I'll be using this, since it's pretty tedious/unusable to use rustc directly without the cargo project information baked in.

[blaenk]

I think this design only supports the "check on save" model of syntax checking, which is unfortunate

@tomjakubowski I'm new to flycheck and emacs in general, do you mean that your change to support cargo rustc makes it only work with 'check on save', whereas the original checker worked in more situations? If so, what about this restricts it that way? Or has that always been the case?

[tomjakubowski]

@tomjakubowski I'm new to flycheck and emacs in general, do you mean that your change to support cargo rustc makes it only work with 'check on save', whereas the original checker worked in more situations? If so, what about this restricts it that way? Or has that always been the case?

That's right. Flycheck does on-the-fly checks by periodically saving the buffer to a temporary file (named something like flycheck_temp.rs) in the same directory the buffer's file resides in, and running its check commands on the temp file instead of the buffer's real filename.

The current Rust checker can check a crate's root file on-the-fly because the name of that file doesn't actually matter, so long as it's passed to rustc by the checker. Files for a crate's modules, however, generally must have certain names to work with the module system, so Flycheck only runs checks for those after you've saved the file to its proper location on the filesystem (it doesn't use the temp buffer trick).

Since Cargo.toml specifies the exact location of the crate's root, the temp file trick won't work with the cargo rustc approach to check crate root files.

[blaenk]

Thanks @tomjakubowski I appreciate the explanation.

[ahwatts]

It looks like if you pass the requisite --extern options to rustc, it figures out at least the multiple-crates issue.

This is probably not a great way to do this, but this works for me™.

(defun flycheck-rust-extern-args (lib-paths)
  (cond
   ((null lib-paths) nil)
   (t (append (flycheck-rust-extern-arg (car lib-paths))
              (flycheck-rust-extern-args (cdr lib-paths))))))

(defun flycheck-rust-extern-arg (lib)
  (let ((file (file-name-nondirectory lib)))
    (string-match "\\`lib\\([a-z0-9_]+\\)-[a-z0-9]+\.rlib\\'" file)
    (list "--extern" (concat (match-string 1 file) "=" lib))))

(defun flycheck-rust-setup ()
  "Setup Rust in Flycheck.

If the current file is part of a Cargo project, configure
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)))
        ;; These are valid crate roots as by Cargo's layout
        (unless (or (flycheck-rust-executable-p rel-name)
                    (flycheck-rust-test-p rel-name)
                    (flycheck-rust-bench-p rel-name)
                    (flycheck-rust-example-p rel-name)
                    (string= "src/lib.rs" rel-name))
          ;; For other files, the library is either the default library or the
          ;; executable
          (setq-local flycheck-rust-crate-root (flycheck-rust-find-crate-root)))
        ;; 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-executable-p rel-name) "bin" "lib"))
        ;; Find build libraries
        (setq-local flycheck-rust-library-path
                    (list (expand-file-name "target/debug" root)
                          (expand-file-name "target/debug/deps" root)))
        ;; specify extern crates
        (setq-local flycheck-rust-args
                    (flycheck-rust-extern-args
                     (directory-files (expand-file-name "target/debug/deps" root) t "\.rlib\\'")))))))

[tomjakubowski]

@lunaryorn would you accept a pull request to replace the checker in this package with one that uses cargo rustc --no-trans? The current checker's intents are noble but I hit edge cases on a seemingly daily basis (as one example: if I run cargo update, the older versions of dependency crates that stick around seem to confuse the checker) and cargo rustc seems like the "official" way to invoke the compiler with bespoke options (such as --no-trans) in a Cargo context.

[swsnr]

@tomjakubowski I'll definitely merge any pull request that resolves the current issues, in whatever way. But I do not want to replace this package with something that has edge cases on its own and just breaks in other situations.

However, I'm not sufficiently familiar with Cargo and Rust to decide, so it's really your judgement call. If you think that checker would sufficiently improve the current situation please go ahead. But please understand that I naturally reserve the right to revert your changes should they turn out to cause issues for other users.

Besides, if it's only a simple checker, please make a pull request against Flycheck itself! There's no need to have it in an extension package. At best we could obsolete and eventually delete this package. Please keep the simple rust checker, though, in case someone doesn't use Cargo.

[ane]

Cargo itself is these days such an integral part of Rust that it is very unlikely one would use Rust without it. The default Rust installer now installs it.

As I understand this PR would improve the Rust default checker (just the compiler) to work a bit better, e.g., to understand different crate layouts to which the compiler itself is oblivious. The package manager - Cargo - understands these constraints and would therefore be a better checker in every way.

Of course, a better goal would just to integrate the Rust checker via Cargo into Flycheck, instead of having a separate checker, as you mentioned.

[blaenk]

Yeah cargo has definitely become very integral to rust, unlike other languages or ecosystems. I personally believe that the cargo checker should be the default for this reason, and yes keeping this checker wouldn't hurt anyone and would definitely help out those (very) rare situations where someone isn't using cargo, but it should be opt-in.

Hope you do submit a PR, @tomjakubowski :)

[swsnr]

@tomjakubowski Any news on this?

[utkarshkukreti]

FWIW I've been using this modified version of the default Rust Flycheck checker for a few hours now and it seems to be working fine. I'm not sending a PR because it doesn't handle bin files right now.

(flycheck-define-checker rust-cargo
  "A cargo rustc checker."
  :command ("cargo" "rustc" "--lib" "--" "-Z" "no-trans")
  :error-patterns
  ((error line-start (file-name) ":" line ":" column ": "
          (one-or-more digit) ":" (one-or-more digit) " error: "
          (or
           ;; Multiline errors
           (and (message (minimal-match (one-or-more anything)))
                " [" (id "E" (one-or-more digit)) "]")
           (message))
          line-end)
   (warning line-start (file-name) ":" line ":" column ": "
            (one-or-more digit) ":" (one-or-more digit) " warning: "
            (message) line-end)
   (info line-start (file-name) ":" line ":" column ": "
         (one-or-more digit) ":" (one-or-more digit) " " (or "note" "help") ": "
         (message) line-end))
  :modes rust-mode
  :predicate (lambda ()
               (or (not flycheck-rust-crate-root) (flycheck-buffer-saved-p))))

[mkpankov]

@utkarshkukreti I modified your version with a hack and now it seems to handle binaries and libraries. Multiple binaries are not handled, though

(flycheck-define-checker rust-cargo
  "A cargo rustc checker."
  :command ("cargo" "rustc"
            (eval (if (string= flycheck-rust-crate-type "lib") "--lib" nil))
            "--" "-Z" "no-trans")
  :error-patterns
  ((error line-start (file-name) ":" line ":" column ": "
          (one-or-more digit) ":" (one-or-more digit) " error: "
          (or
           ;; Multiline errors
           (and (message (minimal-match (one-or-more anything)))
                " [" (id "E" (one-or-more digit)) "]")
           (message))
          line-end)
   (warning line-start (file-name) ":" line ":" column ": "
            (one-or-more digit) ":" (one-or-more digit) " warning: "
            (message) line-end)
   (info line-start (file-name) ":" line ":" column ": "
         (one-or-more digit) ":" (one-or-more digit) " " (or "note" "help") ": "
         (message) line-end))
  :modes rust-mode
  :predicate (lambda ()
               (or (not flycheck-rust-crate-root) (flycheck-buffer-saved-p))))

[swsnr]

@utkarshkukreti @mkpankov I'll definitely merge a PR that adds such a syntax checker to Flycheck 😎 👍 🎆

[mkpankov]

@lunaryorn do you want it to be an addition, rather than replacement of the current checker? I could prepare a PR...

[swsnr]

@mkpankov I'd prefer an additional syntax checker that's before the default rust syntax checker and has a predicate that checks for the existence of the TOML file. If there's no TOML, i.e. if it's not a Cargo project, Flycheck would then fall back to the current rust syntax checker.

I think there are still many situations in which one would write Rust outside of a Cargo project—for instance, all those that write their sample code for StackOverflow answers 😉

[mkpankov]

@lunaryorn will take a look at it. That should be a PR against the main flycheck repo, not flycheck-rust, right?

[swsnr]

@mkpankov Yes, a PR for main repo, please. The syntax checker will have no dependencies, so there's no reason to keep it out of Flycheck.

[mkpankov]

@lunaryorn I'm not sure it'll have no dependencies.

At the very least flycheck-rust-crate-type and flycheck-rust-crate-root are defined in flycheck-rust.

Or were you talking about something else?

[swsnr]

@mkpankov Flycheck itself defines these variables. flycheck-rust just sets them, but you don't need flycheck-rust for that: You could also set them yourself, i.e. via directory variables.

[swsnr]

It would appear that cargo now provides a metadata command which we could probably make use of.

[fmdkdd]

Now that we solely use cargo metadata in #43, I believe this can be closed.