Repository: bazel-contrib/bazel-gazelle

Issue:
Gazelle improperly handles nested `internal` directories
Relevant code: https://github.com/bazel-contrib/bazel-gazelle/blob/master/language/go/generate.go#L844

When a target is in a nested `internal` directory, the `visibility` of that target does not appear to be set correctly. It appears to set visibility based on the first seen `internal` directory. I would expect it to instead set visibility based on the last seen `internal` directory.

I've included some example `go_library` targets based on some testing I did. Our repo has a top level `internal` directory.

```
load("@rules_go//go:def.bzl", "go_library")

go_library(
    name = "internal",
    srcs = ["foo.go"],
    importpath = "github.com/launchdarkly/myRepo/pkg/internal/foobar/internal",
    visibility = ["//pkg:__subpackages__"], # this should be //:foobar:__subpackages__
)

go_library(
    name = "internal",
    srcs = ["bar.go"],
    importpath = "github.com/launchdarkly/myRepo/pkg/internal",
    visibility = ["//pkg:__subpackages__"], # this is correct
)

go_library(
    name = "internal",
    srcs = ["foo.go"],
    importpath = "github.com/launchdarkly/myRepo/internal/testDir/subdir/internal",
    visibility = ["//:__subpackages__"], # this should be //subdir:__subpackages__ but b/c /internal is the first child of root (i.e. `myRepo` in this case), it doesn't work properly
)
```

Discussion:

Solution patch:
diff --git a/language/go/generate.go b/language/go/generate.go
index cecf0b1..8ffb113 100644
--- a/language/go/generate.go
+++ b/language/go/generate.go
@@ -846,8 +846,8 @@ func (g *generator) commonVisibility(importPath string) []string {
 	// subpackages of the parent.
 	// If the import path contains "internal" but rel does not, this is
 	// probably an internal submodule. Add visibility for all subpackages.
-	relIndex := pathtools.Index(g.rel, "internal")
-	importIndex := pathtools.Index(importPath, "internal")
+	relIndex := pathtools.LastIndex(g.rel, "internal")
+	importIndex := pathtools.LastIndex(importPath, "internal")
 	visibility := getGoConfig(g.c).goVisibility
 	if relIndex >= 0 {
 		parent := strings.TrimSuffix(g.rel[:relIndex], "/")
@@ -861,7 +861,6 @@ func (g *generator) commonVisibility(importPath string) []string {
 				visibility = append(visibility, "@"+repo.Name()+"//:__subpackages__")
 			}
 		}
-
 	} else {
 		return []string{"//visibility:public"}
 	}
diff --git a/pathtools/path.go b/pathtools/path.go
index 42962c9..8017d92 100644
--- a/pathtools/path.go
+++ b/pathtools/path.go
@@ -71,15 +71,13 @@ func RelBaseName(rel, prefix, root string) string {
 	return base
 }
 
