pkg/golinters: add noreplace

[carnott-snap]

Add support for enforcing that go.mod should not contain replace clauses.

https://git.sr.ht/~urandom/noreplace

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ldez]

Hello,

I don't think that calling the go CLI is the right way to do that. (https://git.sr.ht/~urandom/noreplace/tree/master/item/noreplace.go#L22)

The following implementation seems better: https://github.com/gostaticanalysis/noreplace

[carnott-snap]

oh, it is super gross, but I did not know about your noreplace, so I followed what gomodguard does. (a merged linter.)

I would rather not support this one off package, let me see if I can drop in the gostaticanalysis package.

EDIT: also, heads up they do the same thing just one level deeper. this is a fundamental limitation of the stdlib, since this is not exported at runtime and hidden behind cmd/go/internal/modload.Init.

[ldez]

ok but it seems weird, I will try to find how the do that without the go command.

FYI, I was not a maintainer when gomodguard was merged.

[ldez]

Another idea, maybe this can be added to gomodguard 🤔

[carnott-snap]

I looked at that first, but gomodguard is about enforcing dependencies and is a pretty complex by comparison. If the author is interested we can, but it seems like pulling in noreplace is easy enough.

Also, the implementation of noreplace is not optimal. Notice that my package actually returns token.Position information so that we can populate your result.Issue, specifically the Replacement LineRange and Pos fields; though I do like memoising the call to modfile.Parse. I think that this will require substantial changes to the analysis.Pass interface, see golang/go#44753.

cc @ryancurrah

[ldez]

Yes in fact the noreplace implementation seems worse than the gomodguard implementation.

---

The code inside the Go command to find the module path:
https://github.com/golang/go/blob/6db80d74200675e20c562684c0bcc6d12a5631eb/src/cmd/go/internal/modload/init.go#L727

[ldez]

offtopic: @carnott-snap how it is possible to contribute to SourceHut repo? (please don't say "with patch by mail" 😸 )

Edit: I have my answer: it's with patch by mail 😹 (git send-email)

[ldez]

Are you sure to want to use SourceHut instead of Github/Gitlab/Bitbucket for this kind of project?

SourceHut uses an old way for community contributions, for me it's more a way to limit contributions because the contribution cost is high.

[carnott-snap]

it is my default, so yes; plus the code should be pretty stable, with the exception of needing to pass more data to golangci-lint, or an eventual modanalyser.Analysis symbol or what ever golang/go#44753 comes up with.

if you would like noreplace.Check to return a different interface (more, less, or different data) let us sort that out now, since even on github it will be harder to make changes after we merge this.

[ldez]

So maybe you can split your code into two parts:

• one to get the mod path
• one to analyze the mod file

Like that it will be possible to get the mod path only one time.

Edit: in fact, I'm not sure what is the best way 🤔

[carnott-snap]

can you elaborate? (suggestions are welcome) I am not familiar with the inner workings of the golangci-lint framework. this code is copypasta from gomodguard.

I am just unclear why noreplace.Check would be running more than once?

[ldez]

My suggestion ldez@11f852d

I tested it on golangci-lint with:

go run ./cmd/golangci-lint/ run --no-config --disable-all --enable=noreplace

[ldez]

More I watch your linter more I think there is a problem with the current implementation of gomodguard inside golangci-lint

[carnott-snap]

ldez@11f852d

left some comments inline; though I was suggesting you use the ```suggestion interface (ctrl+g), as it would keep everything in the pr.

More I watch your linter more I think there is a problem with the current implementation of gomodguard inside golangci-lint

yeah, I think that anything doing go.mod lints may not really be playing nice with analysis.Analyzer, though gomodguard appears to do both module and package lints.

[ldez]

I know "github suggestions" but my suggestions are too large.

[ldez]

I tested the PR on a sandbox project and in fact it doesn't work.

[ryancurrah]

We could add noreplace check to gomodguard. Not sure what issues your seeing.

[ldez]

I think that gomodguard also doesn't work as expected

Edit: it works as expected

[ldez]

@carnott-snap Have you tried your PR (not your noreplace binary but a golangci-lint binary) on a real project?

[carnott-snap]

We could add noreplace check to gomodguard. Not sure what issues your seeing.

If everybody prefers that, I have some sample code I was messing around with, but it was not working. Probably speaks to @ldez's comment. I will wait for confirmation before opening that pr.

[ryancurrah]

I think that gomodguard is also doesn't work as expected

I have been using it where I work for a while, no issues so far. What were you expecting it to do? Maybe I can help clear any confusion up.

If everybody prefers that, I have some sample code I was messing around with, but it was not working. Probably speaks to @ldez's comment. I will wait for confirmation before opening that pr.

It should be pretty easy to identify replace directives with the golang.org/x/mod package.

As per the module documentation.

If the path on the right side of the arrow is an absolute or relative path (beginning with ./ or ../), it is interpreted as the local file path to the replacement module root directory, which must contain a go.mod file. The replacement version must be omitted in this case. - https://golang.org/ref/mod#go-mod-file-replace

So we look for replace directives matching the above conditions and set them as blocked. If this is for local replace directives only.

[ldez]

I have been using it where I work for a while, no issues so far. What were you expecting it to do? Maybe I can help clear any confusion up.

I edited my message, it works.

---

noreplace is really close to the blocked.local_replace_directives option of gomodguard

gomodguard works with go.mod, noreplace doesn't work but currently I don't know why.

---

I see 2 use-cases for a replace directive linter:

• the need to ban all replace directives
• the need to allow only local replace directives (for dev) and ban all other replace directives.

[ldez]

I can see a third case:

• allow only some replace directive because of old dead transitive dependencies or dependencies on docker/moby weird dependencies.

[ldez]

So there are 3 options:

• make this linter works as expected
• merge this linter with gomodguard
• create a dedicated gomodreplace linter

[ryancurrah]

Yeah, those are definitely valid cases. The gomodreplace linter sounds interesting.

[ldez]

I agree, the gomodreplace linter is my favorite option.
I am also interested in working on this topic but maybe @carnott-snap wants to handle that.

[ldez]

@carnott-snap I'm not sure we understand each other, I was talking about creating a dedicated linter for this.

[ldez]

I created the gomodreplace linter https://github.com/ldez/gomodreplace

EDIT: I created the gomoddirectives linter https://github.com/ldez/gomoddirectives

[ldez]

I will close this PR in favor of #1817