internal/analysis/gofix: simple type aliases

jba · jba · commit 85a3833c5213 · 2025-02-13T13:27:57.000-08:00
Offer to replace inlinable type aliases whose RHS is a named type. Despite the amount of new code, there is little to test, because most of it duplicates constants. When there is a chain of inlinable type aliases, as in: type A = T type AA = A var v AA we don't follow the chain to the end. Instead, the first set of fixes produces: type A = T type AA = T var v A That is, the replacements happen "in parallel." A second round of fixes would rewrite var v A to var v T This case is rare enough that it's not worth doing better. For golang/go#32816. Change-Id: Ib6854d60b26a273b592a297cb9a650a31e094392 Reviewed-on: https://go-review.googlesource.com/c/tools/+/649055 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/gopls/internal/analysis/gofix/gofix.go b/gopls/internal/analysis/gofix/gofix.go
@@ -63,8 +63,11 @@ func run(pass *analysis.Pass) (any, error) {
 	// Pass 1: find functions and constants annotated with an appropriate "//go:fix"
 	// comment (the syntax proposed by #32816),
 	// and export a fact for each one.
-	inlinableFuncs := make(map[*types.Func]*inline.Callee) // memoization of fact import (nil => no fact)
-	inlinableConsts := make(map[*types.Const]*goFixInlineConstFact)
+	var (
+		inlinableFuncs   = make(map[*types.Func]*inline.Callee) // memoization of fact import (nil => no fact)
+		inlinableConsts  = make(map[*types.Const]*goFixInlineConstFact)
+		inlinableAliases = make(map[*types.TypeName]*goFixInlineAliasFact)
+	)
 
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 	nodeFilter := []ast.Node{(*ast.FuncDecl)(nil), (*ast.GenDecl)(nil)}
@@ -89,52 +92,96 @@ func run(pass *analysis.Pass) (any, error) {
 			inlinableFuncs[fn] = callee
 
 		case *ast.GenDecl:
-			if decl.Tok != token.CONST {
+			if decl.Tok != token.CONST && decl.Tok != token.TYPE {
 				return
 			}
 			declInline := hasFixInline(decl.Doc)
 			// Accept inline directives on the entire decl as well as individual specs.
 			for _, spec := range decl.Specs {
-				spec := spec.(*ast.ValueSpec) // guaranteed by Tok == CONST
-				specInline := hasFixInline(spec.Doc)
-				if declInline || specInline {
-					for i, name := range spec.Names {
-						if i >= len(spec.Values) {
-							// Possible following an iota.
-							break
-						}
-						val := spec.Values[i]
-						var rhsID *ast.Ident
-						switch e := val.(type) {
-						case *ast.Ident:
-							// Constants defined with the predeclared iota cannot be inlined.
-							if pass.TypesInfo.Uses[e] == builtinIota {
-								pass.Reportf(val.Pos(), "invalid //go:fix inline directive: const value is iota")
+				switch spec := spec.(type) {
+				case *ast.TypeSpec: // Tok == TYPE
+					if !declInline && !hasFixInline(spec.Doc) {
+						continue
+					}
+					if !spec.Assign.IsValid() {
+						pass.Reportf(spec.Pos(), "invalid //go:fix inline directive: not a type alias")
+						continue
+					}
+					if spec.TypeParams != nil {
+						// TODO(jba): handle generic aliases
+						continue
+					}
+					// The alias must refer to another named type.
+					// TODO(jba): generalize to more type expressions.
+					var rhsID *ast.Ident
+					switch e := ast.Unparen(spec.Type).(type) {
+					case *ast.Ident:
+						rhsID = e
+					case *ast.SelectorExpr:
+						rhsID = e.Sel
+					default:
+						continue
+					}
+					lhs := pass.TypesInfo.Defs[spec.Name].(*types.TypeName)
+					// more (jba): test one alias pointing to another alias
+					rhs := pass.TypesInfo.Uses[rhsID].(*types.TypeName)
+					typ := &goFixInlineAliasFact{
+						RHSName:    rhs.Name(),
+						RHSPkgName: rhs.Pkg().Name(),
+						RHSPkgPath: rhs.Pkg().Path(),
+					}
+					if rhs.Pkg() == pass.Pkg {
+						typ.rhsObj = rhs
+					}
+					inlinableAliases[lhs] = typ
+					// Create a fact only if the LHS is exported and defined at top level.
+					// We create a fact even if the RHS is non-exported,
+					// so we can warn about uses in other packages.
+					if lhs.Exported() && typesinternal.IsPackageLevel(lhs) {
+						pass.ExportObjectFact(lhs, typ)
+					}
+
+				case *ast.ValueSpec: // Tok == CONST
+					specInline := hasFixInline(spec.Doc)
+					if declInline || specInline {
+						for i, name := range spec.Names {
+							if i >= len(spec.Values) {
+								// Possible following an iota.
+								break
+							}
+							val := spec.Values[i]
+							var rhsID *ast.Ident
+							switch e := val.(type) {
+							case *ast.Ident:
+								// Constants defined with the predeclared iota cannot be inlined.
+								if pass.TypesInfo.Uses[e] == builtinIota {
+									pass.Reportf(val.Pos(), "invalid //go:fix inline directive: const value is iota")
+									continue
+								}
+								rhsID = e
+							case *ast.SelectorExpr:
+								rhsID = e.Sel
+							default:
+								pass.Reportf(val.Pos(), "invalid //go:fix inline directive: const value is not the name of another constant")
 								continue
 							}
-							rhsID = e
-						case *ast.SelectorExpr:
-							rhsID = e.Sel
-						default:
-							pass.Reportf(val.Pos(), "invalid //go:fix inline directive: const value is not the name of another constant")
-							continue
-						}
-						lhs := pass.TypesInfo.Defs[name].(*types.Const)
-						rhs := pass.TypesInfo.Uses[rhsID].(*types.Const) // must be so in a well-typed program
-						con := &goFixInlineConstFact{
-							RHSName:    rhs.Name(),
-							RHSPkgName: rhs.Pkg().Name(),
-							RHSPkgPath: rhs.Pkg().Path(),
-						}
-						if rhs.Pkg() == pass.Pkg {
-							con.rhsObj = rhs
-						}
-						inlinableConsts[lhs] = con
-						// Create a fact only if the LHS is exported and defined at top level.
-						// We create a fact even if the RHS is non-exported,
-						// so we can warn uses in other packages.
-						if lhs.Exported() && typesinternal.IsPackageLevel(lhs) {
-							pass.ExportObjectFact(lhs, con)
+							lhs := pass.TypesInfo.Defs[name].(*types.Const)
+							rhs := pass.TypesInfo.Uses[rhsID].(*types.Const) // must be so in a well-typed program
+							con := &goFixInlineConstFact{
+								RHSName:    rhs.Name(),
+								RHSPkgName: rhs.Pkg().Name(),
+								RHSPkgPath: rhs.Pkg().Path(),
+							}
+							if rhs.Pkg() == pass.Pkg {
+								con.rhsObj = rhs
+							}
+							inlinableConsts[lhs] = con
+							// Create a fact only if the LHS is exported and defined at top level.
+							// We create a fact even if the RHS is non-exported,
+							// so we can warn about uses in other packages.
+							if lhs.Exported() && typesinternal.IsPackageLevel(lhs) {
+								pass.ExportObjectFact(lhs, con)
+							}
 						}
 					}
 				}
@@ -143,7 +190,7 @@ func run(pass *analysis.Pass) (any, error) {
 	})
 
 	// Pass 2. Inline each static call to an inlinable function
-	// and each reference to an inlinable constant.
+	// and each reference to an inlinable constant or type alias.
 	//
 	// TODO(adonovan):  handle multiple diffs that each add the same import.
 	for cur := range cursor.Root(inspect).Preorder((*ast.CallExpr)(nil), (*ast.Ident)(nil)) {
@@ -218,6 +265,65 @@ func run(pass *analysis.Pass) (any, error) {
 			}
 
 		case *ast.Ident:
+			// If the identifier is a use of an inlinable type alias, suggest inlining it.
+			// TODO(jba): much of this code is shared with the constant case, below.
+			// Try to factor more of it out, unless it will change anyway when we move beyond simple RHS's.
+			if ali, ok := pass.TypesInfo.Uses[n].(*types.TypeName); ok {
+				inalias, ok := inlinableAliases[ali]
+				if !ok {
+					var fact goFixInlineAliasFact
+					if pass.ImportObjectFact(ali, &fact) {
+						inalias = &fact
+						inlinableAliases[ali] = inalias
+					}
+				}
+				if inalias == nil {
+					continue // nope
+				}
+				curFile := currentFile(cur)
+
+				// We have an identifier A here (n), possibly qualified by a package identifier (sel.X,
+				// where sel is the parent of X), // and an inlinable "type A = B" elsewhere (inali).
+				// Consider replacing A with B.
+
+				// Check that the expression we are inlining (B) means the same thing
+				// (refers to the same object) in n's scope as it does in A's scope.
+				// If the RHS is not in the current package, AddImport will handle
+				// shadowing, so we only need to worry about when both expressions
+				// are in the current package.
+				if pass.Pkg.Path() == inalias.RHSPkgPath {
+					// fcon.rhsObj is the object referred to by B in the definition of A.
+					scope := pass.TypesInfo.Scopes[curFile].Innermost(n.Pos()) // n's scope
+					_, obj := scope.LookupParent(inalias.RHSName, n.Pos())     // what "B" means in n's scope
+					if obj == nil {
+						// Should be impossible: if code at n can refer to the LHS,
+						// it can refer to the RHS.
+						panic(fmt.Sprintf("no object for inlinable alias %s RHS %s", n.Name, inalias.RHSName))
+					}
+					if obj != inalias.rhsObj {
+						// "B" means something different here than at the inlinable const's scope.
+						continue
+					}
+				} else if !analysisinternal.CanImport(pass.Pkg.Path(), inalias.RHSPkgPath) {
+					// If this package can't see the RHS's package, we can't inline.
+					continue
+				}
+				var (
+					importPrefix string
+					edits        []analysis.TextEdit
+				)
+				if inalias.RHSPkgPath != pass.Pkg.Path() {
+					_, importPrefix, edits = analysisinternal.AddImport(
+						pass.TypesInfo, curFile, inalias.RHSPkgName, inalias.RHSPkgPath, inalias.RHSName, n.Pos())
+				}
+				// If n is qualified by a package identifier, we'll need the full selector expression.
+				var expr ast.Expr = n
+				if e, _ := cur.Edge(); e == edge.SelectorExpr_Sel {
+					expr = cur.Parent().Node().(ast.Expr)
+				}
+				reportInline(pass, "type alias", "Type alias", expr, edits, importPrefix+inalias.RHSName)
+				continue
+			}
 			// If the identifier is a use of an inlinable constant, suggest inlining it.
 			if con, ok := pass.TypesInfo.Uses[n].(*types.Const); ok {
 				incon, ok := inlinableConsts[con]
@@ -233,14 +339,10 @@ func run(pass *analysis.Pass) (any, error) {
 				}
 
 				// If n is qualified by a package identifier, we'll need the full selector expression.
-				var sel *ast.SelectorExpr
-				if e, _ := cur.Edge(); e == edge.SelectorExpr_Sel {
-					sel = cur.Parent().Node().(*ast.SelectorExpr)
-				}
 				curFile := currentFile(cur)
 
-				// We have an identifier A here (n), possibly qualified by a package identifier (sel.X),
-				// and an inlinable "const A = B" elsewhere (fcon).
+				// We have an identifier A here (n), possibly qualified by a package identifier (sel.X,
+				// where sel is the parent of n), // and an inlinable "const A = B" elsewhere (incon).
 				// Consider replacing A with B.
 
 				// Check that the expression we are inlining (B) means the same thing
@@ -249,7 +351,7 @@ func run(pass *analysis.Pass) (any, error) {
 				// shadowing, so we only need to worry about when both expressions
 				// are in the current package.
 				if pass.Pkg.Path() == incon.RHSPkgPath {
-					// fcon.rhsObj is the object referred to by B in the definition of A.
+					// incon.rhsObj is the object referred to by B in the definition of A.
 					scope := pass.TypesInfo.Scopes[curFile].Innermost(n.Pos()) // n's scope
 					_, obj := scope.LookupParent(incon.RHSName, n.Pos())       // what "B" means in n's scope
 					if obj == nil {
@@ -273,38 +375,38 @@ func run(pass *analysis.Pass) (any, error) {
 					_, importPrefix, edits = analysisinternal.AddImport(
 						pass.TypesInfo, curFile, incon.RHSPkgName, incon.RHSPkgPath, incon.RHSName, n.Pos())
 				}
-				var (
-					pos  = n.Pos()
-					end  = n.End()
-					name = n.Name
-				)
-				// Replace the entire SelectorExpr if there is one.
-				if sel != nil {
-					pos = sel.Pos()
-					end = sel.End()
-					name = sel.X.(*ast.Ident).Name + "." + n.Name
+				// If n is qualified by a package identifier, we'll need the full selector expression.
+				var expr ast.Expr = n
+				if e, _ := cur.Edge(); e == edge.SelectorExpr_Sel {
+					expr = cur.Parent().Node().(ast.Expr)
 				}
-				edits = append(edits, analysis.TextEdit{
-					Pos:     pos,
-					End:     end,
-					NewText: []byte(importPrefix + incon.RHSName),
-				})
-				pass.Report(analysis.Diagnostic{
-					Pos:     pos,
-					End:     end,
-					Message: fmt.Sprintf("Constant %s should be inlined", name),
-					SuggestedFixes: []analysis.SuggestedFix{{
-						Message:   fmt.Sprintf("Inline constant %s", name),
-						TextEdits: edits,
-					}},
-				})
+				reportInline(pass, "constant", "Constant", expr, edits, importPrefix+incon.RHSName)
 			}
 		}
 	}
 
 	return nil, nil
 }
 
+// reportInline reports a diagnostic for fixing an inlinable name.
+func reportInline(pass *analysis.Pass, kind, capKind string, ident ast.Expr, edits []analysis.TextEdit, newText string) {
+	edits = append(edits, analysis.TextEdit{
+		Pos:     ident.Pos(),
+		End:     ident.End(),
+		NewText: []byte(newText),
+	})
+	name := analysisinternal.Format(pass.Fset, ident)
+	pass.Report(analysis.Diagnostic{
+		Pos:     ident.Pos(),
+		End:     ident.End(),
+		Message: fmt.Sprintf("%s %s should be inlined", capKind, name),
+		SuggestedFixes: []analysis.SuggestedFix{{
+			Message:   fmt.Sprintf("Inline %s %s", kind, name),
+			TextEdits: edits,
+		}},
+	})
+}
+
 // hasFixInline reports the presence of a "//go:fix inline" directive
 // in the comments.
 func hasFixInline(cg *ast.CommentGroup) bool {
@@ -339,6 +441,22 @@ func (c *goFixInlineConstFact) String() string {
 
 func (*goFixInlineConstFact) AFact() {}
 
+// A goFixInlineAliasFact is exported for each type alias marked "//go:fix inline".
+// It holds information about an inlinable type alias. Gob-serializable.
+type goFixInlineAliasFact struct {
+	// Information about "type LHSName = RHSName".
+	RHSName    string
+	RHSPkgPath string
+	RHSPkgName string
+	rhsObj     types.Object // for current package
+}
+
+func (c *goFixInlineAliasFact) String() string {
+	return fmt.Sprintf("goFixInline alias %q.%s", c.RHSPkgPath, c.RHSName)
+}
+
+func (*goFixInlineAliasFact) AFact() {}
+
 func discard(string, ...any) {}
 
 var builtinIota = types.Universe.Lookup("iota")
diff --git a/gopls/internal/analysis/gofix/testdata/src/a/a.go b/gopls/internal/analysis/gofix/testdata/src/a/a.go
@@ -101,3 +101,18 @@ func shadow() {
 
 	_ = x
 }
+
+// Type aliases
+
+//go:fix inline
+type A = T // want A: `goFixInline alias "a".T`
+
+var _ A // want `Type alias A should be inlined`
+
+type B = []T // nope: only named RHSs
+
+//go:fix inline
+type AA = // want AA: `goFixInline alias "a".A`
+A         // want `Type alias A should be inlined`
+
+var _ AA // want `Type alias AA should be inlined`
diff --git a/gopls/internal/analysis/gofix/testdata/src/a/a.go.golden b/gopls/internal/analysis/gofix/testdata/src/a/a.go.golden
@@ -101,3 +101,18 @@ func shadow() {
 
 	_ = x
 }
+
+// Type aliases
+
+//go:fix inline
+type A = T // want A: `goFixInline alias "a".T`
+
+var _ T // want `Type alias A should be inlined`
+
+type B = []T // nope: only named RHSs
+
+//go:fix inline
+type AA = // want AA: `goFixInline alias "a".A`
+T         // want `Type alias A should be inlined`
+
+var _ A // want `Type alias AA should be inlined`
diff --git a/gopls/internal/analysis/gofix/testdata/src/b/b.go b/gopls/internal/analysis/gofix/testdata/src/b/b.go
@@ -30,3 +30,5 @@ func g() {
 }
 
 const d = a.D // nope: a.D refers to a constant in a package that is not visible here.
+
+var _ a.A // want `Type alias a\.A should be inlined`
diff --git a/gopls/internal/analysis/gofix/testdata/src/b/b.go.golden b/gopls/internal/analysis/gofix/testdata/src/b/b.go.golden
@@ -34,3 +34,5 @@ func g() {
 }
 
 const d = a.D // nope: a.D refers to a constant in a package that is not visible here.
+
+var _ a.T // want `Type alias a\.A should be inlined`

Original file line number	Diff line number	Diff line change
`@@ -30,3 +30,5 @@ func g() {`
`30`	`30`	`}`
`31`	`31`
`32`	`32`	`const d = a.D // nope: a.D refers to a constant in a package that is not visible here.`
	`33`	`+`
	`34`	+var _ a.A // want `Type alias a\.A should be inlined`
Original file line number	Diff line number	Diff line change
`@@ -34,3 +34,5 @@ func g() {`
`34`	`34`	`}`
`35`	`35`
`36`	`36`	`const d = a.D // nope: a.D refers to a constant in a package that is not visible here.`
	`37`	`+`
	`38`	+var _ a.T // want `Type alias a\.A should be inlined`