Clarity Linter (updated)

[jbencin-stacks]

Replaces #1792

Description

Clarinet currently checks Clarity code for errors, but Clarity also needs a full-featured linter, similar to eslint or clippy, which will flag code that is valid, but does not follow best practices or conventions

Config File

I plan to allow the user to configure lints like this, by specifying the level for each one:

[repl.analysis.lints]
noop = "ignore"
unused_let = "warn"
unused_const = "error"

Eventually, I'll want to rename check_checker and call_checker and move them into the lints section

If any lints need additional config it can be supported like this (I don't know if it will be necessary):

unused_const = { level = "error", some_option = "value" }

Planned Lints

Add a linter to the LSP with the possibility to easily add rules over time. The first few essential rules could include:

• Dead code
  • [ ] Unused function parameter
  • [x] Unused private function (unused_private_fn)
  • [ ] Unused let binding
  • [x] Unused define-constant (unused_const)
  • [ ] Unused use-trait
  • [x] Unused define-data-var (unused_data_var)
  • [x] Unused define-map (unused_map)
  • [ ] Unused define-fungible-token
  • [ ] Unused define-non-fungible-token
  • [ ] Unnecessary as-max-len?
• Safety
  • [ ] Use of unwrap-panic
  • [ ] Use of unwrap-err-panic
  • Prefer native function aliases (introduced in clarity 2 and versions to come)
    • [ ] Prefer element-at? (instead of element-at)
    • [ ] Prefer index-of? (instead of index-of)
    • [ ] Prefer bit-xor (instead of xor) (warning)
• Style/Convention
  • [ ] Invalid naming case
  • [ ] Multiple error constants with same value
• Misc
  • [x] Expression with no effect (noop)
  • [ ] Public function that could be read-only
  • [ ] Within the body of as-contract?, prefer using current-contract instead of tx-sender

Configuration/Customization

• [ ] Allow prefixing identifier with _ to disable "unused" warnings

Refactoring

I have a couple ideas here

• Renaming and Moving Files: Rename existing files and directories to match new names in config file, and move them all under single lints directory
• Single ASTVisitor: As I described in #1792, we could have a single ASTVisitor through which all enable lints are called. This means we wouldn't have to walk the AST for each individual lint. I'm not going to do this right now, but we can do this later if performance becomes a problem

[linear[bot]]

CNET-155

[hstove-stacks]

This sounds great! IMO it would be better to have the config file let you specify each lint rule as "error", "warn", or "off".

Another thing to consider is the. ability to disable a lint rule for a line or even for the whole file

[hugo-stacks]

A new linter rule idea emerged from a discussion around Clarity 4: Within the body of as-contract?, prefer using current-contract instead of tx-sender. It simply makes readability easier.

It goes with some other clarity version-specific rules (such as index-of? instead of index-of)