Clarity linter

[hugoclrd]

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 constant (warning)
  • unused function parameter (warning)
  • unused let binding (warning)
  • unused private function (warning)
• unsafe
  • avoid unwrap-panic (warning)
  • avoid unwrap-err-panic (warning)
  • prefer native function aliases (introduced in clarity 2 and versions to come)
    • prefer element-at? (instead of element-at) (warning)
    • prefer index-of? (instead of index-of) (warning)
    • prefer bit-xor (instead of xor) (warning)
• convention
  • invalid naming case (info)
• misc
  • expression with no effect (warning)
  • public function that could be read-only

[andresgalante]

The first step for this task is to create a spec and discuss it with the team before diving into the implementation.

Evaluate the Check-checker to see if it's a good place to start building the linter, ideally it should avoid issues like the one ALEX had

[obycode]

To add one request in the convention category, how about conventions about constants for errors:

;; do this
(define-constant ERR_SWAP_NOT_FOUND (err u100))
(unwrap! (foo) ERR_SWAP_NOT_FOUND)
;; not this
(define-constant ERR_SWAP_NOT_FOUND u100)
(unwrap! (foo) (err ERR_SWAP_NOT_FOUND))

[jbencin]

My thoughts after looking at the check-checker code...

How "Check checker" Works

CheckChecker implements the ASTVisitor trait. Using ASTVisitor allows an external function to walk the AST, and CheckChecker can register callbacks for only the types of AST nodes it is interested in. It's a simple way to interact with the code without doing any parsing or tree-walking ourselves

Plan for Creating a Full Linter

Implementing ASTVisitor, as in check-checker, gives us the following:

• It allows full access to the Clarity code as an AST
• It allows each lint to maintain it's own internal state

I think this should be sufficient to implement any lint, so here is my proposal for refactoring and allowing additional, arbitrary lints

• Make new Clarinet component, linter, under components/linter
• Make new directory for lints, components/linter/lints. Each lint will be a module under this directory and must implement ASTVisitor. Also, move CheckChecker here
• Create the linter in mod.rs, which will:
  • Process the CLI args and decide which other lints to include at runtime
  • Maintain a list of enabled lints, Vec<Box<dyn ASTVisitor>>
  • Implement ASTVisitor itself in order to walk the AST, and on each node visit will invoke the callbacks for each enabled lint

[hugoclrd]

Looks good to me, thanks @jbencin I think the the LSP integration will also be a priority (both CLI and LSP)

[jbencin]

how about conventions about constants for errors:

Another related lint: If you have two error constants with the same value (real example I just found in my own code):

(define-constant ERR_TT_CHECK_RECIPIENT_NTT_MANAGER (err u5221))
(define-constant ERR_TT_CHECK_SOURCE_NTT_MANAGER (err u5221))

[jbencin]

Another one I just encountered where a lint would have been helpful: Unnecessary as-max-len?, where the length of the buffer returned is the same as the input buffer

[jbencin]

I ran into this today, and I'm not sure if it should be a lint or fixed at the VM level. Apparently it's legal to do is-eq with a single argument and it will always return true:

clarity-repl v3.6.1
Enter "::help" for usage hints.
Connected to a transient in-memory database.
>> (is-eq true)
true
>> (is-eq false)
true

Seems pointless to allow this, as there's no use for it and it can only be a potential source of bugs

[hugoclrd]

Yes very good point! It would be great for the formatter to catch all no-ops ((is-eq true) (+ 1))

Not so long ago, the formatter helped with a code that was looking like that (I can't find in which project but that was the idea)

   ;; not formatted
    (if (and (long-expression-call))
             (other-long-exression-call (some arg)
      (do-this))
      (do-that)
    )

   ;; formatted
   (if (and (long-expression-call))
      (other-long-exression-call (some arg) (do-this))
      (do-that)
    )

Where the (and (long-expression-call)) is a no-op, but because of closing parenthese error, it was hard to catch. So the linter could definitely help avoid that.

I think (again) that it's also something that could be fix in the clarity-vm itself, because all of these variadic functions are really footguns when called with just one argument (+ is-eq and or etc)

[jbencin-stacks]

I'm starting work on finding unused variables. In addition to what's in the description, these also have the potential to be "unused::

• use-trait
• define-data-var
• define-map
• define-fungible-token
• define-non-fungible-token

I'm leaning towards these all being separate lints, but what do you guys think?

Also @hugo-stacks can you edit the issue description to include everything in the thread so far? I don't have permission

[hugo-stacks]

@jbencin-stacks Let's create a new one and close this one. So that you can fully own it It was created with my personal account anyway.

[brice-stacks]

We may want to add some kind of magic comment to allow an unused private function, for example if it is used in tests only (unless there is some better way planned to have test-only code).

[hugo-stacks]

@brice-stacks I'd love to have your opinion regarding https://github.com/stx-labs/clarinet/issues/2022

[hugo-stacks]

Also, we could adopt the leading _ convention for "allowed unused code"

[jbencin-stacks]

@hugo-stacks please close, #2028 replaces this issue

[hugo-stacks]

Thanks @jbencin