fix(cli): unexpected lint warning when providing multiple files (#1496)

* fix(cli): remove second resolveFilenames
* chore(cli-test): test resolveImports
* chore(cli): add import resolution function
* chore(cli): consolidate file resolution
---------

Co-authored-by: Noah Dietz <[email protected]>

Author: quirogas

cmd/api-linter/cli.go

@@ -19,6 +19,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 	"sync"
 
@@ -79,7 +80,7 @@ func newCli(args []string) *cli {
 	fs.StringArrayVar(&protoDescFlag, "descriptor-set-in", nil, "The file containing a FileDescriptorSet for searching proto imports.\nMay be specified multiple times.")
 	fs.StringArrayVar(&ruleEnableFlag, "enable-rule", nil, "Enable a rule with the given name.\nMay be specified multiple times.")
 	fs.StringArrayVar(&ruleDisableFlag, "disable-rule", nil, "Disable a rule with the given name.\nMay be specified multiple times.")
-	fs.BoolVar(&listRulesFlag, "list-rules", false, "Print the rules and exit.  Honors the output-format flag.")
+	fs.BoolVar(&listRulesFlag, "list-rules", false, "Print the rules and exit. Honors the output-format flag.")
 	fs.BoolVar(&debugFlag, "debug", false, "Run in debug mode. Panics will print stack.")
 	fs.BoolVar(&ignoreCommentDisablesFlag, "ignore-comment-disables", false, "If set to true, disable comments will be ignored.\nThis is helpful when strict enforcement of AIPs are necessary and\nproto definitions should not be able to disable checks.")
 
@@ -150,9 +151,12 @@ func (c *cli) lint(rules lint.RuleRegistry, configs lint.Configs) error {
 	}
 	var errorsWithPos []protoparse.ErrorWithPos
 	var lock sync.Mutex
+
+	imports := resolveImports(c.ProtoImportPaths)
+
 	// Parse proto files into `protoreflect` file descriptors.
 	p := protoparse.Parser{
-		ImportPaths:           append(c.ProtoImportPaths, "."),
+		ImportPaths:           imports,
 		IncludeSourceCodeInfo: true,
 		LookupImport:          lookupImport,
 		ErrorReporter: func(errorWithPos protoparse.ErrorWithPos) error {
@@ -165,21 +169,7 @@ func (c *cli) lint(rules lint.RuleRegistry, configs lint.Configs) error {
 		},
 	}
 	// Resolve file absolute paths to relative ones.
-	// Using supplied import paths first.
-	protoFiles := c.ProtoFiles
-	if len(c.ProtoImportPaths) > 0 {
-		protoFiles, err = protoparse.ResolveFilenames(c.ProtoImportPaths, c.ProtoFiles...)
-		if err != nil {
-			return err
-		}
-	}
-	// Then resolve again against ".", the local directory.
-	// This is necessary because ResolveFilenames won't resolve a path if it
-	// relative to *at least one* of the given import paths, which can result
-	// in duplicate file parsing and compilation errors, as seen in #1465 and
-	// #1471. So we resolve against local (default) and flag specified import
-	// paths separately.
-	protoFiles, err = protoparse.ResolveFilenames([]string{"."}, protoFiles...)
+	protoFiles, err := protoparse.ResolveFilenames(p.ImportPaths, c.ProtoFiles...)
 	if err != nil {
 		return err
 	}
@@ -305,3 +295,75 @@ func getOutputFormatFunc(formatType string) formatFunc {
 	}
 	return yaml.Marshal
 }
+
+func resolveImports(imports []string) []string {
+	// If no import paths are provided, default to the current directory.
+	if len(imports) == 0 {
+		return []string{"."}
+	}
+
+	// Get the absolute path of the current working directory.
+	cwd, err := os.Getwd()
+	if err != nil {
+		// Fallback: If we can't get CWD, return only the provided paths and "."
+		seen := map[string]bool{
+			".": true,
+		}
+		result := []string{"."} // Always include "."
+		for _, p := range imports {
+			if !seen[p] {
+				seen[p] = true
+				result = append(result, p)
+			}
+		}
+		return result
+	}
+
+	// Resolve the canonical path for the current working directory.
+	// This helps with symlinks (e.g., /var vs /private/var on macOS).
+	evaluatedCwd, err := filepath.EvalSymlinks(cwd)
+	if err != nil {
+		// Fallback to Clean if EvalSymlinks fails (e.g., path does not exist)
+		evaluatedCwd = filepath.Clean(cwd)
+	}
+
+	// Initialize resolvedImports with "." and track its canonical absolute path.
+	resolvedImports := []string{"."}
+	seenAbsolutePaths := map[string]bool{
+		evaluatedCwd: true, // Mark canonical CWD as seen
+	}
+
+	for _, p := range imports {
+		absPath, err := filepath.Abs(p)
+		if err != nil {
+			// If we can't get the absolute path, treat it as an external path
+			// and add it if not already seen (by its original string form).
+			if !seenAbsolutePaths[p] {
+				seenAbsolutePaths[p] = true
+				resolvedImports = append(resolvedImports, p)
+			}
+			continue
+		}
+
+		// Resolve the canonical path for the current import path.
+		evaluatedAbsPath, err := filepath.EvalSymlinks(absPath)
+		if err != nil {
+			// Fallback to Clean if EvalSymlinks fails
+			evaluatedAbsPath = filepath.Clean(absPath)
+		}
+
+		// Check if the current import path's canonical form is the CWD's canonical form
+		// or a subdirectory of it. If so, it's covered by ".", so we skip it.
+		if evaluatedAbsPath == evaluatedCwd || strings.HasPrefix(evaluatedAbsPath, evaluatedCwd+string(os.PathSeparator)) {
+			continue
+		}
+
+		// Add the original path if its canonical absolute form has not been seen before.
+		if !seenAbsolutePaths[evaluatedAbsPath] {
+			seenAbsolutePaths[evaluatedAbsPath] = true
+			resolvedImports = append(resolvedImports, p)
+		}
+	}
+
+	return resolvedImports
+}

cmd/api-linter/cli_test.go

@@ -1,6 +1,9 @@
 package main
 
 import (
+	"os"
+	"path/filepath"
+	"sort"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -54,3 +57,145 @@ func TestNewCli(t *testing.T) {
 		})
 	}
 }
