Directive resolve_regexp returns invalid labels

[alandonham]

What version of gazelle are you using?

master

What version of rules_go are you using?

0.50.1

What version of Bazel are you using?

7.4.0

Does this issue reproduce with the latest releases of all the above?

Yes

What operating system and processor architecture are you using?

MacOS Arm64

What did you do?

Declare a resolve_regexp directive.
# gazelle:resolve_regexp py foo\. //x/y/z

Create a Python file with the following import:
from foo.bar import baz

What did you expect to see?

deps should resolve to //x/y/z when running Gazelle

What did you see instead?

deps resolves to //x/y/zbar when running Gazelle

This pull request introduces a change that attempts to create a label from the import statement using regex. This is problematic, because if the regex does not match on the entire import statement, it will only replace pieces of the import statement, resulting in an incorrect label that still passes the label.Parse() check and is then written to the BUILD file.

For example, the import statement passed to FindRuleWithOverride() and then o.resolveRegexDep() in the above example is the string foo.bar.

The following is then called inside of resolveRegexDep():
o.ImpRegex.ReplaceAllString("foo.bar", "//x/y/z")

According to the docs:

// ReplaceAllString returns a copy of src, replacing matches of the [Regexp]
// with the replacement string repl.
// Inside repl, $ signs are interpreted as in [Regexp.Expand].

Thus, we end up with the label //x/y/zbar.

Other regex passed into the same directive has other corner cases that can be hit, and result in various invalid labels.

Creating the label by running ReplaceAllString() appears to contain many corner cases where regex that has been successfully utilized in the past now results in invalid labels.

[fmeum]

It should work with a regex like this:

bazel-gazelle/resolve/resolve_test.go

Line 30 in 2a84bad

{Key: "resolve_regexp", Value: "go ^github.com/foo/(.*)/(.*)$ @com_example//$1/bar_sub_dir/$2:replacement"},

We could automatically wrap the regex in ^...$ if it isn't already.

[fmeum]

Cc @lkassar-stripe

[alandonham]

It should work with a regex like this:

bazel-gazelle/resolve/resolve_test.go

Line 30 in 2a84bad

{Key: "resolve_regexp", Value: "go ^github.com/foo/(.*)/(.*)$ @com_example//$1/bar_sub_dir/$2:replacement"},

We could automatically wrap the regex in ^...$ if it isn't already.

My big concern here is that ReplaceAllString() usage to generate labels is going to be challenging to ensure it's correct. It introduces a way for users to generate invalid and malformed labels without an obvious way to control what is generated. Users will end up needing to reverse engineer what is happening to their labels, as I did, and attempt to manipulate the regex and label string so that it generates a valid label. I think this will make the directive itself much more challenging to use.

Also, changing the behavior of resolve_regexp in a non-backwards compatible way is not ideal. Perhaps it would make more sense to add an additional parameter to enable this regex string replacement or put it behind its own directive?

[fmeum]

I agree that the PR you linked has unintended consequences for existing usage patterns and we should fix that.

What do you think of applying ReplaceAllString only if the regex contains any capturing groups (can be determined with https://pkg.go.dev/regexp#Regexp.NumSubexp) and/or the replacement string contains references to groups? I could see that being sufficiently backwards compatible with intentional usage of the new feature while also being less surprising.

[alandonham]

I agree that the PR you linked has unintended consequences for existing usage patterns and we should fix that.

What do you think of applying ReplaceAllString only if the regex contains any capturing groups (can be determined with https://pkg.go.dev/regexp#Regexp.NumSubexp) and/or the replacement string contains references to groups? I could see that being sufficiently backwards compatible with intentional usage of the new feature while also being less surprising.

I think that approach sounds reasonable. I like the spirit of the regex replacement, so long as we can narrow down the scope of what it is able to impact to intentional use of it rather than it being the default, as you mentioned.

[fmeum]

I'm a bit swamped right now, would you be willing to send a PR to implement some variant of this approach?

[alandonham]

I'm a bit swamped right now, would you be willing to send a PR to implement some variant of this approach?

Sure, I'll try and get around to this today or tomorrow.