Should not recommend to use ignore build tag

[mem]

This is related but not the same as #253

I was getting a very weird error from golangci-lint about an empty ruleset and forgetting to call Load().

In short, I was in a similar situation that described by #253: in order to avoid my main module from depending on the dsl, I created another module that contains the rules and other tools. When trying to run ruleguard, it fails to load the rules because the dsl is not imported by the main module (the one being checked).

After much confusion, I ended up moving the rules to the main module. After running

go get github.com/quasilyte/go-ruleguard/dsl@v0.3.21

ruleguard worked!

My CI process runs go mod tidy to make sure that go.mod and go.sum are up to date. This was failing because go mod tidy was removing github.com/quasilyte/go-ruleguard/dsl from go.mod.

I was able to easily reproduce this behavior locally:

go get github.com/quasilyte/go-ruleguard/dsl@v0.3.21 # adds to go.mod
go mod tidy                                          # removes from go.mod

After digging around, it turns out that while go mod tidy does not care about build constraints (it behaves as if every possible build constraint is in effect, so that it loads all the packages and all the files in the module), it will ignore anything using ignore as a tag.

That's documented here:

https://go.dev/ref/mod#go-mod-tidy

go mod tidy works by loading all of the packages in the main module and all of the packages they import, recursively. This includes packages imported by tests (including tests in other modules). go mod tidy acts as if all build tags are enabled, so it will consider platform-specific source files and files that require custom build tags, even if those source files wouldn’t normally be built. There is one exception: the ignore build tag is not enabled, so a file with the build constraint // +build ignore will not be considered. Note that go mod tidy will not consider packages in the main module in directories named testdata or with names that start with . or _ unless those packages are explicitly imported by other packages.

and it's implemented here:

https://github.com/golang/go/blob/master/src/cmd/go/internal/imports/build.go#L188

I opened an issue in the go repo: golang/go#54993

And also a CL: https://go.dev/cl/430136

The behavior around ignore is not going to change, so until #253 is addressed, I propose that ruleguard stops recommending the use of the ignore tag, as that makes the rules invisible to go mod tidy and therefore it removes the dsl from go.mod.

[mem]

For the benefit of people running into this problem, this is the actual error you see when using gocritic or golangci-lint:

some_file.go:nnn:mmm: ruleguard: execution error: used Run() with an empty rule set; forgot to call Load() first? (gocritic)

that's caused by not being able to load the rules. gocritic/golangci-lint eats up the error and reports the above instead.

[quasilyte]

Ideally, I would like to make dsl dependency optional for running ruleguard.
It was easier before with GOPATH, when dsl could be a global dependency just for the tool, but with Go modules it's now harder to figure out the right way.

Basically it boils down to how go/* packages work, it's not enough to embed the dsl pakcage, bundle it with a binary and load that for the users (unless they specify that they want their own version of dsl package). Typechecking won't fly. :(

[mem]

Oh, sure, @quasilyte, I understand that, that's why I said this is related but not the same as the other issue.

My "problem" right now is that the existing documentation tells you to do something that won't work (given some conditions).

Would you like me to open a PR to change the recommendation to use ignore to something else? E.g. ruleguard? ruleguard_rules?

[quasilyte]

@mem sure. At the very least it would be helpful to describe the issues that can show up in that case.

[mem]

OK, I have created a PR (#411) where I added a troubleshooting section to README.md (to try to make it visible). There I show some of the output I have seen when running into this isuse. Hopefully that's clear enough to help others running into this.