Better support for linters that don't require all of go/loader about lint HOT 12 CLOSED

Thanks for opening this issue.

I hadn't thought of linters not needing to parse code at all. I had thought about linters avoiding type-checking, such as those working directly on the AST only, but not about skipping go/loader altogether. Perhaps we could have these levels of what is given to a checker:

• Import paths (either package paths or Go file names, i.e. what gotool returns)
• go/loader with configurable TypeCheckFuncBodies (if just false, pretty much just the ASTs)
• ssa with all the bells and whistles

At first I was thinking of saying that this should only work with linters that use go/loader. But I agree with you that it seems like a silly limitation. Then again, without it we lose control of things like whether test files are included or what build context (GOOS, GOARCH, etc) is used.

Perhaps we could make linters that don't require the AST use go/loader anyway? Without type checking, the performance penalty of parsing the packages and files into an AST would be negligible. And it would greatly simplify the interface and let us keep control of loading parameters as outlined above.

How would lint integrate with tools from the Go linter ecosystem like gometalinter?

To clarify; as you say, gometalinter is fundamentally a different tool. It is more separated from the linters it runs, and as such it is both more flexible and slower. Which also means the linters don't need to be adapted to work with it.

My idea here is not to replace gometalinter. For example, if a linter developer refuses to maintain an entry point implementing Checker, I don't think I'd myself be willing to maintain a fork/vendor adding it. We'll see.

As for how everything would fit together; On one hand we have the interface. If tools like gometalinter want to make use of that with particular linters like we do, they're welcome to.

On the other hand, something kinda fun could happen. We could have our meta linter satisfy Checker too :) Then, for example, gometalinter could replace all the linters that now satisfy Checker with simply metalint.

from lint.

Whatever happens, there is no going back after 1.0. Marking as such.

from lint.

Some numbers on running metalint -tests=false std at different levels (real time, not user time):

• Just gotool: ~0.1s
• Up to go/loader with no func body type-checking: ~1.8s
• Up to go/loader with func body type-checking: ~2.8s
• Up to go/ssa: ~3.6s

go/loader without body type-checking is slower than I anticipated. I'll investigate some to see if it can be made slimmer.

from lint.

Seems like there is no way to change the way go/loader imports packages - was thinking I could make it use importer.For("gc") instead of importer.For("source"), but it seems like it handles imports itself directly instead of leveraging go/types.Config.Importer.

Edit: confirmed, it's hard-coded to parsing manually from source.

from lint.

Modifying go/loader manually for the sake of completeness:

• Up to go/loader with no type checking at all (just parsing): ~0.8s

It seems like parsing ASTs from source and minimal type-checking take similar amounts of time - ~0.7s and ~1s.

from lint.

Perhaps we could have these levels of what is given to a checker: [...]

Sounds reasonable.

if a linter developer refuses to maintain an entry point implementing Checker, I don't think I'd myself be willing to maintain a fork/vendor adding it.

At the end of the day, what people want is fast linting. I bet there are quite a few people willing to patch commonly used linters, if the developer doesn't get around to implementing Checker, or maintain a fork.

Perhaps we could make linters that don't require the AST use go/loader anyway?

In practice, I don't think it's that bad to use go/loader all the time, regardless of which linters will be run. A common use-case will be that people run a handful of linters that require go/loader anyway, and alongside some other linters that don't need it. If they use metalint to run just lll (for example), we could argue that this is not the main use-case and they have to live with the fact that metalint does more work than strictly necessary.

But also see #10.

We could have our meta linter satisfy Checker too :)

I'd go for that. It provides the most flexibility for composing linters/metalinters, and would simplify testing.

The limitation I see here is that when metalint (or other metalinters) return results from multiple linters, the information which linter originally found the Issue is lost. The way gometalinter handles this is by appending (name-of-linter) to each message. This is OK for human users and also relatively safe to parse, as every line ends with the same pattern. We could just do the same, or extend Issue in such a way that the linter name can be indicated. Any thoughts about this?

from lint.

But also see #10.

I mostly agree with you on this - but let's move it to #10. This issue is about avoiding work and ssa, that one is about linters that may not work well with go/loader.

We could just do the same, or extend Issue in such a way that the linter name can be indicated. Any thoughts about this?

Exactly - we could just add something like Source() string with the checker name. Or more complex versions of the same that also returned its import path, etc.

from lint.

We could either go with that and have

type Issue interface {
	Source() string
	Pos() token.Pos
	Message() string
}

which repeats the source string for every issue, or group the issues:

type Checker interface {
	Check(*loader.Program, *ssa.Program) ([]IssueGroup, error)
}

type IssueGroup interface {
	Source() string
	Issues() []Issue
}

type Issue interface {
	Message() string
	Pos() token.Pos
}

But I must say I like the simpler solution better, even though mentioning the source in every issue is repetitive. Most developers will write a linter (not a metalinter), and they should not need to worry about what an IssueGroup (or slice thereof) is. So all they need to do is to return their linter's name with Source().

from lint.

I think neither of those is adequate. Remember that the interface is for linters, not meta-linters.

Sure that each issue repeating the source is perhaps a bit redundant, but it doesn't mess with the simple interface that works for both linters and meta-linters.

from lint.

To clarify - linters themselves need not say what their name is. That's up to meta-linters to specify, when merging issues from multiple sources/linters.

from lint.

We already decouple SSA, so updated the title.

from lint.

Now that go/packages and go/analysis are a thing, this repo lost its purpose.

from lint.