What is a lint rule made of?

[a-frantz]

Below are my thoughts on the necessary components of a lint rule. Ideally, whoever proposes a new rule would fully define each of the below pieces. Together, they can be implemented into a lint rule.

• The title is a name for the rule, in snake_case
• The subject is a one sentence summary of the rule
• The body is a full-text description of the rule. It contains all the details needed to understand, comply with, and enforce the rule.
• The fix is optional and is a helpful message to the user about how to comply with the rule
• At least one tag is required and groups similar rules together. The purpose of a tag is to organize documentation and make it easy for users to toggle categories of related rules on/off. There is no limit to how many tags may be associated with a rule.
  • examples: spacing, naming, sorting, completeness, container, and deprecated
• The level is one of Low/Medium/High.
  • Low means the rule is arbitrary or a matter of style
  • High means what the user has written may have unintended consequences
  • Medium is everything else. This is commonly issues of clarity, portability, etc.
• The examples are optional and are either "Good" or "Bad"

Ideally, everything listed here would make it into Rust; either as literal code or as part of doc strings. We should aim for having documentation which is parsed from the code. The code will be a dependable source of truth.

[a-frantz]

@peterhuene @adthrasher I want to discuss the implementations for subject, body, and fix. The most straightforward implementation is that each Rule will have static text for these 3. I don't see any reason to complicate it further. There will be different conditions under which those 3 text fields are shown to the user. Those different conditions will inform how exactly we should define these fields.

Above, I say subject should be one sentence, but I think that slightly misses the mark because it leaves open the potential for run-on sentences that defeat the purpose of the field. We want subject to describe the issue in as few characters as possible so it can be easily displayed on one terminal line or in a small text box within an IDE. When asking rule or code contributors to write a subject I think the language we want to use is a brief sentence fragment describing the core issue. Should there be a hard character limit?

body I think would be more-or-less hidden from users under most conditions. I envision a sprocket explain <lint code or lint name> command which will write out the body of a lint rule. body would also form the bulk of the text in downstream documents/webpages/wikis/etc. The length of a body is going to almost entirely depend on the complexity of a given rule. I'm not sure we can provide much more guidance on length, other than what I say above (It contains all the details needed to understand, comply with, and enforce the rule.)

I'm not entirely sure how to define fix. One sentence should suffice in most cases, right? Is there a length that is too long? Does every rule need a fix?
For now, I think fix should always be a complete sentence (as opposed to subject which can be a sentence fragment to reduce character count) and every rule should have one. We can reassess if this proves difficult for any future rule. IDK, I don't feel confident on this one. Open to other thoughts.
EDIT: I've realized that how we are currently using fix in the code is better than what I propose here. fix is best when it's context-dependent. There may be more than one fix for any given rule. The fix communicated to the user will depend on the user's code. See the code for the snake_case rule for a good example.
My gut says fix details can be ignored during the discussion phase of a rule proposal, and only needs to be addressed during the code implementation. But should it be required in all cases where a lint is thrown? I think for now we require it, and we can always revisit that requirement.

When we come to an agreement on this, I'll start a PR with whatever changes we decide. I will also update the PR/discussion templates and any other documentation to reflect the decisions reached here.

Note, I have intentionally left out the discussion of examples. I think those will be much more complicated to turn into Rust code than these 3 aspects of lint rules. So I'm just kicking that can down the road 🤷

[peterhuene]

Adding my thoughts to the above as I'm now consolidating the lint rules and migrating them to the new AST representation.

I think there should be separation between a rule and the diagnostics a rule can output, as it shouldn't need to be 1:1.

Here's what I think is relevant to a rule:

• A snake case id or name that uniquely identifies that rule for a particular version of WDL; a rule's id will show up in linting configuration and possibly comments in the source to disable the rule.
• A description or subject of the rule that is a single sentence describing the rule's purpose; this might be displayed in a list of available rules, for example.
• An explanation or body that is the long-form in-depth description of the rule and why it exists; this might be displayed in a command like sprocket explain.
• A set of tags that categorize the lint rule; these can be used to filter the rules to run and displayed in commands like sprocket explain.
• An optional url to link to more information about the rule, including examples and fixes; the url can also be printed if the rule emits any diagnostics, allowing users to easily click the lint in a terminal for more information.

I think we should create an mdbook for the rules, put it up on GitHub pages, and link to them from our rules via url too.

Here's what I think is relevant to a rule diagnostic, modeled off of other Diagnostic representations in the wild:

