Changed flycheck-rust-manifest-directory to use 'cargo metadata' #1422

kngwyu · 2018-03-07T07:37:34Z

This commit fixes #1397.
By the recent change of cargo(rust-lang/cargo#4788), flycheck can't detect error files in multi-crate project of Rust.
So I changed to use workspace_root in cargo metadata 's outputs, as working directory.
If the user's cargo is a bit old and there's no 'workspace_root', flycheck-rust-manifest-directory search a directory with Cargo.toml, as ever.

I've seen @fmdkdd's comment in rust-lang/cargo#4998, but I wrote this patch just because I need this, so please feel free to abort this PR if you don't like.

CLAassistant · 2018-03-07T07:37:41Z

All committers have signed the CLA.

fmdkdd · 2018-03-07T09:37:19Z

Thanks for reminding me about this. It looks like we don't have a choice, we have to call cargo metadata after all, as the alternative rust-lang/rust#47939 has been dropped.

And thank you for writing the patch! I'll try to review it soon.

kngwyu · 2018-03-07T14:07:19Z

I checked this change cause

Suspicious state from syntax checker rust-cargo: Flycheck checker rust-cargo returned non-zero exit code 101, but its output contained no errors: error: no library targets found

in the project like

├── Cargo.toml
├── crate1
│   ├── Cargo.lock
│   ├── Cargo.toml
│   ├── src
│   └── target
├── crate2
│   ├── Cargo.toml
│   └── src
├── rustfmt.toml
├── target

so it needs to be fixed!
I'm sorry for poor testing.

kngwyu · 2018-03-07T14:34:34Z

This error occurs in a Project which has both bin and lib (and etc..) targets.
In addition, changing working-directory into workspace_root can make compile time much more longer!
Hmm, now I feel this is a bad way....

Is there any good idea to separate the directory where flycheck run command, from working directory?

fmdkdd · 2018-03-07T14:45:41Z

Is there any good idea to separate the directory where flycheck run command, from working directory?

The working directory is the directory where flycheck runs the command.

But for fixing #1397, the only thing that we need to change is to expand the names relatively to the workspace root, whereas previously they were relative to the working directory. We don't want to change the working directory.

So the change should probably rather go to :error-filter or :error-parser.

kngwyu · 2018-03-07T14:49:35Z

@fmdkdd
Thanks for your advice.
I'll try it.

kngwyu · 2018-03-07T15:34:12Z

I modified to use completely new way.
My idea is very simple:
Absolute Path to workspace root + '/' + Relative Path to error file = Absolute Path to error file

This works well in my test directory, though I'm not sure there are no counter cases.

fmdkdd · 2018-03-08T13:15:02Z

@kngwyu Thanks! That's closer to what the fix should be. I agree that we should expand into an absolute path, only if the workspace root is present.

But doing the expansion in flycheck-parse-rustc-diagnostic will break the plain rust checker, which uses the same parser. I think we should rather do the expansion into absolute paths in the :error-filter of rust-cargo, so we don't mess with parsing and the other checker.

I've pushed some changes in that direction on your branch. I've also fixed the tests, and added a new test for the workspace situation. It works with nightly and stable on my end.

Can you check on your end if the patch still works?

kngwyu · 2018-03-08T13:37:14Z

@fmdkdd Oh, I didn't consider rust checker, sorry, and thanks for your all work!

fmdkdd · 2018-03-10T11:37:29Z

@kngwyu No worries, that's what reviews are for.

I've changed alist-get to let-alist as well, since alist-get is not available in Emacs 24. I think we should be good.

@cpitclaudel Can you take a look?

cpitclaudel

Done! Thanks @kngwyu for this nice PR. I've left a few comments / questions

cpitclaudel · 2018-03-11T01:03:43Z

flycheck.el

       (locate-dominating-file buffer-file-name "Cargo.toml")))

+(defun flycheck-rust-cargo-metadata ()
+  "Exec 'cargo metadata' and return the result as parsed JSON object.


I'd say Run rather than Exec, but that's nitpicking :)

cpitclaudel · 2018-03-11T01:04:51Z

flycheck.el

+(defun flycheck-rust-cargo-metadata ()
+  "Exec 'cargo metadata' and return the result as parsed JSON object.
+
+Return nil if the command failed or if a parse error occured."


Do we really want to fail silently for failed calls to cargo metadata and for parse errors? (IIUC this is for backwards compatibility, but I'm afraid it'll create harder-to-debug bugs if things change unexpectedly)

I think it's only because 'cargo' 's bug if parse error occurs, so we can allow error here, but I'm not sure.

cpitclaudel · 2018-03-11T01:05:05Z

flycheck.el

+  "Exec 'cargo metadata' and return the result as parsed JSON object.
+
+Return nil if the command failed or if a parse error occured."
+  (let ((metadata-lines


-when-let here would remove the need for the and below

cpitclaudel · 2018-03-11T01:08:04Z

flycheck.el

+           (process-lines "cargo" "metadata" "--no-deps" "--format-version" "1"))))
+    (and metadata-lines
+         (ignore-errors
+           (json-read-from-string (string-join metadata-lines ""))))))


