From 6550bcc7c43308ced61adf2ee7d6d4ae8e74dfee Mon Sep 17 00:00:00 2001 From: Boris Kreitchman Date: Fri, 1 Aug 2025 17:32:29 +0300 Subject: [PATCH 1/3] go/analysis/passes/shadow: add filename if shadowed variable is in other file --- go/analysis/passes/shadow/shadow.go | 23 +++++++++++++++++-- go/analysis/passes/shadow/shadow_test.go | 5 ++++ .../testdata/src/crossfile/crossfile.go | 15 ++++++++++++ .../shadow/testdata/src/crossfile/other.go | 7 ++++++ 4 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 go/analysis/passes/shadow/testdata/src/crossfile/crossfile.go create mode 100644 go/analysis/passes/shadow/testdata/src/crossfile/other.go diff --git a/go/analysis/passes/shadow/shadow.go b/go/analysis/passes/shadow/shadow.go index 8f768bb76c5..afc14f3aab0 100644 --- a/go/analysis/passes/shadow/shadow.go +++ b/go/analysis/passes/shadow/shadow.go @@ -6,9 +6,11 @@ package shadow import ( _ "embed" + "fmt" "go/ast" "go/token" "go/types" + "path/filepath" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -262,7 +264,24 @@ func checkShadowing(pass *analysis.Pass, spans map[types.Object]span, ident *ast } // Don't complain if the types differ: that implies the programmer really wants two different things. if types.Identical(obj.Type(), shadowed.Type()) { - line := pass.Fset.Position(shadowed.Pos()).Line - pass.ReportRangef(ident, "declaration of %q shadows declaration at line %d", obj.Name(), line) + shadowedPos := pass.Fset.Position(shadowed.Pos()) + + // Build the message, adding filename only if in a different file + message := fmt.Sprintf("declaration of %q shadows declaration at line %d", obj.Name(), shadowedPos.Line) + currentFile := pass.Fset.Position(ident.Pos()).Filename + if shadowedPos.Filename != currentFile { + message += fmt.Sprintf(" in %s", filepath.Base(shadowedPos.Filename)) + } + + pass.Report(analysis.Diagnostic{ + Pos: ident.Pos(), + End: ident.End(), + Message: message, + Related: []analysis.RelatedInformation{{ + Pos: shadowed.Pos(), + End: shadowed.Pos() + token.Pos(len(shadowed.Name())), + Message: fmt.Sprintf("shadowed symbol %q declared here", obj.Name()), + }}, + }) } } diff --git a/go/analysis/passes/shadow/shadow_test.go b/go/analysis/passes/shadow/shadow_test.go index 4fcdc922ee5..4fb532f2aae 100644 --- a/go/analysis/passes/shadow/shadow_test.go +++ b/go/analysis/passes/shadow/shadow_test.go @@ -15,3 +15,8 @@ func Test(t *testing.T) { testdata := analysistest.TestData() analysistest.Run(t, testdata, shadow.Analyzer, "a") } + +func TestCrossFile(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, shadow.Analyzer, "crossfile") +} diff --git a/go/analysis/passes/shadow/testdata/src/crossfile/crossfile.go b/go/analysis/passes/shadow/testdata/src/crossfile/crossfile.go new file mode 100644 index 00000000000..e821012787d --- /dev/null +++ b/go/analysis/passes/shadow/testdata/src/crossfile/crossfile.go @@ -0,0 +1,15 @@ +// Copyright 2025 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// This file contains a test for the shadowed variable checker with cross-file reference. + +package crossfile + +func ShadowGlobal() { + { + global := 1 // want "declaration of .global. shadows declaration at line 7 in other.go" + _ = global + } + _ = global +} diff --git a/go/analysis/passes/shadow/testdata/src/crossfile/other.go b/go/analysis/passes/shadow/testdata/src/crossfile/other.go new file mode 100644 index 00000000000..108060a5437 --- /dev/null +++ b/go/analysis/passes/shadow/testdata/src/crossfile/other.go @@ -0,0 +1,7 @@ +// Copyright 2025 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package crossfile + +var global int From 28b865ab55cacab5fdff8fe64ac9ff4bf7cc7999 Mon Sep 17 00:00:00 2001 From: Boris Kreitchman Date: Sat, 2 Aug 2025 12:24:10 +0300 Subject: [PATCH 2/3] fix cross-file --- go/analysis/passes/shadow/shadow.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/go/analysis/passes/shadow/shadow.go b/go/analysis/passes/shadow/shadow.go index afc14f3aab0..2f935b9e91d 100644 --- a/go/analysis/passes/shadow/shadow.go +++ b/go/analysis/passes/shadow/shadow.go @@ -245,6 +245,10 @@ func checkShadowing(pass *analysis.Pass, spans map[types.Object]span, ident *ast if shadowed.Parent() == types.Universe { return } + + shadowedPos := pass.Fset.Position(shadowed.Pos()) + identPos := pass.Fset.Position(ident.Pos()) + if strict { // The shadowed identifier must appear before this one to be an instance of shadowing. if shadowed.Pos() > ident.Pos() { @@ -252,24 +256,23 @@ func checkShadowing(pass *analysis.Pass, spans map[types.Object]span, ident *ast } } else { // Don't complain if the span of validity of the shadowed identifier doesn't include - // the shadowing identifier. + // the shadowing identifier, except for cross-file shadowing where file processing + // order affects span checks. span, ok := spans[shadowed] if !ok { pass.ReportRangef(ident, "internal error: no range for %q", ident.Name) return } - if !span.contains(ident.Pos()) { + + if shadowedPos.Filename == identPos.Filename && !span.contains(ident.Pos()) { return } } // Don't complain if the types differ: that implies the programmer really wants two different things. if types.Identical(obj.Type(), shadowed.Type()) { - shadowedPos := pass.Fset.Position(shadowed.Pos()) - // Build the message, adding filename only if in a different file message := fmt.Sprintf("declaration of %q shadows declaration at line %d", obj.Name(), shadowedPos.Line) - currentFile := pass.Fset.Position(ident.Pos()).Filename - if shadowedPos.Filename != currentFile { + if shadowedPos.Filename != identPos.Filename { message += fmt.Sprintf(" in %s", filepath.Base(shadowedPos.Filename)) } From fe25404a266785c1e7bb834a41cd35ca2a8be74f Mon Sep 17 00:00:00 2001 From: Boris Kreitchman Date: Fri, 8 Aug 2025 14:03:38 +0300 Subject: [PATCH 3/3] go/analysis/passes/shadow: separate package-level variables logic --- go/analysis/passes/shadow/shadow.go | 74 +++++++++---------- .../testdata/src/crossfile/crossfile.go | 22 +++++- .../shadow/testdata/src/crossfile/other.go | 9 ++- 3 files changed, 62 insertions(+), 43 deletions(-) diff --git a/go/analysis/passes/shadow/shadow.go b/go/analysis/passes/shadow/shadow.go index 2f935b9e91d..aa35eeba835 100644 --- a/go/analysis/passes/shadow/shadow.go +++ b/go/analysis/passes/shadow/shadow.go @@ -16,6 +16,7 @@ import ( "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/analysis/passes/internal/analysisutil" "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/internal/typesinternal" ) // NOTE: Experimental. Not part of the vet suite. @@ -113,7 +114,7 @@ func (s span) contains(pos token.Pos) bool { // growSpan expands the span for the object to contain the source range [pos, end). func growSpan(spans map[types.Object]span, obj types.Object, pos, end token.Pos) { - if strict { + if strict || typesinternal.IsPackageLevel(obj) || isPkgName(obj) { return // No need } s, ok := spans[obj] @@ -245,46 +246,43 @@ func checkShadowing(pass *analysis.Pass, spans map[types.Object]span, ident *ast if shadowed.Parent() == types.Universe { return } - - shadowedPos := pass.Fset.Position(shadowed.Pos()) - identPos := pass.Fset.Position(ident.Pos()) - - if strict { - // The shadowed identifier must appear before this one to be an instance of shadowing. - if shadowed.Pos() > ident.Pos() { - return - } - } else { - // Don't complain if the span of validity of the shadowed identifier doesn't include - // the shadowing identifier, except for cross-file shadowing where file processing - // order affects span checks. - span, ok := spans[shadowed] - if !ok { - pass.ReportRangef(ident, "internal error: no range for %q", ident.Name) + // Package names (imports) don't have a type and are always in scope in the file, + // so they are always reported when shadowed. + if !isPkgName(shadowed) { + // Don't complain if the types differ: that implies the programmer really wants two different things. + if !types.Identical(obj.Type(), shadowed.Type()) { return } - - if shadowedPos.Filename == identPos.Filename && !span.contains(ident.Pos()) { - return + // Package-level variables are always in scope, so they're always reported when shadowed. + if !strict && !typesinternal.IsPackageLevel(shadowed) { + // Don't complain if the span of validity of the shadowed identifier doesn't include + // the shadowing identifier. + span, ok := spans[shadowed] + if !ok || !span.contains(ident.Pos()) { + return + } } } - // Don't complain if the types differ: that implies the programmer really wants two different things. - if types.Identical(obj.Type(), shadowed.Type()) { - // Build the message, adding filename only if in a different file - message := fmt.Sprintf("declaration of %q shadows declaration at line %d", obj.Name(), shadowedPos.Line) - if shadowedPos.Filename != identPos.Filename { - message += fmt.Sprintf(" in %s", filepath.Base(shadowedPos.Filename)) - } - - pass.Report(analysis.Diagnostic{ - Pos: ident.Pos(), - End: ident.End(), - Message: message, - Related: []analysis.RelatedInformation{{ - Pos: shadowed.Pos(), - End: shadowed.Pos() + token.Pos(len(shadowed.Name())), - Message: fmt.Sprintf("shadowed symbol %q declared here", obj.Name()), - }}, - }) + shadowedPos := pass.Fset.Position(shadowed.Pos()) + message := fmt.Sprintf("declaration of %q shadows declaration at line %d", obj.Name(), shadowedPos.Line) + currentFile := pass.Fset.Position(ident.Pos()).Filename + if shadowedPos.Filename != currentFile { + message += fmt.Sprintf(" in %s", filepath.Base(shadowedPos.Filename)) } + pass.Report(analysis.Diagnostic{ + Pos: ident.Pos(), + End: ident.End(), + Message: message, + Related: []analysis.RelatedInformation{{ + Pos: shadowed.Pos(), + End: shadowed.Pos() + token.Pos(len(shadowed.Name())), + Message: fmt.Sprintf("shadowed symbol %q declared here", obj.Name()), + }}, + }) +} + +// isPkgName reports whether obj is a package name (import). +func isPkgName(obj types.Object) bool { + _, ok := obj.(*types.PkgName) + return ok } diff --git a/go/analysis/passes/shadow/testdata/src/crossfile/crossfile.go b/go/analysis/passes/shadow/testdata/src/crossfile/crossfile.go index e821012787d..5380e28930c 100644 --- a/go/analysis/passes/shadow/testdata/src/crossfile/crossfile.go +++ b/go/analysis/passes/shadow/testdata/src/crossfile/crossfile.go @@ -6,10 +6,24 @@ package crossfile +import "fmt" + func ShadowGlobal() { - { - global := 1 // want "declaration of .global. shadows declaration at line 7 in other.go" - _ = global - } + global := 1 // want "declaration of .global. shadows declaration at line 8 in other.go" + _ = global +} + +func ShadowGlobalWithDifferentType() { + global := "text" // OK: different type. _ = global } + +func ShadowPackageName() { + fmt := "text" // want "declaration of .fmt. shadows declaration at line 9" + _ = fmt +} + +// To import fmt package +func PrintHelper() { + fmt.Println() +} diff --git a/go/analysis/passes/shadow/testdata/src/crossfile/other.go b/go/analysis/passes/shadow/testdata/src/crossfile/other.go index 108060a5437..c0275a8e406 100644 --- a/go/analysis/passes/shadow/testdata/src/crossfile/other.go +++ b/go/analysis/passes/shadow/testdata/src/crossfile/other.go @@ -4,4 +4,11 @@ package crossfile -var global int +var ( + global int +) + +func ShadowUnimportedPackageName() { + fmt := "text" // OK: fmt package is not imported in this file + _ = fmt +}