fix(gazelle): Do not resolve absolute imports to sibling modules (#3106)

Currently, gazelle allows absolute imports to be resolved to sibling
modules: an `import foo` statement will resolve to a `foo.py` file in
the same folder if such file exists. This seems to be a Python 2
behavior (ie. pre-`from __future__ import absolute_import`), and doesn't
work on the current rules_python setup.

This behavior is explicitly tested in the
[siblings_import](https://github.com/bazel-contrib/rules_python/tree/cbe6d38d01c14de46d90ea717d0f2090117533fa/gazelle/python/testdata/sibling_imports)
test case. However, recreating the exact same repository layout from
this test case and running `bazel test //pkg:unit_test`, the test fails
with the import failing.

This PR adds a new directive, `gazelle:python_resolve_sibling_imports`,
to allow disabling such behavior.

The actual changes are in 3 places:

- In `gazelle/python/target.go`, the directive is added to `if
t.siblingSrcs.Contains(fileName) && fileName !=
filepath.Base(dep.Filepath)`, which is where the import is converted to
a full absolute import if it matches a sibling file;
- In `gazelle/python/generate.go`, the handling of `conftest.py` was
dependent on this behavior (ie. it added a dependency on the module
`conftest`, assuming that it would be resolved to the relative module).
That was modified to compute the full absolute module path instead.
- In `gazelle/python/resolve.go`, resolve relative imports even when
using file generation mode.

I also explicitly added `gazelle:python_resolve_sibling_imports true` to
any test that breaks if the default value of this directive is changed
to `false`.

Author: amartani

CHANGELOG.md

@@ -95,6 +95,10 @@ END_UNRELEASED_TEMPLATE
   ([#2921](https://github.com/bazel-contrib/rules_python/issues/2921)).
 * (repl) Normalize the path for the `REPL` stub to make it possible to use the
   default stub template from outside `rules_python` ({gh-issue}`3101`).
+* (gazelle) Fixes gazelle adding sibling module dependencies to resolve
+  absolute imports (Python 2's behavior without `absolute_import`). Previous
+  behavior can be restored using the directive
+  `# gazelle:python_resolve_sibling_imports true`
 
 {#v0-0-0-added}
 ### Added

gazelle/README.md

@@ -228,6 +228,8 @@ Python-specific directives are as follows:
 | Controls whether to generate a separate `pyi_deps` attribute for type-checking dependencies or merge them into the regular `deps` attribute. When `false` (default), type-checking dependencies are merged into `deps` for backward compatibility. When `true`, generates separate `pyi_deps`. Imports in blocks with the format `if typing.TYPE_CHECKING:`/`if TYPE_CHECKING:` and type-only stub packages (eg. boto3-stubs) are recognized as type-checking dependencies. |
 | [`# gazelle:python_generate_proto`](#directive-python_generate_proto)                                                                                                                                                                                                                                                                | `false` |
 | Controls whether to generate a `py_proto_library` for each `proto_library` in the package. By default we load this rule from the `@protobuf` repository; use `gazelle:map_kind` if you need to load this from somewhere else. |
+| `# gazelle:python_resolve_sibling_imports` | `false` |
+| Allows absolute imports to be resolved to sibling modules (Python 2's behavior without `absolute_import`). |
 
 #### Directive: `python_root`:
 

gazelle/python/configure.go

@@ -72,6 +72,7 @@ func (py *Configurer) KnownDirectives() []string {
 		pythonconfig.GeneratePyiDeps,
 		pythonconfig.ExperimentalAllowRelativeImports,
 		pythonconfig.GenerateProto,
+		pythonconfig.PythonResolveSiblingImports,
 	}
 }
 
@@ -247,6 +248,12 @@ func (py *Configurer) Configure(c *config.Config, rel string, f *rule.File) {
 				log.Fatal(err)
 			}
 			config.SetGenerateProto(v)
+		case pythonconfig.PythonResolveSiblingImports:
+			v, err := strconv.ParseBool(strings.TrimSpace(d.Value))
+			if err != nil {
+				log.Fatal(err)
+			}
+			config.SetResolveSiblingImports(v)
 		}
 	}
 

gazelle/python/generate.go

@@ -259,7 +259,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 						fqTarget.String(), actualPyBinaryKind, err)
 					continue
 				}
-				pyBinary := newTargetBuilder(pyBinaryKind, pyBinaryTargetName, pythonProjectRoot, args.Rel, pyFileNames).
+				pyBinary := newTargetBuilder(pyBinaryKind, pyBinaryTargetName, pythonProjectRoot, args.Rel, pyFileNames, cfg.ResolveSiblingImports()).
 					addVisibility(visibility).
 					addSrc(filename).
 					addModuleDependencies(mainModules[filename]).
@@ -301,7 +301,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 			collisionErrors.Add(err)
 		}
 
-		pyLibrary := newTargetBuilder(pyLibraryKind, pyLibraryTargetName, pythonProjectRoot, args.Rel, pyFileNames).
+		pyLibrary := newTargetBuilder(pyLibraryKind, pyLibraryTargetName, pythonProjectRoot, args.Rel, pyFileNames, cfg.ResolveSiblingImports()).
 			addVisibility(visibility).
 			addSrcs(srcs).
 			addModuleDependencies(allDeps).
@@ -354,7 +354,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 			collisionErrors.Add(err)
 		}
 
-		pyBinaryTarget := newTargetBuilder(pyBinaryKind, pyBinaryTargetName, pythonProjectRoot, args.Rel, pyFileNames).
+		pyBinaryTarget := newTargetBuilder(pyBinaryKind, pyBinaryTargetName, pythonProjectRoot, args.Rel, pyFileNames, cfg.ResolveSiblingImports()).
 			setMain(pyBinaryEntrypointFilename).
 			addVisibility(visibility).
 			addSrc(pyBinaryEntrypointFilename).
@@ -387,7 +387,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 			collisionErrors.Add(err)
 		}
 
