koalaman / shellcheck

ShellCheck, a static analysis tool for shell scripts
https://www.shellcheck.net
GNU General Public License v3.0
36.32k stars 1.77k forks source link

Add support for configuring Shellcheck via .editorconfig #2128

Open ghost opened 3 years ago

ghost commented 3 years ago

For new checks and feature suggestions

Scenario

Let's say that there is a project with the following directory setup:

|-> Makefile
|-> bin/main.sh
\-> src/*.sh

Where depending on the options given to the make invocation, different scripts from the src/ directory are combined together to generate bin/main.sh.

In such a scenario, a variable in src/foo.sh may only be referenced in src/bar.sh, meaning that SC2034 may be perfectly acceptable in context, but would need to be suppressed, as shellcheck would generate this warning.

Current configuration options

There are several methods by which this warning may be suppressed:

  1. Command line options
  2. Ignore directives over every instance, in each file
  3. Ignore directives under the shebang, in each file
  4. Ignore directive in the .shellcheckrc file
Option 1: Command line options

This is the simplest option, it solves this issue completely if shellcheck is invoked manually, however, in this hypothetical scenario this would not be sufficient. For example, if both make test and the .travis.yml file call shellcheck, then there are two locations where the command line options are specified, and therefore is a duplication of the code that may be unmaintainable. Furthermore, this option would apply to all files in the project, meaning that unintended violations of ignored diagnostics in other files would be missed.

Option 2: Ignore directives over every instance, in each file

This option simplifies the invocations, as they can remain simply shellcheck <regex>.sh, and also prevents unintended violations from slipping past shellcheck. However, this further exacerbates the issue with option 1, as all variables that are intended to be used by other files would need the directive added above every single line that declares a variable that is not used in that file. This is just as unmaintainable as option 1 for this scenario.

Option 3: Ignore directives under the shebang, in each file

This is slightly better than option 2, as it only requires one directive per file. However, a new contributor to the project may be unaware of this pattern, or unaware of how shellcheck works or what it is. This hypothetical contributor may then add a src/baz.sh without the directive under the shebang, thus breaking automated builds. Or, more consqutially, a well meaning contributor, even a well informed one, may know about shellcheck and how to use the directives, may blindly copy the directives from another file without knowing which directive disabled which issue. In this case, a directive that is unique to one file may leak to another, and thus an issue may not be caught by shellcheck in this new file.

Option 4: Ignore directive in the .shellcheckrc file

This option resolves the maintainability option (assuming it is tracked as part of the project's VCS) and prevents developers from mistakenly missing or including the wrong ignore directives, as per the wiki.

As of v0.7.0 you can create a file .shellcheckrc in your home directory (or your project's base directory), and add disable directives to it.

$ cat ~/.shellcheckrc
disable=SC2059

However, the .shellcheckrc does not (currently) provide syntax for specifying files by filename or regex. This is fundamentally the issue in this scenario.

Solutions

There are two possible methods of configuring which directives to ignore in which files, in a maintainable and version controlled manner:

  1. Updating the .shellcheckrc syntax
  2. Adding support for .editorconfig
Updating the .shellcheckrc syntax

This option really appeals to me as the natural, even "obvious", solution. Since there is already support for a project wide configuration file, it would make sense to extend this functionality to allow specifying certain directives to specific files. However, updating the syntax may affect existing .shellcheckrc files, so the specific changes to the syntax should be backwards compatible.

Adding support for .editorconfig

For an equivalent hypothetical project in C#, for example, a (mostly) analogous warning to SC2034 would be IDE0051, and to disable this warning for specific files would be as simple as adding the directive to the project's .editorconfig:

[src/*.cs]
dotnet_diagnostic.IDE0051.severity = none

So for shellcheck, this hypothetical project could add something like this to their .editorconfig:

shellcheck_SC2034 = disabled

This has three benefits over updating the .shellcheckrc syntax.

  1. It consolidates all linting configurations into one file; .editorconfig, which is widely recognized.
  2. The syntax is already well established, meaning that the .shellcheckrc syntax is free to be updated in the future in a divergent way.
  3. .editorconfig files use the INI format, for which Haskell already has a parsing library, Data.Ini
Freed-Wu commented 1 year ago

Looks like .editorconfig is closer to standard than .shellcheckrc. Is there any progress about this issue?