Run revive over a project with an invalid go source file

[josemiguelmelo]

Hello,

Describe the bug
When you run Revive on a project that has an invalid go source file, it fails because it can't read the AST.

To Reproduce
Steps to reproduce the behavior:

1. Create an invalid go source file, such as:

package main

import "fmt"

func example() {
	ok := 1
	if ok == 1 {
		fmt.Print("OK")
	
}

2. I run it with the following flags & no configuration file:

revive -formatter json

Expected behavior
Instead of failing the entire analysis, Revive could add the AST parsing error to the revive failures list and output it at the end along with other failures.

Logs

Current output:

example.go:10:3: expected '}', found 'EOF'

with exit status 1

[chavacava]

The current behavior is that of golint.

$ golint test
test/add-constant_test.go:6:2: expected declaration, found "github.com/mgechev/revive/lint"
$ revive test
test/add-constant_test.go:6:2: expected declaration, found "github.com/mgechev/revive/lint"

[josemiguelmelo]

Is it required to have the exact same behaviour as golint? I think it would be nice to at least have the option to not fail when the AST is invalid in a file. I mean revive doesn't need compilation to analyse the code, so this invalid AST could become a revive Failure instead of finishing the process with exit status 1.

[chavacava]

Hi @josemiguelmelo, thanks for filling the issue.
Does revive need to behave as golint? I think yes because it is announced as a drop-in replacement for golint.
I understand your point but we also need to take into account that skipping one file's analysis may bias other files' analysis in the context of typechecked analysis.

Maybe we can add a command line flag to ask for skipping incorrect files.

[josemiguelmelo]

Sure, sure. That's why I was saying to add this incorrect file to the final results as a Failure, without failing the entire analysis. However, it gets a slightly different behaviour than the golint.

Maybe the command line flag would be nice. With it, revive continues with same behaviour as golint, unless the user specifically uses the flag.

[mgechev]

Although configurability allows a level of flexibility, over time it can get hard to follow what's the combined behavior of different flags.

If we decide to introduce this feature I'd prefer if we enable it by default for everyone. As @chavacava mentioned, ideally we want to keep compatibility with golint, but we can continue the discussion and collect more opinions.

[josemiguelmelo]

I agree that it can get hard over time. It would be just a way of continuing with the same behaviour as golint by default, except if the user specifies it.

[doniacld]

Hey!
I would be interested to work on this issue. If you could assign it to me please ?

[chavacava]

closed by #598