Adds a ShellCheck linter.
Details
Details
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Branch
- master
- Lint
Lint Passed - Unit
Test Failures - Build Status
Buildable 5164 Build 5182: [Placeholder Plan] Wait for 30 Seconds
| Time | Test | |
|---|---|---|
| 7,936 ms | ArcanistXHPASTLinterTestCase::testLinter | |
| 762 ms | ArcanistCSSLintLinterTestCase::testLinter | |
| 149 ms | ArcanistCSSLintLinterTestCase::testVersion | |
| 112 ms | ArcanistChmodLinterTestCase::testLinter | |
| 256 ms | ArcanistClosureLinterTestCase::testLinter | |
| View Full Test Results (1 Failed · 49 Passed · 8 Skipped) |
Event Timeline
| src/lint/linter/ArcanistShellCheckLinter.php | ||
|---|---|---|
| 72–78 | If possible, let's get rid of this per T7674 (basically, favor lint <filename> form because a bunch of linters freak out and behave differently in subtle ways when given lint <filename> vs echo file | lint, and everyone expects the lint <filename> behaviors). | |
| 126–142 | I think this is not correct, and leads to the same issue as D12034 (linter does not respect severity map). Basically: linters should not set severity; instead, they should set a code, and the code should imply a severity via getLintSeverityMap(). This will let users override the severity. (Possibly they may have to call setSeverity($this->getLintMessageSeverity($code)) manually right now, but we should make that implicit if it isn't already.) | |
| src/lint/linter/ArcanistShellCheckLinter.php | ||
|---|---|---|
| 126–142 | As far as I can tell from existing examples, there isn't an implicit setSeverity. FWIW I was struggling with this for D10738 and find it confusing that getDefaultMessageSeverity needs to be overridden less as 'return a constant' and more as parseALinterDependentString. | |
| src/lint/linter/ArcanistShellCheckLinter.php | ||
|---|---|---|
| 126–142 | Right. But the intention of this block of code is not to allow custom severities but rather to respect the external linter severities, i.e. if the external linter flags a warning then we respect this severity. | |
Comment Actions
Here's a test case to illustrate my thinking:
#!/bin/sh `backticks are cool` ~~~~~~~~~~ error:2:1 error:2:1 ~~~~~~~~~~ ~~~~~~~~~~ { "config": { "shellcheck.shell": "bash", "severity.rules": { "(.*)": "error" } } }
I assert that this test case should pass. It does not, because messages are raised at the wrong severity (the configured severity is ignored completely).
My claim is:
- This test case should pass.
- severity and severity.rules should be respected.
- These settings should be stronger than the external linter's defaults.
This is an issue we've hit several times now (D12034 is another example); I think we should make it much harder for linters which do have these properties to be written. I believe the current expectation is that well-behaved linters must call $message->setSeverity($this->getLintMessageSeverity($code)); explicitly. Many of them do, but some don't and this isn't obvious and is easy to get wrong.
Instead, this should be called implicitly, and before the severity is set the caller should probably verify that the linter did not try to set itself. e.g. a block that looks roughly like this should exist somewhere in the parent:
// ...
$messages = $linter->getMessages();
foreach ($messages as $message) {
if ($message->getSeverity() !== null) {
throw new Exception(pht('...'));
}
$code = $message->getCode();
$severity = $linter->getLintMessageSeverity($code);
$message->setSeverity($severity);
}
// ...Comment Actions
Also interested in this, would be greatly useful for our (unfortunately) large shell components. Any updates?
Comment Actions
See T10038 - the most likely way to move this forward is by creating an extension from it and adding to Community Resources, and each interested party to install it separately.
Basically no new linters are accepted upstream right now, and even bugfixes are rare.
Comment Actions
To package it as a library is a good idea, but code isn't up to date with current Arcanist requirements. See T9316 for more context.
[2017-03-14 11:50:32] EXCEPTION: (Exception) Linter "ArcanistShellCheckLinter" generated a lint message that is invalid because it does not have a name . Lint messages must have a name. at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:620] arcanist(head=accept, ref.master=3b6b523c2b23, ref.accept=3b6b523c2b23), phutil(head=master, ref.master=5e14a5b17809), shellcheck-linter(head=master, r ef.master=c56d9d5d29db) #0 ArcanistLintEngine::validateLintMessage(ArcanistShellCheckLinter, ArcanistLintMessage) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.ph p:220] #1 ArcanistLintEngine::run() called at [<arcanist>/src/workflow/ArcanistLintWorkflow.php:334] #2 ArcanistLintWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]
Would be nice to breakdown according the code, as they are categorized, but meanwhile, a minimal solution could be:
--- a/lint/linter/ArcanistShellCheckLinter.php +++ b/lint/linter/ArcanistShellCheckLinter.php @@ -111,6 +111,7 @@ final class ArcanistShellCheckLinter extends ArcanistExternalLinter { ->setPath($path) ->setLine($child->getAttribute('line')) ->setChar($child->getAttribute('column')) + ->setName($this->getLinterName()) ->setCode($code) ->setDescription($child->getAttribute('message'));