• A snake case rule to correlate the diagnostic with the rule that is emitting it.
• A severity level, which is typically error, warning, or note; I think that this better encapsulates the existing high, medium, and low levels in that: errors are definite problems (a missing runtime section, for example), warnings are potential problems, and notes are more style than substance; this also allows for the diagnostics to colorize differently and not treat all diagnostics as warnings like we currently do.
• A message that is a single sentence describing the diagnostic.
• An optional fix describing the way to fix the problem as a single sentence.
• An optional set of labeled spans with exactly one in the set marked as "primary"; these will allow the diagnostic to annotate the source with messages; the primary span and will be used as the location of the diagnostic when printing a "short form" message.

Currently our diagnostic formatting is fairly verbose:

warning: <version>::<code>::[<tags>]::<level>: <message>
<optional-labeled-spans> (we currently just repeat the same message for all spans)
<optional-fix>
<see-also-sprocket-explain>

I think we can remove version entirely as it's not relevant to users and the with the new parser, one doesn't have to specify the version of the file you're linting, it just automatically runs the appropriate lint rule set based on the version of the file that was parsed.

The code is numerical and not particularly helpful as we already have the id of the lint and (hopefully) a url users can click on for more information; there's a reason that clippy doesn't emit numerical codes as they're not easy to search and inherently have no meaning when you see one. Additionally, we have to maintain in the source that they're unique, which is burdensome as the list grows.

Including the tags, as it is a list, really makes the message unwieldy and requires quite a bit of eye travel to get at relevant information for users (i.e. "oi, what's all this then?"). I really think we should not include this in diagnostic messages.

The level, to me, is synonymous with existing severity in codespan_reporting and redundant.

Therefore, I propose we use a long format like:

<severity>[<rule-id>]: <message>
<optional-labeled-spans>
<optional-fix>
<optional-see-this-url-for-more-information>
<see-also-sprocket-explain>

and a short format like this (which corresponds to DisplayStyle::Short in codespan-reporting:

<optional-primary-location>: <severity>[<rule-id>]: <message>

[a-frantz]

I'm changing directions. Hot take inbound: 🔥

If we have sprocket validate and sprocket lint, I would like the default behavior of lint to be "yell at the user when any lint is violated". If you don't want to be yelled at, try sprocket validate or sprocket lint --tolerant (so your CI stays green but prints every warning).

Isn't our ultimate goal to get people adopting our lints? Why are we writing these if we expect people to ignore them? Let's yell at people 😂 Breaking their CI when they violate our rules is a good thing. We are trying to drive adoption! Let's be bold.

[peterhuene]

I'm onboard with that approach 👍

That said, I do like check ("check that your source is valid") over validate ("validate your source"); I don't think I can articulate a reason why, though.

One thing to think about though is whether or not a check command would ever warn; currently all the diagnostics emitted by parsing and validating are errors and should non-zero exit, but what if it warned for things like an unused declaration, which historically is a diagnostic more in the realm of the compiler than the linter.

[a-frantz]

I'm leaning toward validate because there already exists miniwdl check which does the same thing. I want to differentiate us from miniwdl, but IDK maybe using the same vocab as miniwdl is a good thing?

EDIT: similarly I've thought instead of (or in addition to) sprocket lint --fix we could have sprocket fmt, in the same vein as cargo fmt. IDK if matching existing tools is good or bad, but I feel like it's probably bad? No reasoning for that.

[adthrasher]

I'm leaning toward validate because there already exists miniwdl check which does the same thing. I want to differentiate us from miniwdl, but IDK maybe using the same vocab as miniwdl is a good thing?

FWIW, womtool uses validate. So either way we're overlapping one of the two main players.

[a-frantz]

One thing to think about though is whether or not a check command would ever warn

By default, I think every diagnostic emitted by check/validate/whatever would be an error and result in a non-zero exit code. Any violation of the spec should be considered as meaning "your code is broken and can't be run." Is there ever a situation where that meaning should correspond to a zero exit code?

but what if it warned for things like an unused declaration, which historically is a diagnostic more in the realm of the compiler than the linter.

Hmmm... That's an interesting question. IDK if I can articulate why, but in my mind check/validate would only be concerned with violations of the WDL specification. But should lint cover unused declarations??? Is that OOS for a linter? I don't think it goes anywhere else, and we don't want to introduce another command just for these compiler-esque-warnings, so I think they have to be folded into lint... Not a perfect fit, but I think it's ok?

---

FWIW, womtool uses validate. So either way we're overlapping one of the two main players.

wompwomp