RFC: nimlint: automate enforcing best practices in code

timotheecour commented 3 years ago

goal: a tool that would increase automation, improve quality of existing code and PR's, decrease time spent by reviewers in reviewing PR's.

This would use compilerapi.nim to give same access to code as what nim has, or be built into nim if it's simpler to do so (like --styleCheck on steroids).

This would be usable both as a library and as cmdline: as cmdline, it'd accept all options accepted by nim, eg:

nimlint -d:foo -b:js bar.nim

As library code, TBD.

Some small number of false positive recommendations are acceptable

The linter should be customizable (in particular to suite ones needs in their third party code).

lint items

[x] code block => runnableExamples
[ ] rst code blocks without a test => recommend using a test (eg :test: "nim c $1"; perhaps with :status: 1)
[x] proc + noSideEffect => func
[x] assert in a test file => doAssert
[x] isMainModule in stdlib => recommend moving to tests/stdlib/tfoo.nim
[x] double backticks => single backtick
[x] capitalize the first letter
[ ] lots of testament specific checks (eg exitcode: 0 usually useless)
[ ] etc, countless things that would be best done as a tool instead of relying on every reviewer to check all of those in every PR (plus for making it easier to improve stdlib + third party projects)
[ ] check for presence of multiple yield statements in inline iterators, which cause code bloat
[ ] see also: https://nim-lang.github.io/Nim/contributing.html#best-practices
[ ] naming style https://nim-lang.github.io/Nim/nep1.html

features

[ ] syntax to ignore lint recommendations, analog to #!nimpretty off (maybe via a pragma or special #!nimlint:off syntax)

extension: nimfix

nimfix could be revived but it's a more complex problem than simply reporting issues (where false positives aren't as bad as bad fixes), so can be discussed separately; can be combined with nimpretty to enforce style

/cc @xflywind what do you think? feel free to add to this lint items list

links

ringabout commented 3 years ago

Sounds good to me, it could be integrated into Github APP in the future.

timotheecour commented 3 years ago

@xflywind IMO this should be integrated in the compiler, enabled by a new nim flag --lint:lintoptions.... This avoids having to duplicate work already done by the compiler, stays in sync with latest compiler features, and works with all existing nim flags.

That's the only way to have a robust tool that allows arbitrary lint results, in particular the most interesting ones require semantic analysis.

Using the compiler as a library (compilerapi) would be either very restrictive or require a lot of duplicating of the logic in the compiler. I can give examples if needed.

Implementation wise, it actually doesn't sound too difficult, the general idea would be:

add a lint flag , eg: nim c --lint:lintopts main.nim
throughout compiler code, add when defined(nimLint): lintReport(...) (note that there's already a lintReport proc for styleCheck, we can reuse/extend that or create a different proc)

Furthermore, it would be nice to enable user defined lint checks, for example testament could define its own lint checks for tests (eg assert vs doAssert), that remains TBD for how to do that but via macros would be nice.

note

a flag --lint main seems more flexible than a nim lint main command because it works with all commands, eg nim doc --lint main could be useful

ringabout commented 3 years ago

see https://github.com/nim-compiler-dev/lint Work in progress.

ringabout commented 3 years ago

Releated: https://github.com/nim-lang/Nim/pull/14728 But a bit out of scope, since we have strictfunc, It's better to test them by hand.

arkanoid87 commented 3 years ago

would it be possible to add to the linter responsibility to track down noSideEffects escape hatch usages in project? Like the cast(noSideEffect) pragma. It would ensure noSideEffect property when importing nim modules, and also improve it's usage when developing libs.

timotheecour / Nim