fix(gazelle) Fix dependency added as both deps and pyi_deps (#3036)

Fix an issue in #3014
where a dependency may end up being added in both `deps` and `pyi_deps`,
in cases where the regular and the type-checking import refer to
different python modules on the same `py_library` target. Other cases
are already deduplicated earlier on, but this case can only be
deduplicated in the resolve phase.

(No new changelog entry since this is a fix to an unreleased feature
that is already in the changelog)

Author: amartani

gazelle/python/resolve.go

@@ -124,12 +124,16 @@ func (py *Resolver) Embeds(r *rule.Rule, from label.Label) []label.Label {
 }
 
 // addDependency adds a dependency to either the regular deps or pyiDeps set based on
-// whether the module is type-checking only.
-func addDependency(dep string, mod Module, deps, pyiDeps *treeset.Set) {
-	if mod.TypeCheckingOnly {
-		pyiDeps.Add(dep)
+// whether the module is type-checking only. If a module is added as both
+// non-type-checking and type-checking, it should end up in deps and not pyiDeps.
+func addDependency(dep string, typeCheckingOnly bool, deps, pyiDeps *treeset.Set) {
+	if typeCheckingOnly {
+		if !deps.Contains(dep) {
+			pyiDeps.Add(dep)
+		}
 	} else {
 		deps.Add(dep)
+		pyiDeps.Remove(dep)
 	}
 }
 
@@ -240,7 +244,7 @@ func (py *Resolver) Resolve(
 							override.Repo = ""
 						}
 						dep := override.Rel(from.Repo, from.Pkg).String()
-						addDependency(dep, mod, deps, pyiDeps)
+						addDependency(dep, mod.TypeCheckingOnly, deps, pyiDeps)
 						if explainDependency == dep {
 							log.Printf("Explaining dependency (%s): "+
 								"in the target %q, the file %q imports %q at line %d, "+
@@ -251,7 +255,7 @@ func (py *Resolver) Resolve(
 					}
 				} else {
 					if dep, distributionName, ok := cfg.FindThirdPartyDependency(moduleName); ok {
-						addDependency(dep, mod, deps, pyiDeps)
+						addDependency(dep, mod.TypeCheckingOnly, deps, pyiDeps)
 						// Add the type and stub dependencies if they exist.
 						modules := []string{
 							fmt.Sprintf("%s_stubs", strings.ToLower(distributionName)),
@@ -261,8 +265,8 @@ func (py *Resolver) Resolve(
 						}
 						for _, module := range modules {
 							if dep, _, ok := cfg.FindThirdPartyDependency(module); ok {
-								// Type stub packages always go to pyiDeps
-								pyiDeps.Add(dep)
+								// Type stub packages are added as type-checking only.
+								addDependency(dep, true, deps, pyiDeps)
 							}
 						}
 						if explainDependency == dep {
@@ -321,7 +325,7 @@ func (py *Resolver) Resolve(
 						}
 						matchLabel := filteredMatches[0].Label.Rel(from.Repo, from.Pkg)
 						dep := matchLabel.String()
-						addDependency(dep, mod, deps, pyiDeps)
+						addDependency(dep, mod.TypeCheckingOnly, deps, pyiDeps)
 						if explainDependency == dep {
 							log.Printf("Explaining dependency (%s): "+
 								"in the target %q, the file %q imports %q at line %d, "+

gazelle/python/testdata/type_checking_imports_across_packages/BUILD.in

@@ -0,0 +1,2 @@
+# gazelle:python_generation_mode package
+# gazelle:python_generate_pyi_deps true

gazelle/python/testdata/type_checking_imports_across_packages/BUILD.out

@@ -0,0 +1,2 @@
+# gazelle:python_generation_mode package
+# gazelle:python_generate_pyi_deps true

gazelle/python/testdata/type_checking_imports_across_packages/README.md

@@ -0,0 +1,6 @@
+# Overlapping deps and pyi_deps across packages
+
+This test reproduces a case where a dependency may be added to both `deps` and
+`pyi_deps`. Package `b` imports `a.foo` normally and imports `a.bar` as a
+type-checking only import. The dependency on package `a` should appear only in
+`deps` (and not `pyi_deps`) of package `b`.

gazelle/python/testdata/type_checking_imports_across_packages/WORKSPACE

@@ -0,0 +1 @@
+workspace(name = "gazelle_python_test")

gazelle/python/testdata/type_checking_imports_across_packages/a/BUILD.in

@@ -0,0 +1,10 @@
+load("@rules_python//python:defs.bzl", "py_library")
+
+py_library(
+    name = "a",
+    srcs = [
+        "bar.py",
+        "foo.py",
+    ],
+    visibility = ["//:__subpackages__"],
+)