Bogdanp / racket-review

A linter for Racket.
BSD 3-Clause "New" or "Revised" License
41 stars 3 forks source link

racket-analysis #5

Open sorawee opened 3 years ago

sorawee commented 3 years ago

I created yet another linter, unaware that your project already existed. It analyzes the expanded code, so it has an access to binding informations, allowing one to discover issues like invalid (cond [else ...]) when else is not bound to else from racket (even though symbolically this looks correct).

My plan was to rewrite the whole thing all over again, because the current code has some fundamental flaws. But seeing that this project already exists, I don't want to make a duplicate effort. Do you plan to support analyzing expanded code? If so, it might make more sense to join the efforts somehow rather than creating yet another separate tool.

By the way, there's another rule from racket-analysis that can be readily added to your current framework. Sometimes people write:

(case s
  ['foo 1]
  ['bar 2])

But this is equivalent to:

(case s
  [(quote foo) 1]
  [(quote bar) 2])

which is probably not what they intend.

sorawee commented 3 years ago

Oh, and another rule is to catch null and empty in:

(match e
  [null 1]
  [(cons a b) 2])

because people are also mistaken that the first case will match the empty list, but in fact it will bind null to any matching value.

Bogdanp commented 3 years ago

I think those would be good to add; I've definitely run into both problems. If you feel like it, you could try adding those rules to get familiar w/ how review works. Be warned, though, the implementation isn't as good as it should be.

The reason I originally implemented this as a surface-level only thing was because I wanted it to be fast, but I think I had a mistaken idea about how slow expanding individual modules on the fly would really be. Since Greg added racket-xp-mode to racket-mode, I've been using it without problems. Although xp-mode is slow enough that I notice it sometimes, it's not a huge deal.

I think that, ideally, a linter like this would have two modes: one where it searches for stylistic issues by only looking at surface syntax, like review does right now (minus the parts where it tries to keep track of scope) and another, deeper mode where it expands things and looks for potential bugs. It'd perform the analyses one after the other and report problems as soon as it finds them, which would preserve the speed for the most part. The split would likely improve the code and it would make it much easier to support custom syntax (which review doesn't do at all right now -- I just have various rules for macros I use often).

If that seems like a good direction to you, I think it'd be great to collaborate.

sorawee commented 3 years ago

I will add the rules to detect both problems tonight.

My immediate goal is to analyze the Racket codebase itself as a part of the CI process, so I think I will keep working on racket-analysis for now, but will focus on backend stuff so that racket-review (and other tools) could process information from it.