+
+func TestResolveImports(t *testing.T) {
+	// Save the original working directory and restore it at the end of the test.
+	originalCWD, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("Failed to get original working directory: %v", err)
+	}
+	defer func() {
+		if err := os.Chdir(originalCWD); err != nil {
+			t.Errorf("Failed to restore original working directory: %v", err)
+		}
+	}()
+
+	defaultSetup := func(t *testing.T, cwd, externalDir string) {}
+
+	tests := []struct {
+		name             string
+		protoImportPaths func(cwd, externalDir string) []string
+		setup            func(t *testing.T, cwd, externalDir string)
+		want             func(externalDir string) []string
+	}{
+		{
+			name: "NoProtoImportPaths",
+			protoImportPaths: func(_, _ string) []string {
+				return []string{}
+			},
+			setup: defaultSetup,
+			want: func(_ string) []string {
+				return []string{"."}
+			},
+		},
+		{
+			name: "ExplicitDot",
+			protoImportPaths: func(_, _ string) []string {
+				return []string{"."}
+			},
+			setup: defaultSetup,
+			want: func(_ string) []string {
+				return []string{"."}
+			},
+		},
+		{
+			name: "SubdirectoryOfCWD",
+			protoImportPaths: func(_, _ string) []string {
+				return []string{"test_dir"}
+			},
+			setup: func(t *testing.T, cwd, _ string) {
+				if err := os.Mkdir(filepath.Join(cwd, "test_dir"), 0755); err != nil {
+					t.Fatalf("Failed to create temp subdirectory: %v", err)
+				}
+			},
+			want: func(_ string) []string {
+				return []string{"."} // "test_dir" should be covered by "."
+			},
+		},
+		{
+			name: "SubdirectoryOfCWDWithDot",
+			protoImportPaths: func(_, _ string) []string {
+				return []string{".", "test_dir"}
+			},
+			setup: func(t *testing.T, cwd, _ string) {
+				if err := os.Mkdir(filepath.Join(cwd, "test_dir"), 0755); err != nil {
+					t.Fatalf("Failed to create temp subdirectory: %v", err)
+				}
+			},
+			want: func(_ string) []string {
+				return []string{"."}
+			},
+		},
+		{
+			name: "ExternalAbsolutePath",
+			protoImportPaths: func(_, externalDir string) []string {
+				return []string{externalDir}
+			},
+			setup: defaultSetup,
+			want: func(externalDir string) []string {
+				return []string{".", externalDir}
+			},
+		},
+		{
+			name: "MixedPaths",
+			protoImportPaths: func(_, externalDir string) []string {
+				return []string{"./relative_dir", externalDir, "test_dir"}
+			},
+			setup: func(t *testing.T, cwd, _ string) {
+				if err := os.Mkdir(filepath.Join(cwd, "relative_dir"), 0755); err != nil {
+					t.Fatalf("Failed to create relative_dir: %v", err)
+				}
+				if err := os.Mkdir(filepath.Join(cwd, "test_dir"), 0755); err != nil {
+					t.Fatalf("Failed to create test_dir: %v", err)
+				}
+			},
+			want: func(externalDir string) []string {
+				return []string{".", externalDir}
+			},
+		},
+		{
+			name: "NonExistentRelativePath",
+			protoImportPaths: func(_, _ string) []string {
+				return []string{"non_existent_dir"}
+			},
+			setup: defaultSetup,
+			want: func(_ string) []string {
+				return []string{"."} // Should still resolve to just "." as non_existent_dir is relative to CWD
+			},
+		},
+		{
+			name: "CurrentDirAsAbsolutePath",
+			protoImportPaths: func(cwd, _ string) []string {
+				return []string{cwd}
+			},
+			setup: defaultSetup,
+			want: func(_ string) []string {
+				return []string{"."}
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			cwd := t.TempDir()
+			externalDir := t.TempDir()
+
+			if err := os.Chdir(cwd); err != nil {
+				t.Fatalf("Failed to change directory to %q: %v", cwd, err)
+			}
+
+			test.setup(t, cwd, externalDir)
+
+			protoImportPaths := test.protoImportPaths(cwd, externalDir)
+			want := test.want(externalDir)
+
+			got := resolveImports(protoImportPaths)
+			sort.Strings(got)
+			sort.Strings(want)
+
+			if diff := cmp.Diff(want, got); diff != "" {
+				t.Errorf("resolveImports() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}

cmd/api-linter/integration_test.go

@@ -182,9 +182,6 @@ func TestMultipleFilesFromParentDir(t *testing.T) {
 	// This test addresses a previously found bug:
 	// https://github.com/googleapis/api-linter/issues/1465
 
-	// Skipping until Issue # 1465 is addressed
-	t.Skip("Skipping until Issue # 1465 is addressed")
-
 	projDir, err := os.MkdirTemp("", "proj")
 	if err != nil {
 		t.Fatal(err)