-// Index returns the starting index of the string sub within the non-absolute
-// slash-separated path p. sub must start and end at component boundaries
-// within p.
+// Index returns the starting index of the first ocurrence of the string sub
+// within the slash-separated path p. sub must start and end at component
+// boundaries within p.
 func Index(p, sub string) int {
 	if sub == "" {
 		return 0
 	}
-	p = path.Clean(p)
-	sub = path.Clean(sub)
 	if path.IsAbs(sub) {
 		if HasPrefix(p, sub) {
 			return 0
@@ -114,6 +112,48 @@ func Index(p, sub string) int {
 	}
 }
 
+// LastIndex returns the starting index of the last occurrence of the string sub
+// within the slash-separated path p. sub must start and end at component
+// boundaries within p.
+func LastIndex(p, sub string) int {
+	if sub == "" {
+		return len(p)
+	}
+	if path.IsAbs(sub) {
+		if HasPrefix(p, sub) {
+			return 0
+		} else {
+			return -1
+		}
+	}
+	if p == "" || p == "/" {
+		return -1
+	}
+
+	// prevIndex returns the starting index in p of the component that starts
+	// before index i.
+	prevIndex := func(i int) int {
+		slash := strings.LastIndexByte(p[:i], '/')
+		if slash < 0 {
+			return 0
+		}
+		return slash + 1
+	}
+	i := prevIndex(len(p))
+	for {
+		suffix := p[i:]
+		if len(suffix) >= len(sub) &&
+			suffix[:len(sub)] == sub &&
+			(len(suffix) == len(sub) || suffix[len(sub)] == '/') {
+			return i
+		}
+		if i == 0 || (p[0] == '/' && i == 1) {
+			return -1
+		}
+		i = prevIndex(i - 1)
+	}
+}
+
 // Prefixes returns an iterator (iter.Seq) over all the prefixes of p.
 // For example, if p is "a/b/c", the iterator yields "", "a", "a/b", "a/b/c".
 //



Test patch:
diff --git a/pathtools/path_test.go b/pathtools/path_test.go
index fd330d7..923b0e8 100644
--- a/pathtools/path_test.go
+++ b/pathtools/path_test.go
@@ -21,6 +21,262 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
+func TestIndex(t *testing.T) {
+	for _, tc := range []struct {
+		desc, p, sub string
+		want         int
+	}{
+		{
+			desc: "empty",
+			p:    "",
+			sub:  "",
+			want: 0,
+		},
+		{
+			desc: "empty_p",
+			p:    "",
+			sub:  "a",
+			want: -1,
+		},
+		{
+			desc: "empty_sub",
+			p:    "a",
+			sub:  "",
+			want: 0,
+		},
+		{
+			desc: "match_start_1",
+			p:    "a/b/c",
+			sub:  "a",
+			want: 0,
+		},
+		{
+			desc: "match_start_2",
+			p:    "aa/bb/cc",
+			sub:  "aa/bb",
+			want: 0,
+		},
+		{
+			desc: "match_first",
+			p:    "aa/aa",
+			sub:  "aa",
+			want: 0,
+		},
+		{
+			desc: "match_full",
+			p:    "aaa/bbb/ccc",
+			sub:  "aaa/bbb/ccc",
+			want: 0,
+		},
+		{
+			desc: "match_middle_2",
+			p:    "a/b/c/d",
+			sub:  "b/c",
+			want: 2,
+		},
+		{
+			desc: "match_end_2",
+			p:    "aa/bb/cc",
+			sub:  "bb/cc",
+			want: 3,
+		},
+		{
+			desc: "match_end_1",
+			p:    "a/b/c",
+			sub:  "c",
+			want: 4,
+		},
+		{
+			desc: "partial_match_start",
+			p:    "aa/bb",
+			sub:  "aa/b",
+			want: -1,
+		},
+		{
+			desc: "partial_match_end",
+			p:    "aa/bb",
+			sub:  "a/bb",
+			want: -1,
+		},
+		{
+			desc: "match_abs_both_start",
+			p:    "/a/b",
+			sub:  "/a",
+			want: 0,
+		},
+		{
+			desc: "match_abs_p_start",
+			p:    "/a/b",
+			sub:  "a",
+			want: 1,
+		},
+		{
+			desc: "match_abs_sub_start",
+			p:    "a/b",
+			sub:  "/a",
+			want: -1,
+		},
+		{
+			desc: "partial_match_abs",
+			p:    "/aa/bb",
+			sub:  "/aa/b",
+			want: -1,
+		},
+		{
+			desc: "match_unclean_dots",
+			p:    "a/b/../c",
+			sub:  "b/..",
+			want: 2,
+		},
+		{
+			desc: "match_unclean_slashes",
+			p:    "a/b//c",
+			sub:  "b//c",
+			want: 2,
+		},
+		{
+			desc: "match_unclean_slashes_no",
+			p:    "a/b//c",
+			sub:  "b/c",
+			want: -1,
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			if got := Index(tc.p, tc.sub); got != tc.want {
+				t.Errorf("got %d; want %d", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestLastIndex(t *testing.T) {
+	for _, tc := range []struct {
+		desc, p, sub string
+		want         int
+	}{
+		{
+			desc: "empty",
+			p:    "",
+			sub:  "",
+			want: 0,
+		},
+		{
+			desc: "empty_p",
+			p:    "",
+			sub:  "a",
+			want: -1,
+		},
+		{
+			desc: "empty_sub",
+			p:    "a",
+			sub:  "",
+			want: 1,
+		},
+		{
+			desc: "match_start_1",
+			p:    "a/b/c",
+			sub:  "a",
+			want: 0,
+		},
+		{
+			desc: "match_start_2",
+			p:    "aa/bb/cc",
+			sub:  "aa/bb",
+			want: 0,
+		},
+		{
+			desc: "match_last",
+			p:    "aa/aa",
+			sub:  "aa",
+			want: 3,
+		},
+		{
+			desc: "match_full",
+			p:    "aaa/bbb/ccc",
+			sub:  "aaa/bbb/ccc",
+			want: 0,
+		},
+		{
+			desc: "match_middle_2",
+			p:    "a/b/c/d",
+			sub:  "b/c",
+			want: 2,
+		},
+		{
+			desc: "match_end_2",
+			p:    "aa/bb/cc",
+			sub:  "bb/cc",
+			want: 3,
+		},
+		{
+			desc: "match_end_1",
+			p:    "a/b/c",
+			sub:  "c",
+			want: 4,
+		},
+		{
+			desc: "partial_match_start",
+			p:    "aa/bb",
+			sub:  "aa/b",
+			want: -1,
+		},
+		{
+			desc: "partial_match_end",
+			p:    "aa/bb",
+			sub:  "a/bb",
+			want: -1,
+		},
+		{
+			desc: "match_abs_both_start",
+			p:    "/a/b",
+			sub:  "/a",
+			want: 0,
+		},
+		{
+			desc: "match_abs_p_start",
+			p:    "/a/b",
+			sub:  "a",
+			want: 1,
+		},
+		{
+			desc: "match_abs_sub_start",
+			p:    "a/b",
+			sub:  "/a",
+			want: -1,
+		},
+		{
+			desc: "partial_match_abs",
+			p:    "/aa/bb",
+			sub:  "/aa/b",
+			want: -1,
+		},
+		{
+			desc: "match_unclean_dots",
+			p:    "a/b/../c",
+			sub:  "b/..",
+			want: 2,
+		},
+		{
+			desc: "match_unclean_slashes",
+			p:    "a/b//c",
+			sub:  "b//c",
+			want: 2,
+		},
+		{
+			desc: "match_unclean_slashes_no",
+			p:    "a/b//c",
+			sub:  "b/c",
+			want: -1,
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			if got := LastIndex(tc.p, tc.sub); got != tc.want {
+				t.Errorf("got %d; want %d", got, tc.want)
+			}
+		})
+	}
+}
+
 func TestHasPrefix(t *testing.T) {
 	for _, tc := range []struct {
 		desc, path, prefix string
diff --git a/tests/go_internal_nested_visibility/BUILD.in b/tests/go_internal_nested_visibility/BUILD.in
new file mode 100644
index 0000000..496fea7
--- /dev/null
+++ b/tests/go_internal_nested_visibility/BUILD.in
@@ -0,0 +1 @@
+# gazelle:prefix example.com/m
diff --git a/tests/go_internal_nested_visibility/BUILD.out b/tests/go_internal_nested_visibility/BUILD.out
new file mode 100644
index 0000000..496fea7
--- /dev/null
+++ b/tests/go_internal_nested_visibility/BUILD.out
@@ -0,0 +1 @@
+# gazelle:prefix example.com/m
diff --git a/tests/go_internal_nested_visibility/MODULE.bazel b/tests/go_internal_nested_visibility/MODULE.bazel
new file mode 100644
index 0000000..e69de29
diff --git a/tests/go_internal_nested_visibility/README.md b/tests/go_internal_nested_visibility/README.md
new file mode 100644
index 0000000..90491b9
--- /dev/null
+++ b/tests/go_internal_nested_visibility/README.md
@@ -0,0 +1,8 @@
+This test checks how the Go extension generates visibility directives
+for nested internal directories.
+
+For `//internal`, visibility should be `//:__subpackages__`.
+
+For `//a/internal`, visibility should be `//a:__subpackages__`.
+
+For `//internal/a/internal/b`, visibility should be `//internal/a:__subpackages__`.
diff --git a/tests/go_internal_nested_visibility/a/internal/BUILD.out b/tests/go_internal_nested_visibility/a/internal/BUILD.out
new file mode 100644
index 0000000..18b1240
--- /dev/null
+++ b/tests/go_internal_nested_visibility/a/internal/BUILD.out
@@ -0,0 +1,8 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "internal",
+    srcs = ["internal.go"],
+    importpath = "example.com/m/a/internal",
+    visibility = ["//a:__subpackages__"],
+)
diff --git a/tests/go_internal_nested_visibility/a/internal/internal.go b/tests/go_internal_nested_visibility/a/internal/internal.go
new file mode 100644
index 0000000..5bf0569
--- /dev/null
+++ b/tests/go_internal_nested_visibility/a/internal/internal.go
@@ -0,0 +1 @@
+package internal
diff --git a/tests/go_internal_nested_visibility/expectedStderr.txt b/tests/go_internal_nested_visibility/expectedStderr.txt
new file mode 100644
index 0000000..e69de29
diff --git a/tests/go_internal_nested_visibility/internal/BUILD.out b/tests/go_internal_nested_visibility/internal/BUILD.out
new file mode 100644
index 0000000..1c6015e
--- /dev/null
+++ b/tests/go_internal_nested_visibility/internal/BUILD.out
@@ -0,0 +1,8 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "internal",
+    srcs = ["internal.go"],
+    importpath = "example.com/m/internal",
+    visibility = ["//:__subpackages__"],
+)
diff --git a/tests/go_internal_nested_visibility/internal/a/internal/b/BUILD.out b/tests/go_internal_nested_visibility/internal/a/internal/b/BUILD.out
new file mode 100644
index 0000000..a74d6ac
--- /dev/null
+++ b/tests/go_internal_nested_visibility/internal/a/internal/b/BUILD.out
@@ -0,0 +1,8 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "b",
+    srcs = ["b.go"],
+    importpath = "example.com/m/internal/a/internal/b",
+    visibility = ["//internal/a:__subpackages__"],
+)
diff --git a/tests/go_internal_nested_visibility/internal/a/internal/b/b.go b/tests/go_internal_nested_visibility/internal/a/internal/b/b.go
new file mode 100644
index 0000000..e0836a8
--- /dev/null
+++ b/tests/go_internal_nested_visibility/internal/a/internal/b/b.go
@@ -0,0 +1 @@
+package b
diff --git a/tests/go_internal_nested_visibility/internal/internal.go b/tests/go_internal_nested_visibility/internal/internal.go
new file mode 100644
index 0000000..5bf0569
--- /dev/null
+++ b/tests/go_internal_nested_visibility/internal/internal.go
@@ -0,0 +1 @@
+package internal