go-ruleguard icon indicating copy to clipboard operation
go-ruleguard copied to clipboard

Add support for matching imports (by prefix/substring)

Open powerman opened this issue 5 years ago • 5 comments

This will enable custom and more flexible depguard-like rules.

This is needed, for example, to restrict packages inside pkg/ to import package inside internal/ - like grep -r "\"$(go list -m)/internal" pkg. Probably this will also need ability to restrict to which packages this should be applied (to apply this rule only for packages in pkg/).

powerman avatar Sep 05 '20 13:09 powerman

There are several separate issues here:

  1. Run some rules only for packages that meet some condition.
  2. gogrep can't handle import specs that great, so it's hard (impossible) to write that pattern right now. We can work around this (see below).

Conditional rules

It's probably possible to come up with the syntax to declare some rules, but I have no good ideas right now.

Should it be a per-rule filter? Should it be a per rule file constraint?

As a workaround, I can suggest having a separate rules file and run ruleguard twice: once with the normal rules set, then with the imports-related rules set:

ruleguard -rules rules.go ./src/...
ruleguard -rules import_rules.go ./src/pkg/...

Matching ImportSpec

Let's suppose that gogrep supports import matching. We'll assume that we want to match "foo" import.

In Go, there are at least 2 ways to write that import on a syntax level:

// Without explicit () group
import "foo"

// Somewhere inside () group
import (
  // ..
  "foo"
  // ..
)

This will lead to an awkward situation where you need to write 2 patterns:

m.Match(
  `import "foo"`,
  `import ($*_; "foo"; $*)`,
)

What you probably want to do is to match *ast.ImportSpec which is a single import declaration unit. Like so:

m.MatchImportSpec(`"foo"`)
m.MatchImportSpec(`$alias $path`).Where(... filters over alias and path)

That's not a valid syntax as of yet, just an idea.

quasilyte avatar Sep 06 '20 20:09 quasilyte

Should it be a per-rule filter? Should it be a per rule file constraint?

I believe it should be per-rule filter, because I suppose most rules are general and must be applied to all files, thus rules which should be restricted to subset of files/packages should be rare exception. So, needs to add this filter to each rule unlikely will became annoying, and at same time absence of filter with too wide scope (like per rule file) will helps to avoid restricting other rules to subset of files by mistake.

BTW, maybe it'll be easier to implement matching it in same way as for import specs - i.e. match by import path of processed file?

As a workaround, I can suggest having a separate rules file and run ruleguard twice:

I don't like the idea, because I don't see how to run it this way from .golangci.yml - it doesn't provide configuration options required for this.

powerman avatar Sep 06 '20 20:09 powerman

BTW, maybe it'll be easier to implement matching it in same way as for import specs - i.e. match by import path of processed file?

Do you mean a filter that checks the package path of the file that is being processes? It does sound like a valid idea. I think it's possible to recognize such filters and build an index for them, so we don't have to run these rules for every package only to see that it doesn't match.

quasilyte avatar Sep 07 '20 14:09 quasilyte

Do you mean a filter that checks the package path of the file that is being processes?

Yes.

powerman avatar Sep 07 '20 16:09 powerman

There is m.File().PkgPath that is available for a while now.

https://go-ruleguard.github.io/by-example/file-predicates

e.g.:

m.File().PkgPath.Matches("some-regexp")

quasilyte avatar Apr 19 '22 21:04 quasilyte