That call to string-join should use \n, not ``, I think (though it might not matter in this particular case?)
Besides, would it read nicer to use call-process and `json-read` instead of `process-lines` + `string-join` + `json-read-from-string`?

In this case, we may just use flycheck-parse-json?

@fmdkdd
flycheck-parse-json returns a list of parsed JSON lines, so isn't it too complex for this case, where what we want is only metadata ?

It may be overkill, yes. Using call-process and json-read directly is fine :)

cpitclaudel · 2018-03-11T01:08:57Z

flycheck.el

+Return nil if the workspace root does not exist (for Rust
+versions inferior to 1.25)."
+  (let ((metadata (flycheck-rust-cargo-metadata)))
+    (let-alist metadata


No need to bind metadata here, I think ((let-alist (flycheck-rust-cargo-metadata) …))

kngwyu · 2018-03-11T03:51:21Z

Thanks for review.
I modified a bit, but for flycheck-rust-cargo-metadata I'm not sure how we should handle errors and parse json.

kngwyu · 2018-03-12T14:20:34Z

I simplified flycheck-rust-cargo-metadata and changed not to use ignore-errors in this function.
The newest option --no-deps in cargo metadata --no-deps --format-version 1 was added 2 years ago, so backwards compatibility is OK.
Thus this function never fail unless cargo doesn't exist or breaking change is done.

fmdkdd

Looking good! Just a small documentation change.

fmdkdd · 2018-03-12T21:10:12Z

flycheck.el

+(defun flycheck-rust-cargo-metadata ()
+  "Run 'cargo metadata' and return the result as parsed JSON object.
+
+Return nil if the command failed or if a parse error occured."


I guess this line can go now that we don't ignore errors anymore.

Oh and it looks like the file is misformatted. Please run make format, which should take care of the issue.

kngwyu · 2018-03-13T03:02:22Z

Modified.
Thanks for your review!

fmdkdd · 2018-03-13T07:31:40Z

flycheck.el

  "Run 'cargo metadata' and return the result as parsed JSON object.

-Return nil if the command failed or if a parse error occured."
+We don't ignore any errors in this function."


I don't think we need to spell out that we don't do any error catching, as that is a detail of implementation. Anyone caring about implementation can see in the code that no error checking is done.

Please remove this line :)

kngwyu · 2018-03-13T07:50:37Z

Modified.

Due to a recent change to cargo [1], in a workspace setting (multiple crates) filenames are now relative to the workspace root, whereas previously they were relative to the crate. This commit uses `cargo metadata` to figure out the workspace root, if it exists. If it doesn't, it fallbacks on the manifest directory. [1]: rust-lang/cargo#4788

fmdkdd · 2018-03-13T11:48:08Z

Great! Thank you very much for this patch! 👏

fmdkdd force-pushed the issue-1397 branch from 382e0e4 to 8356ad5 Compare March 8, 2018 13:07

cpitclaudel reviewed Mar 11, 2018

View reviewed changes

fmdkdd requested changes Mar 12, 2018

View reviewed changes

fmdkdd approved these changes Mar 13, 2018

View reviewed changes

fmdkdd reviewed Mar 13, 2018

View reviewed changes

fmdkdd force-pushed the issue-1397 branch from d111c9a to a11c313 Compare March 13, 2018 11:41

fmdkdd merged commit 1e4ec2b into flycheck:master Mar 13, 2018

fmdkdd mentioned this pull request Mar 13, 2018

Checking doesn't work for workspace projects. flycheck/flycheck-rust#61

Closed

Uh oh!

Changed flycheck-rust-manifest-directory to use 'cargo metadata' #1422

Changed flycheck-rust-manifest-directory to use 'cargo metadata' #1422

Uh oh!

Conversation

kngwyu commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmdkdd commented Mar 7, 2018

Uh oh!

kngwyu commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kngwyu commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmdkdd commented Mar 7, 2018

Uh oh!

kngwyu commented Mar 7, 2018

Uh oh!

kngwyu commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmdkdd commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kngwyu commented Mar 8, 2018

Uh oh!

fmdkdd commented Mar 10, 2018

Uh oh!

cpitclaudel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kngwyu commented Mar 11, 2018

Uh oh!

kngwyu commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmdkdd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kngwyu commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kngwyu commented Mar 13, 2018

Uh oh!

fmdkdd commented Mar 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

kngwyu commented Mar 7, 2018 •

edited

Loading

CLAassistant commented Mar 7, 2018 •

edited

Loading

kngwyu commented Mar 7, 2018 •

edited

Loading

kngwyu commented Mar 7, 2018 •

edited

Loading

kngwyu commented Mar 7, 2018 •

edited

Loading

fmdkdd commented Mar 8, 2018 •

edited

Loading

kngwyu commented Mar 12, 2018 •

edited

Loading