Page MenuHomePhabricator

Add a ShellCheck linter
AbandonedPublic

Authored by joshuaspence on Feb 3 2015, 8:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 4:13 AM
Unknown Object (File)
Tue, Mar 26, 4:13 AM
Unknown Object (File)
Tue, Mar 26, 4:13 AM
Unknown Object (File)
Tue, Mar 26, 4:13 AM
Unknown Object (File)
Sun, Mar 24, 4:24 AM
Unknown Object (File)
Sat, Mar 16, 6:20 AM
Unknown Object (File)
Thu, Feb 29, 10:32 PM
Unknown Object (File)
Feb 22 2024, 10:37 AM
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 Passed
Unit
Tests Skipped
Build Status
Buildable 4296
Build 4309: [Placeholder Plan] Wait for 30 Seconds

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.
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
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 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 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

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

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.

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