Added a proper support for cargo --message-format json
#84
Conversation
Massively refactored code, so the error mode behaviour is less dependent on conditions and more polymorphic.
|
@Arcanemagus if you have time and wish, can you please review code I wrote, just as you did at #74? |
|
Sorry, just saw this. Looking over it now! |
| .toEqual('a\r\nb') | ||
|
|
||
| multi = linter.parse('a:1:2: 3:4 error: a\n\rb\n\rx:1:2: 3:4 error: asd\r\n') | ||
| multi = linter.parse('a:1:2: 3:4 error: a\n\rb\n\rx:1:2: 3:4 error: asd\r\n', []) |
There was a problem hiding this comment.
🐛 I'm guessing this should be errorModes.OLD_RUSTC.parse.
| # current dir is set to handle overrides | ||
|
|
||
| sb_exec.exec(@rustcPath, ['--version'], {stream: 'stdout', cwd: curDir, stdio: 'pipe'}).then (stdout) => |
There was a problem hiding this comment.
📝 stdout is the default stream, there's no need to explicitly state it here. The same goes for stdio as 'pipe' is the default.
| else | ||
| false | ||
| throw 'rustc returned inapplicable result: ' + stdout |
There was a problem hiding this comment.
💭 Does "inapplicable" work here? Maybe "unparseable"?
| # current dir is set to handle overrides | ||
|
|
||
| sb_exec.exec(@rustcPath, ['--version'], {stream: 'stdout', cwd: curDir, stdio: 'pipe'}).then (stdout) => |
There was a problem hiding this comment.
💡 This will work just fine, but to avoid calling rustc --version for every lint call, you might want to think about caching the result. For an example here is what I recently put together to cache the PHPCS version as the behavior allowed there is also heavily dependent on version.
There was a problem hiding this comment.
The same can be said for other "check calls", such as here
| [] | ||
|
|
||
| initCmd: (editingFile) => | ||
| curDir = path.dirname editingFile | ||
| cargoManifestPath = @locateCargo path.dirname editingFile |
There was a problem hiding this comment.
🐎 Use the curDir variable you define on the line above here, no need to call path.dirname(editingFile) again.
| elements = [] | ||
| XRegExp.forEach output, pattern, (match) -> | ||
| if match.from_col == match.to_col | ||
| match.to_col = parseInt(match.to_col) + 1 |
There was a problem hiding this comment.
💡 Situations like this are where atom-linter::rangeFromLineNumber is great as it will automatically determine a range highlighting the next "word", as defined by the language. So you can feed it a single point and it will generate a better range than just the single character. It would require some refactoring though as it requires access to the TextEditor of the file being linted.
There was a problem hiding this comment.
@Arcanemagus if I add atom-linter as a dependency, I should probably remove sb-exec, since it's included, should I?
There was a problem hiding this comment.
Correct, atom-linter::exec is a wrapper for sb-exec so there is no need to include it directly.
| parseJsonMessages = (messages, disabledWarnings) -> | ||
| elements = [] | ||
| for input in messages | ||
| continue unless input and input.spans |
There was a problem hiding this comment.
📝 Linter messages don't (currently) require a range, is this ignoring potentially useful messages just because they don't have a span?
| [primary_span.line_start - 1, primary_span.column_start - 1], | ||
| [primary_span.line_end - 1, primary_span.column_end - 1] | ||
| ] | ||
| input.level = 'error' if input == 'fatal error' |
There was a problem hiding this comment.
🐛 Should input == 'fatal error' be input.message == 'fatal error'?
| neededOutput: (stdout, stderr) -> | ||
| stdout | ||
|
|
||
| parse: (output, disabledWarnings) -> |
There was a problem hiding this comment.
🐛 Can the functionality here be merged into parseJsonOutput? They are almost identical.
|
|
||
| buildRustcArguments = (linter, paths) -> | ||
| [editingFile, cargoManifestPath] = paths | ||
| Promise.resolve().then () => |
There was a problem hiding this comment.
🐛 It looks like you are trying to use this as a way to return a Promise, you should just go directly there:
new Promise (resolve, reject) =>
I'd consider this an optional change though I guess, as it makes the CoffeeScript of the function even worse than it is from the start 😛.
|
@Arcanemagus huge thanks for the big and helpful review! The tricky part, although, is caching results, as a user might switch tools |
|
That's why that was just a suggestion and not something that needed to be addressed. As I said it would work just fine as is 😉. |
|
Hmm, so the way I was caching it in other providers I cached the version based on the path to the executable, I was thinking of something along the lines of caching the result here based on the current directory as that should handle changing executables by directory which is what most "executable version management" systems do. Would that not work here? Or does the version get changed often for the same directory? |
|
I hope it's not rude to butt in, I was trying to find a solution for my cargo builds being invalidated all the time, stumbled across this PR, and did some unsolicited testing :) |
| canUseIntermediateJSON = nightlyWithJSON or stableWithJSON | ||
| switch commandMode | ||
| when 'cargo' | ||
| canUseProperCargoJSON = match.nightly and match.date > '2016-10-10' |
There was a problem hiding this comment.
This is a really small deal but I happened to be running 2016-10-10 and it does have "--message-support". Is this the right version number to key off?
There was a problem hiding this comment.
You are right, the right starting version is probably 10-08 or 10-09 since it was merged 15 days ago in cargo. Thanks for the help, I 'll get this fixed :)
| stdout | ||
|
|
||
| parse: (output, options) -> | ||
| options.additionalFilter = (json) -> |
There was a problem hiding this comment.
I think this is a bug, on my machine it was filtering out 100% of errors returned from the compiler. I think that either "json" here should be "input" to match below, or vice versa.
There was a problem hiding this comment.
Whoops, thanks a lot! I wonder how did I miss that one. I will need to write better tests once this is finished.
|
@Arcanemagus well, it's not that common to change toolchains in Rust, but it's something that one can and expect being able to easily do. For example, consider someone coding a library with a stable compiler in Atom with linter-rust, and then switching over to a beta or a nightly toolchain (that's where the override is being set for a project, and toolchains' versions are changed as a consequence) to check if there are any warnings considering any possible stabilizations or deprecations.
There is, actually, no precedent of 'per-directory' toolchain swapping in Rust, as far as I'm aware of. On the other side, I can imagine someone making a large directory of subdirectories that have one All in all, there are several ways to handle caching:
What do you think? |
|
I would vote for option 2, since it sounds unlikely that it would change during a session of Atom being open, at least that's the reasoning I went with for Maybe a note in the readme that if the use changes the version a restart of Atom will be required to catch any additional features (un)available in the new version? Just wanted to say thanks for all the work you've been putting into this provider btw! |
|
I've been following this discussion. :-) I think switching the toolchain during a session is not so rare. I, for example, recently worked on a rust macro library which is supposed to run on rust-stable. However all the nice macro-debugging tools are only available in rust-nightly. Whenever I discovered a hard bug with rust-stable I switched to rust-nightly until I fixed it - and back again, repeatedly. All during one atom session. I think option 2.1 would be a good start, i.e. let the user decide if caching is enabled. From this point on any kind of heuristic can still be added in the future. |
|
What is the performance hit of not caching? |
|
@zofrex Well, to be honest, it's not that big comparing to compilation times :) But It's more a future-proofing thing. Rustc is constantly improving and we are about to have incremental compilation soon. As an example of compilation times improving see this issue (in short, the compilation time went from 30 secs to 5 in a time span of year). Going into details, |
|
@Arcanemagus it's me who should thank you for your reviews and advices! (: Thanks for helping me out here and there. Kinda off-topic, but I'd ask you another linter related question. I understand that most linters do not have big timings on performing a lint, but as you've probably seen in the discussions Rust does suffer from big compilation times. |
|
@colin-kiegel thanks for the input! Yes, I think it's what we should go with. I've been reluctant to add another option, since I know people are sometimes confused with a number of options, but I hope 10 options is not a really big deal.. |
|
I've enabled 'caching forever' to be a default choice with an option in settings called @Arcanemagus if you have no other notices, I believe you can mark your review as a positive. Thanks again for your review. |
|
@White-Oak Linter v2 incorporates the busy-signal package, which has a nice indicator when it is currently running a lint: In fact, it completely removes the "No issues" text. you can check it out following the instructions here: https://github.com/steelbrain/linter-ui-default |
|
@gmist I hope you are not against that I merged this. |
|
of course not against |
|
PS: I would suggest to default to 'no caching' to be on the safe side. Oak notifications@github.com schrieb am Mo., 24. Okt. 2016, 22:36:
|
|
@colin-kiegel default settings are rarely changed, if making 'no caching' be the default choice, then why use caching anyway. |
|
@White-Oak I guess in the end it's a matter of taste. Its ok for me to change the setting, but I would still suggest at least add a little warning in the settings description, that the rust linter will not detect a change in toolchain until restart. Again of course it's your choice. But this discussion reminds me, there may be another "hacky" approach instead of caching. You mentioned that you want to avoid spawning processes for shell commands. It may be possible to exchange a handful of small shell calls plus logic and bundle them into a small shell script instead, which can be passed to only one shell call. Of course the small shell script needs to be interpreted by the shell, but the spawning of processes can be saved. However the details of this approach will most likely be very OS-specific. If it is possible at all it would probably be a bit brittle - but I still wanted to share the thought, just in case... ^^ PS: Thanks for the great work. |
|
@colin-kiegel there is one :) |
|
Maybe this isn't the right place to ask, but: Are the changes in this PR present in version 0.7.0? I am still experiencing the #77 problem with rust nightly from yesterday, and version 0.7.0, with Atom 1.11.2 on Linux x86_64. I just discovered a workaround, which is to compile with |
|
@AaronM04 thanks for the report! This is really weird, and really strange! Can you please as well post a version of rustc? |
|
|
@White-Oak |
|
@AaronM04 yes, it is a stable from when JSON errors were only supported via RUSTCFLAGS. Hope it cleared everything out. Latest stable (1.13), beta and nightly version all support proper JSON errors' support. Hope it helps :) |
|
@White-Oak I switched to the real nightly and everything works. Thanks! |
Massively refactored code, so the error mode behaviour is less dependent on conditions and more polymorphic.
I've tested this in all five modes that
linter-rustcurrently supports:RUSTFLAGS, that caused several problems and only enabled for a short ammount of time from AUgust to October 2016--message-format json, that are introduced by this PRIn short, I split the linter in two files:
linter-rust.coffeeandmode.coffee. The latter is now responsible on mode-specific interactions such as error parsing, arguments generating.Fixes #77.
As far as I tested it, it works, but I, probably, need some review on code.