-		conftestTarget := newTargetBuilder(pyLibraryKind, conftestTargetname, pythonProjectRoot, args.Rel, pyFileNames).
+		conftestTarget := newTargetBuilder(pyLibraryKind, conftestTargetname, pythonProjectRoot, args.Rel, pyFileNames, cfg.ResolveSiblingImports()).
 			addSrc(conftestFilename).
 			addModuleDependencies(deps).
 			addResolvedDependencies(annotations.includeDeps).
@@ -419,7 +419,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 				fqTarget.String(), actualPyTestKind, err, pythonconfig.TestNamingConvention)
 			collisionErrors.Add(err)
 		}
-		return newTargetBuilder(pyTestKind, pyTestTargetName, pythonProjectRoot, args.Rel, pyFileNames).
+		return newTargetBuilder(pyTestKind, pyTestTargetName, pythonProjectRoot, args.Rel, pyFileNames, cfg.ResolveSiblingImports()).
 			addSrcs(srcs).
 			addModuleDependencies(deps).
 			addResolvedDependencies(annotations.includeDeps).
@@ -476,7 +476,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
 
 	for _, pyTestTarget := range pyTestTargets {
 		if conftest != nil {
-			conftestModule := Module{Name: strings.TrimSuffix(conftestFilename, ".py")}
+			conftestModule := Module{Name: importSpecFromSrc(pythonProjectRoot, args.Rel, conftestFilename).Imp}
 			if pyTestTarget.annotations.includePytestConftest == nil {
 				// unset; default behavior
 				pyTestTarget.addModuleDependency(conftestModule)
@@ -605,7 +605,7 @@ func generateProtoLibraries(args language.GenerateArgs, cfg *pythonconfig.Config
 			pyProtoLibraryName = ruleName
 		}
 
-		pyProtoLibrary := newTargetBuilder(pyProtoLibraryKind, pyProtoLibraryName, pythonProjectRoot, args.Rel, &emptySiblings).
+		pyProtoLibrary := newTargetBuilder(pyProtoLibraryKind, pyProtoLibraryName, pythonProjectRoot, args.Rel, &emptySiblings, false).
 			addVisibility(visibility).
 			addResolvedDependency(":" + protoRuleName).
 			generateImportsAttribute().build()
@@ -622,7 +622,7 @@ func generateProtoLibraries(args language.GenerateArgs, cfg *pythonconfig.Config
 			continue
 		}
 
-		emptyRule := newTargetBuilder(pyProtoLibraryKind, ruleName, pythonProjectRoot, args.Rel, &emptySiblings).build()
+		emptyRule := newTargetBuilder(pyProtoLibraryKind, ruleName, pythonProjectRoot, args.Rel, &emptySiblings, false).build()
 		res.Empty = append(res.Empty, emptyRule)
 	}
 

gazelle/python/resolve.go

@@ -164,16 +164,14 @@ func (py *Resolver) Resolve(
 		modules := modulesRaw.(*treeset.Set)
 		it := modules.Iterator()
 		explainDependency := os.Getenv("EXPLAIN_DEPENDENCY")
-		// Resolve relative paths for package generation
-		isPackageGeneration := !cfg.PerFileGeneration() && !cfg.CoarseGrainedGeneration()
 		hasFatalError := false
 	MODULES_LOOP:
 		for it.Next() {
 			mod := it.Value().(Module)
 			moduleName := mod.Name
 			// Transform relative imports `.` or `..foo.bar` into the package path from root.
 			if strings.HasPrefix(mod.From, ".") {
-				if !cfg.ExperimentalAllowRelativeImports() || !isPackageGeneration {
+				if !cfg.ExperimentalAllowRelativeImports() {
 					continue MODULES_LOOP
 				}
 
@@ -210,9 +208,9 @@ func (py *Resolver) Resolve(
 					baseParts = pkgParts[:len(pkgParts)-(relativeDepth-1)]
 				}
 				// Build absolute module path
-				absParts := append([]string{}, baseParts...)       // base path
-				absParts = append(absParts, fromParts...)          // subpath from 'from'
-				absParts = append(absParts, imported)              // actual imported symbol
+				absParts := append([]string{}, baseParts...) // base path
+				absParts = append(absParts, fromParts...)    // subpath from 'from'
+				absParts = append(absParts, imported)        // actual imported symbol
 
 				moduleName = strings.Join(absParts, ".")
 			}

gazelle/python/target.go

@@ -25,34 +25,36 @@ import (
 
 // targetBuilder builds targets to be generated by Gazelle.
 type targetBuilder struct {
-	kind              string
-	name              string
-	pythonProjectRoot string
-	bzlPackage        string
-	srcs              *treeset.Set
-	siblingSrcs       *treeset.Set
-	deps              *treeset.Set
-	resolvedDeps      *treeset.Set
-	visibility        *treeset.Set
-	main              *string
-	imports           []string
-	testonly          bool
-	annotations       *annotations
+	kind                  string
+	name                  string
+	pythonProjectRoot     string
+	bzlPackage            string
+	srcs                  *treeset.Set
+	siblingSrcs           *treeset.Set
+	deps                  *treeset.Set
+	resolvedDeps          *treeset.Set
+	visibility            *treeset.Set
+	main                  *string
+	imports               []string
+	testonly              bool
+	annotations           *annotations
+	resolveSiblingImports bool
 }
 
 // newTargetBuilder constructs a new targetBuilder.
-func newTargetBuilder(kind, name, pythonProjectRoot, bzlPackage string, siblingSrcs *treeset.Set) *targetBuilder {
+func newTargetBuilder(kind, name, pythonProjectRoot, bzlPackage string, siblingSrcs *treeset.Set, resolveSiblingImports bool) *targetBuilder {
 	return &targetBuilder{
-		kind:              kind,
-		name:              name,
-		pythonProjectRoot: pythonProjectRoot,
-		bzlPackage:        bzlPackage,
-		srcs:              treeset.NewWith(godsutils.StringComparator),
-		siblingSrcs:       siblingSrcs,
-		deps:              treeset.NewWith(moduleComparator),
-		resolvedDeps:      treeset.NewWith(godsutils.StringComparator),
-		visibility:        treeset.NewWith(godsutils.StringComparator),
-		annotations:       new(annotations),
+		kind:                  kind,
+		name:                  name,
+		pythonProjectRoot:     pythonProjectRoot,
+		bzlPackage:            bzlPackage,
+		srcs:                  treeset.NewWith(godsutils.StringComparator),
+		siblingSrcs:           siblingSrcs,
+		deps:                  treeset.NewWith(moduleComparator),
+		resolvedDeps:          treeset.NewWith(godsutils.StringComparator),
+		visibility:            treeset.NewWith(godsutils.StringComparator),
+		annotations:           new(annotations),
+		resolveSiblingImports: resolveSiblingImports,
 	}
 }
 
@@ -77,7 +79,7 @@ func (t *targetBuilder) addModuleDependency(dep Module) *targetBuilder {
 	if dep.From != "" {
 		fileName = dep.From + ".py"
 	}
-	if t.siblingSrcs.Contains(fileName) && fileName != filepath.Base(dep.Filepath) {
+	if t.resolveSiblingImports && t.siblingSrcs.Contains(fileName) && fileName != filepath.Base(dep.Filepath) {
 		// importing another module from the same package, converting to absolute imports to make
 		// dependency resolution easier
 		dep.Name = importSpecFromSrc(t.pythonProjectRoot, t.bzlPackage, fileName).Imp
@@ -138,7 +140,6 @@ func (t *targetBuilder) setAnnotations(val annotations) *targetBuilder {
 	return t
 }
 
-
 // generateImportsAttribute generates the imports attribute.
 // These are a list of import directories to be added to the PYTHONPATH. In our
 // case, the value we add is on Bazel sub-packages to be able to perform imports

gazelle/python/testdata/annotation_include_dep/BUILD.in

@@ -1 +1,2 @@
 # gazelle:python_generation_mode file
+# gazelle:python_resolve_sibling_imports true

gazelle/python/testdata/annotation_include_dep/BUILD.out

@@ -1,6 +1,7 @@
 load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test")
 
 # gazelle:python_generation_mode file
+# gazelle:python_resolve_sibling_imports true
 
 py_library(
     name = "__init__",

gazelle/python/testdata/annotation_include_pytest_conftest/with_conftest/BUILD.in

@@ -0,0 +1 @@
+# gazelle:python_resolve_sibling_imports true

gazelle/python/testdata/annotation_include_pytest_conftest/with_conftest/BUILD.out

@@ -1,5 +1,7 @@
 load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test")
 
+# gazelle:python_resolve_sibling_imports true
+
 py_binary(
     name = "binary",
     srcs = ["binary.py"],