blank-imports and constant-logical-error do not support cgo

[veger]

Describe the bug
When using CGO an import "C" is required to active cgo (and import/define C code). The blank-imports check does not accept this and throws the following warning:

blank-imports: a blank import should be only in a main or test package, or have a comment justifying it (revive)

To Reproduce

Consider this main.go file:

package main

import "unsafe"

// #include <stdlib.h>
import "C"

func main() {
	cStr := C.CString("test")

	C.free(unsafe.Pointer(cStr))
}

Run revive for this file and the waring appears for the import "C" line.

Expected behavior
There should not be a warning for this file.

Desktop (please complete the following information):

• OS: Arch Linux
• go version go1.17.7 linux/amd64

[veger]

Related I guess:

	myStruct := C.myStruct{}
	result := C.someFunction(&myStruct) // <- lint warning here

triggers constant-logical-expr: expression always evaluates to true (revive)

[chavacava]

Hi @veger thanks for filling the issue.
I'll try to work on it in the week.

And always keep in mind what Rob Pike says about Cgo :)

[veger]

Yes I am not a fan as well, but we have to use a proprietary, 3rd-party library that would take years to reproduce and we simply have not the time to do so unfortunately (because it would be a very nice project)... :)

[chavacava]

Hi @veger I'm not been successful in reproducing the problem.
From the error message you provided I can guess you are running revive through a linter aggregator (like golangci-lint) could you please fetch and checkout the branch reproduce635`and do

$ make build
$ ./revive -config defaults.toml deleteme/blank-import-cgo.go

[veger]

Edit: Sorry due to all tries, I confused some results and this one works fine as well (I'll continue figuring out a reproducible test)

Yes I am using the aggregator golangci-lint, but unfortunately I cannot reproduce your example branch using golangci-lint with my config... So I guess I messed up my 'reproducible example' 😞

While trying to make it reproducible with your branch I found another cgo-related error.
With this change

diff --git a/defaults.toml b/defaults.toml
index 8d298de..e60c086 100644
--- a/defaults.toml
+++ b/defaults.toml
@@ -5,4 +5,6 @@ errorCode = 0
 warningCode = 0
 
 [rule.blank-imports]
+[rule.file-header]
+  arguments =["This is the text that must appear at the top of source files."]

when I run the build revive I get:

❯ ./revive -config defaults.toml deleteme/blank-import-cgo.go
deleteme/blank-import-cgo.go:2:1: the file doesn't have an appropriate header

Needless to say the file is not there (I guess it is temporarily there), but I also asked to only lint the black-import-cgo.go file and not the one with the error.

I'll continue trying to reproduce 'my' error

[veger]

Finally I figured it out... The example uses package main, when looking at the error at package main the 'blank-import' behavior is allowed 🤦

I tried some more but was still not able to make an example. 😞

Using golangco-lint I was able to, but only with all linters enabled:

linters-settings:
  revive:
    ignore-generated-header: true
    rules:
      - name: blank-imports
        disabled: false

and the following code changes in 'deleteme' (basically it is only the import in a non-main/test package)

diff --git a/deleteme/blank-import-cgo.go b/deleteme/blank-import-cgo.go
index 928b001..e2ff120 100644
--- a/deleteme/blank-import-cgo.go
+++ b/deleteme/blank-import-cgo.go
@@ -1,13 +1,5 @@
-// Package main ...
-package main
-
-import "unsafe"
+// Package deleteme ...
+package deleteme
 
 // #include <stdlib.h>
 import "C"
-
-func main() {
-       cStr := C.CString("test")
-
-       C.free(unsafe.Pointer(cStr))
-}

When running it gives:

$ golangci-lint run -v --disable-all -E revive --config=golangci.yml ./deleteme/...                                                  [1]
INFO [config_reader] Used config file golangci.yml 
INFO [lintersdb] Active 1 linters: [revive]       
INFO [loader] Go packages loading at mode 7 (compiled_files|files|name) took 43.883437ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 136.644µs 
INFO [linters context/goanalysis] analyzers took 0s with no stages 
INFO [runner] Issues before processing: 2, after processing: 1 
INFO [runner] Processors filtering stat (out/in): cgo: 1/2, identifier_marker: 1/1, path_shortener: 1/1, source_code: 1/1, severity-rules: 1/1, filename_unadjuster: 1/1, skip_files: 1/1, exclude: 1/1, exclude-rules: 1/1, nolint: 1/1, max_from_linter: 1/1, path_prettifier: 1/1, skip_dirs: 1/1, diff: 1/1, max_same_issues: 1/1, autogenerated_exclude: 1/1, uniq_by_line: 1/1, max_per_file_from_linter: 1/1, path_prefixer: 1/1, sort_results: 1/1 
INFO [runner] processing took 190.561µs with stages: exclude-rules: 60.729µs, identifier_marker: 50.372µs, nolint: 20.293µs, autogenerated_exclude: 18.773µs, path_prettifier: 17.887µs, source_code: 11.075µs, skip_dirs: 4.964µs, max_same_issues: 1.268µs, uniq_by_line: 1.011µs, filename_unadjuster: 737ns, max_from_linter: 724ns, cgo: 685ns, path_shortener: 658ns, max_per_file_from_linter: 473ns, exclude: 214ns, severity-rules: 175ns, sort_results: 146ns, diff: 141ns, skip_files: 137ns, path_prefixer: 99ns 
INFO [runner] linters took 498.841µs with stages: revive: 252.117µs 
deleteme/blank-import-cgo.go:5:8: blank-imports: a blank import should be only in a main or test package, or have a comment justifying it (revive)
import "C"
       ^
INFO File cache stats: 1 entries of total size 76B 
INFO Memory: 2 samples, avg is 4

As you can see it is only running revive as a linter and it shows the lint issue.

Interestingly enough, with ignore-generated-header: false (in the golangci.yml) the blank-imports issue goes away.

Running revive -config defaults.toml ./deleteme/... (with either true or false for the above option) results in not blaknk-imports issues...

In case it is interesting:

$ golangci-lint version
golangci-lint has version v1.44.0 built from (unknown, mod sum: "h1:YJPouGNQEdK+x2KsCpWMIBy0q6MSuxHjkWMxJMNj/DU=") on (unknown)

Note that the constant-logical-error issue I could not (yet) reproduce... I guess there is something (else) in my code that triggers it... I can take another look later if needed.

[chavacava]

Hi @veger thanks for the investigations.
I've quickly tried to reproduce by changing the package name but the failure did not show up.

I'll continue searching.

[veger]

It only shows up when using revive via golangci-lint, I was also not able to reproduce it directly with revive itself.

Maybe golangci-lint does something 'behind the scene' causing revive to choke? 🤔

[chavacava]

I really do not known golangci-lint internals thus it is hard to me to imagine how it can force revive to produce these false positives.
Maybe opening an issue on golangci-lint side will help.
I'll continue trying to see if some special configuration makes revive produce these errors

[oschwald]

This may be related to golangci/golangci-lint#2611.

[veger]

I grabbed and build golangci-lint with this commit and tried. Unfortunately I still have the same issue.

$ golangci-lint version                                                                                                                                                      [1]
golangci-lint has version v1.44.3-0.20220223082849-30c6166b0811 built from (unknown, mod sum: "h1:SrfFX8QEdN2/8lMuUn54ENLCLBM2aQguAia4q8G0q2E=") on (unknown)

I'll open an issue at golangci-lint

[chavacava]

From the comments in golangci/golangci-lint#2612 it is clear that it is more a golangci-lint issue than a revive's one
@veger are you OK for closing this issue?

[chavacava]

@veger because the actual file revive analyses (when ran inside golang-ci) it is a generated file you might configure revive to ignore it by setting

# When set to false, ignores files with "GENERATED" header, similar to golint
ignoreGeneratedHeader = false

[veger]

Yes I am fine with closing, it indeed seems to related to golangci-lint and how it processes the files.

Shouldn't the ignoreGeneratedHeader not be set to true? It enabled ignoring the generated files?

We already set this to true, as we have no (direct) influence on any of our generated code and cannot fix any linting issues that are found.

[chavacava]

The config flag is somewhat difficult to understand :)
When setting ignoreGeneratedHeader to true you are saying to revive: "ignore the presence of the GENERATED header"... thus because revive ignores the presence of the header, a generated file will look as an ordinary file (i.e. it will anlyze it)

[chavacava]

Not a revive issue

[veger]

The config flag is somewhat difficult to understand :)

Thanks for the explaining 👍
I'll reverse our configuration, as I misunderstood the flag completely.