it would be nice if there was some way to mark a form as ignorabe by a given linter

[hiredman]

I have a number of compojure routes that look like

(defroutes app
  (GET "/foo" [] #'foo)
  (GET "/bar" [] #'bar))

over this form the unused-fn-args complains about some macro generated fun arg not being used, and in the long term it would be nice to get eastwood not to complain about that some how, in the short term it would be great to be able to signal eastwood "I've manually looked at this, and it is fine"

one possibility is some kind of marker macro, but those have issues, and mean you need a runtime component.

maybe something like:

^{:eastwood {:ignore #{:unused-fn-args}}
(defroutes app
  (GET "/foo" [] #'foo)
  (GET "/bar" [] #'bar)))

[jafingerhut]

Agreed that it would be nice to have some way to signal to eastwood that particular chunks of code should have some kinds of warnings disabled.

Right now we are in the middle of getting eastwood updated from the older analyzer jvm.tools.analyzer to the newer tools.analyzer / tools.analyzer.jvm. Yeah, those names look similar, but a fair amount has changed under the hood. We have a bit more testing and bug fixing to do before it will be ready to release, but hopefully some time in Dec 2013. We should be able to look at this request after that release.

[jonase]

I guess you're already aware of the :exclude-linters option?

I'm not sure I like the idea (in general) that a linting tool requires users to add annotations to the source code to help it along. (It might be a good idea, i'm just not sure...)

The :unused-fn-args linter has come up in a few bug reports so another option is to move it out of the default set of linters.

[hiredman]

yeah, I mean, what is the point of a linter if you just annotate everything as ignorable by the linter, the thing is, there is an unused binding introduced by a macro in the macro-expansion of that code, but it isn't even visible in the user code, so is it a problem? how should a linter deal with that? how can it tell?

[jonase]

Here's a simple heuristic that I think would work pretty well in these cases: Collect all symbols from the (non-macroexpanded) top level form. If the symbol does not appear in collected the set, don't issue a warning. Does this sound like reasonable approach?

[hiredman]

@jonase in that case the linter won't be useful if you have a custom binding macro

[hiredman]

another example here is if the misspelled keyword linter thinks a keyword is misspelled, but it isn't

[jonase]

The misspelled keyword linter is a great example where hints in the source code are ..ehm.. not very useful

^{:eastwood {:ignore #{misspelled-keyword}}} :foo

Also, users must accept some false positives (and trust their own judgement instead of blindly following the tools "advice"). If the number of false positives exceeds the number of valid warnings it's a clear indication that maybe that linter wasn't a great idea to begin with.

(I didn't quite understand the part of "a custom binding macro" -- do you have an example?)

[jafingerhut]

I am interested in the idea of disabling specific warnings in Eastwood, without having to disable an entire category of warnings in an entire namespace. I am curious as to what others thought about how to implement such a thing.

I have used lint tools in C and other languages where there were specially formatted comments just before the code where you wished to disable a lint warning. One implementation difficulty with doing so for Eastwood is that we would need a reader that preserved comments somehow in the result of the read. The current tools.reader discards comments.

Even if the reader preserved comments, there is the question of how to represent those in the result of reading the code. Comments can occur in pretty much arbitrary places in the resulting data structure, so metadata couldn't do it.

An alternate method instead of comments would be to add metadata to Clojure expressions, e.g. like this:

(defn my-fn [a b c]
   (let [x (+ a b)]
      ^{:eastwood {:exclude-linters [ :def-in-def ]}}
      (def bar (* x c))
      (- b c)))

tools.reader will preserve this metadata, and I could imagine Eastwood being enhanced to pay attention to this metadata to control which subexpressions it issues warnings, or is presented from doing so.

Thoughts?

[Bronsa]

tools.reader has a source-logging-push-back-reader that attaches source info the the forms being read which eastwood could use to extract info from comments.

That said, there would be absolutely no advantage in using that over using the metadata approach since it would suffer the same limitation (the source info would be stored as metadata on the next form) and would only add complexity in that it would require some parsing of the source info to extract any eastwood configuration

[hiredman]

@jafingerhut metadata to skip linters for some forms sounds good to me

[gfredericks]

This is definitely a major usability issue for me, because I would like to be able to maintain a warning-free codebase (so that I can completely ignore the linter unless there's a warning, e.g. by running it in CI) without having to disable entire categories of linting just because of one edge case.

For example, I just ran eastwood on a codebase from work and got four errors:

• 2 false positives from calling clojure.java.jdbc/insert! with acceptable arguments, due to that function having unconventional :arglists metadata
• 1 false positive from a form such as (-> x (f) (or y)), warning about calling or with one arg
• 1 legit warning about (merge {...}).

As I said, eastwood would be most useful to me if I can throw it into my CI script and forget about it; but currently I can't do that without dealing with the three false positives somehow. One of those would need a patch/release to java.jdbc (or at least me monkey-patching it), and the other would require a refactoring that I'm not too interested in doing just to please a linter. The (or y) problem can probably be solved by modifying the linter, which I support, but would like to be able to get a warning-free codebase in the meantime without having to refactor.

[gfredericks]

@jafingerhut what's the status of the analyzer porting work you mentioned in December? And does anybody have a concrete idea of how much work is involved in doing the metadata approach? I haven't dug into the eastwood codebase yet.

[jafingerhut]

@gfredericks Eastwood has been modified to use Nicola's tools.analyzer(.jvm) since the Jan 2014 release of version 0.1.0. It has been modified in later releases to keep up with changes he has made in the library since then -- often he makes those Eastwood changes himself.

This issue is getting near the top of my personal list of Eastwood changes that I'd like to see done, for the reasons you mention. There will likely always be false positive warnings from Eastwood, i.e. not bugs in the code, but they are similar to other things that are bugs.

That said, I don't want to make any promises of when I will have time to work on it. It may be within a month.

[amalloy]

Any news on this? eastwood is pretty cool, but without some way to say "yes, I know what I'm doing in this case" it is not very useful, for the reasons @gfredericks mentions.

[Bronsa]

I'll be looking at adding metadata annotations to disable per form linters this week, if @jafingerhut doesn't beat me to it.

[jafingerhut]

@Bronsa If you get to it that soon, it would take a miracle for me to beat you to it. Still trying to get my head above water at work, as I have for the last 2 months. Go for it!

[Bronsa]

just an update -- I've been looking at this over the past few days and it's a little more involved than I'd hoped, not sure how long it's going to take me but I'm definitely interested in having a fix for this as soon as possible

[jafingerhut]

One way I had been considering implementing this is roughly as follows -- hack up tools.reader so that it also returns a separate data structure with start & end line & column numbers for each sub-form, and which have metadata like ^{:eastwood/lint ...} (where I don't have anything concrete proposed for what that metadata looks like, except that it should allow you to enable or disable any of the existing or future linters by name, as can be done from the command line for entire Eastwood runs).

Calculate and generate warnings as normal, but just before printing them, check their line/column number. If it lies within a range of start/end line/column numbers that have the warning suppressed, don't print it. The finding of the warning's line/column number in the data structure returned by the reader should ideally be a 'narrowest scope' search, i.e. find the smallest sub-form that contains the warning's line/column number, then look for the smallest enclosing form that enables or disables the linter, and use that setting. This would allow one to give metadata for large forms that enable a warning, but on particular sub-forms of that large form, to disable it.

I only have pieces of such an implementation lying around on disk, from several months to a year or so ago now, mainly the part about hacking up tools.reader, but it isn't in any kind of usable shape.

[ty-i3]

+1

The ability to suppress a specific lint warning on a specific form would allow us to fully incorporate eastwood in our dev process, including the CI builds.

[borkdude]

FYI, clj-kondo now has something similar, but using reader comments instead of metadata as to not pollute the runtime, but the idea is the same:

clj-kondo/clj-kondo#872 (comment)