Page MenuHomePhabricator

Add a ShellCheck linter
AbandonedPublic

Authored by joshuaspence on Feb 3 2015, 8:25 PM.
Tags
None
Tokens
"Doubloon" token, awarded by shepting."Love" token, awarded by adamchainz."Love" token, awarded by Kentzo."Love" token, awarded by johnny-bit."Like" token, awarded by epkugelmass."Doubloon" token, awarded by thoughtpolice.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

Adds a ShellCheck linter.

Test Plan

Added unit tests.

Diff Detail

Repository
rARC Arcanist
Branch
master
Lint
Lint OK
Unit
Unit Test Errors
Build Status
Buildable 5164
Build 5182: [Placeholder Plan] Wait for 30 Seconds

Unit TestsFailed

Excuse: Unrelated, will check on this
TimeTest
7,936 msArcanistXHPASTLinterTestCase::testLinter
762 msArcanistCSSLintLinterTestCase::testLinter
149 msArcanistCSSLintLinterTestCase::testVersion
112 msArcanistChmodLinterTestCase::testLinter
256 msArcanistClosureLinterTestCase::testLinter
View Full Test Results (1 Failed · 49 Passed · 8 Skipped)

Event Timeline

joshuaspence retitled this revision from to Add a ShellCheck linter.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence edited edge metadata.

Make it work

joshuaspence edited the test plan for this revision. (Show Details)Feb 11 2015, 9:07 AM
avivey added a subscriber: avivey.Feb 18 2015, 7:30 AM
epkugelmass added a subscriber: epkugelmass.

Bump, any love for this?

johnny-bit added a subscriber: johnny-bit.
epriestley requested changes to this revision.Apr 6 2015, 1:38 PM
epriestley edited edge metadata.
epriestley added inline comments.
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.)

This revision now requires changes to proceed.Apr 6 2015, 1:38 PM
cburroughs added inline comments.Apr 6 2015, 4:01 PM
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.

joshuaspence marked an inline comment as done.Apr 6 2015, 9:29 PM
joshuaspence added inline comments.
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.

joshuaspence edited edge metadata.

Remove support for STDIN

Kentzo added a subscriber: Kentzo.

I'm looking forward to integrate this in our product!

epriestley requested changes to this revision.May 19 2015, 2:44 PM
epriestley edited edge metadata.

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);
}
// ...
This revision now requires changes to proceed.May 19 2015, 2:44 PM
kaya added a subscriber: kaya.Mar 22 2016, 5:25 PM

Any news on this?

figroc added a subscriber: figroc.Feb 24 2017, 3:44 AM
adam93 added a subscriber: adam93.Mar 6 2017, 2:32 PM

Also interested in this, would be greatly useful for our (unfortunately) large shell components. Any updates?

avivey added a comment.Mar 6 2017, 3:05 PM

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.

dereckson added a comment.EditedMar 14 2017, 11:52 AM

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'));