Skip to content

Commit

Permalink
internal/analysis/gofix: change "forward" back to "inline"
Browse files Browse the repository at this point in the history
For golang/go#32816.

Change-Id: I02605efe2ca4db4fbef68ae26a57cb793ad5bf56
Reviewed-on: https://go-review.googlesource.com/c/tools/+/647736
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
  • Loading branch information
jba committed Feb 7, 2025
1 parent 82317ce commit 9c087d9
Show file tree
Hide file tree
Showing 9 changed files with 112 additions and 146 deletions.
3 changes: 1 addition & 2 deletions gopls/doc/analyzers.md
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,7 @@ Package documentation: [framepointer](https://pkg.go.dev/golang.org/x/tools/go/a
## `gofix`: apply fixes based on go:fix comment directives


The gofix analyzer inlines functions that are marked for inlining
and forwards constants that are marked for forwarding.
The gofix analyzer inlines functions and constants that are marked for inlining.

Default: on.

Expand Down
9 changes: 4 additions & 5 deletions gopls/doc/release/v0.18.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,8 @@ instead.

## New `gofix` analyzer

Gopls now reports when a function call should be inlined or a use of a constant
should be forwarded.
These diagnostics and the associated code actions are triggered by "//go:fix"
Gopls now reports when a function call or a use of a constant should be inlined.
These diagnostics and the associated code actions are triggered by "//go:fix inline"
directives at the function and constant definitions.
(See [the go:fix proposal](https://go.dev/issue/32816).)

Expand All @@ -90,10 +89,10 @@ func Square(x int) int { return Pow(x, 2) }
If gopls sees a call to `intmath.Square` in your code, it will suggest inlining
it, and will offer a code action to do so.

The same feature works for constants, only the directive is "//go:fix forward".
The same feature works for constants.
With a constant definition like this:
```
//go:fix forward
//go:fix inline
const Ptr = Pointer
```
gopls will suggest replacing `Ptr` in your code with `Pointer`.
Expand Down
22 changes: 10 additions & 12 deletions gopls/internal/analysis/gofix/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,14 @@

/*
Package gofix defines an Analyzer that inlines calls to functions
marked with a "//go:fix inline" doc comment,
and forwards uses of constants
marked with a "//go:fix forward" doc comment.
and uses of constants
marked with a "//go:fix inline" doc comment.
# Analyzer gofix
gofix: apply fixes based on go:fix comment directives
The gofix analyzer inlines functions that are marked for inlining
and forwards constants that are marked for forwarding.
The gofix analyzer inlines functions and constants that are marked for inlining.
# Functions
Expand Down Expand Up @@ -48,31 +46,31 @@ to enable automatic migration.
# Constants
Given a constant that is marked for forwarding, like this one:
Given a constant that is marked for inlining, like this one:
//go:fix forward
//go:fix inline
const Ptr = Pointer
this analyzer will recommend that uses of Ptr should be replaced with Pointer.
As with inlining, forwarding can be used to replace deprecated constants and
As with functions, inlining can be used to replace deprecated constants and
constants in obsolete packages.
A constant definition can be marked for forwarding only if it refers to another
A constant definition can be marked for inlining only if it refers to another
named constant.
The "//go:fix forward" comment must appear before a single const declaration on its own,
The "//go:fix inline" comment must appear before a single const declaration on its own,
as above; before a const declaration that is part of a group, as in this case:
const (
C = 1
//go:fix forward
//go:fix inline
Ptr = Pointer
)
or before a group, applying to every constant in the group:
//go:fix forward
//go:fix inline
const (
Ptr = Pointer
Val = Value
Expand Down
88 changes: 35 additions & 53 deletions gopls/internal/analysis/gofix/gofix.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ var Analyzer = &analysis.Analyzer{
Doc: analysisinternal.MustExtractDoc(doc, "gofix"),
URL: "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/gofix",
Run: run,
FactTypes: []analysis.Fact{new(goFixInlineFuncFact), new(goFixForwardConstFact)},
FactTypes: []analysis.Fact{new(goFixInlineFuncFact), new(goFixInlineConstFact)},
Requires: []*analysis.Analyzer{inspect.Analyzer},
}

Expand Down Expand Up @@ -64,19 +64,14 @@ func run(pass *analysis.Pass) (any, error) {
// 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)
forwardableConsts := make(map[*types.Const]*goFixForwardConstFact)
inlinableConsts := make(map[*types.Const]*goFixInlineConstFact)

inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
nodeFilter := []ast.Node{(*ast.FuncDecl)(nil), (*ast.GenDecl)(nil)}
inspect.Preorder(nodeFilter, func(n ast.Node) {
switch decl := n.(type) {
case *ast.FuncDecl:
hasInline, hasForward := fixDirectives(decl.Doc)
if hasForward {
pass.Reportf(decl.Doc.Pos(), "use //go:fix inline for functions")
return
}
if !hasInline {
if !hasFixInline(decl.Doc) {
return
}
content, err := readFile(decl)
Expand All @@ -97,20 +92,12 @@ func run(pass *analysis.Pass) (any, error) {
if decl.Tok != token.CONST {
return
}
declInline, declForward := fixDirectives(decl.Doc)
if declInline {
pass.Reportf(decl.Doc.Pos(), "use //go:fix forward for constants")
return
}
// Accept forward directives on the entire decl as well as individual specs.
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, specForward := fixDirectives(spec.Doc)
if specInline {
pass.Reportf(spec.Doc.Pos(), "use //go:fix forward for constants")
return
}
if declForward || specForward {
specInline := hasFixInline(spec.Doc)
if declInline || specInline {
for i, name := range spec.Names {
if i >= len(spec.Values) {
// Possible following an iota.
Expand All @@ -120,29 +107,29 @@ func run(pass *analysis.Pass) (any, error) {
var rhsID *ast.Ident
switch e := val.(type) {
case *ast.Ident:
// Constants defined with the predeclared iota cannot be forwarded.
// Constants defined with the predeclared iota cannot be inlined.
if pass.TypesInfo.Uses[e] == builtinIota {
pass.Reportf(val.Pos(), "invalid //go:fix forward directive: const value is iota")
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 forward directive: const value is not the name of another constant")
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 := &goFixForwardConstFact{
con := &goFixInlineConstFact{
RHSName: rhs.Name(),
RHSPkgName: rhs.Pkg().Name(),
RHSPkgPath: rhs.Pkg().Path(),
}
if rhs.Pkg() == pass.Pkg {
con.rhsObj = rhs
}
forwardableConsts[lhs] = con
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.
Expand All @@ -155,8 +142,8 @@ func run(pass *analysis.Pass) (any, error) {
}
})

// Pass 2. Inline each static call to an inlinable function,
// and forward each reference to a forwardable constant.
// Pass 2. Inline each static call to an inlinable function
// and each reference to an inlinable constant.
//
// TODO(adonovan): handle multiple diffs that each add the same import.
for cur := range cursor.Root(inspect).Preorder((*ast.CallExpr)(nil), (*ast.Ident)(nil)) {
Expand Down Expand Up @@ -231,14 +218,14 @@ func run(pass *analysis.Pass) (any, error) {
}

case *ast.Ident:
// If the identifier is a use of a forwardable constant, suggest forwarding it.
// If the identifier is a use of an inlinable constant, suggest inlining it.
if con, ok := pass.TypesInfo.Uses[n].(*types.Const); ok {
fcon, ok := forwardableConsts[con]
fcon, ok := inlinableConsts[con]
if !ok {
var fact goFixForwardConstFact
var fact goFixInlineConstFact
if pass.ImportObjectFact(con, &fact) {
fcon = &fact
forwardableConsts[con] = fcon
inlinableConsts[con] = fcon
}
}
if fcon == nil {
Expand All @@ -253,7 +240,7 @@ func run(pass *analysis.Pass) (any, error) {
curFile := currentFile(cur)

// We have an identifier A here (n), possibly qualified by a package identifier (sel.X),
// and a forwardable "const A = B" elsewhere (fcon).
// and an inlinable "const A = B" elsewhere (fcon).
// Consider replacing A with B.

// Check that the expression we are inlining (B) means the same thing
Expand All @@ -268,10 +255,10 @@ func run(pass *analysis.Pass) (any, error) {
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 forwardable const %s RHS %s", n.Name, fcon.RHSName))
panic(fmt.Sprintf("no object for inlinable const %s RHS %s", n.Name, fcon.RHSName))
}
if obj != fcon.rhsObj {
// "B" means something different here than at the forwardable const's scope.
// "B" means something different here than at the inlinable const's scope.
continue
}
}
Expand Down Expand Up @@ -304,9 +291,9 @@ func run(pass *analysis.Pass) (any, error) {
pass.Report(analysis.Diagnostic{
Pos: pos,
End: end,
Message: fmt.Sprintf("Constant %s should be forwarded", name),
Message: fmt.Sprintf("Constant %s should be inlined", name),
SuggestedFixes: []analysis.SuggestedFix{{
Message: fmt.Sprintf("Forward constant %s", name),
Message: fmt.Sprintf("Inline constant %s", name),
TextEdits: edits,
}},
})
Expand All @@ -317,20 +304,15 @@ func run(pass *analysis.Pass) (any, error) {
return nil, nil
}

// fixDirectives reports the presence of "//go:fix inline" and "//go:fix forward"
// directives in the comments.
func fixDirectives(cg *ast.CommentGroup) (inline, forward bool) {
// hasFixInline reports the presence of a "//go:fix inline" directive
// in the comments.
func hasFixInline(cg *ast.CommentGroup) bool {
for _, d := range directives(cg) {
if d.Tool == "go" && d.Name == "fix" {
switch d.Args {
case "inline":
inline = true
case "forward":
forward = true
}
if d.Tool == "go" && d.Name == "fix" && d.Args == "inline" {
return true
}
}
return
return false
}

// A goFixInlineFuncFact is exported for each function marked "//go:fix inline".
Expand All @@ -340,21 +322,21 @@ type goFixInlineFuncFact struct{ Callee *inline.Callee }
func (f *goFixInlineFuncFact) String() string { return "goFixInline " + f.Callee.String() }
func (*goFixInlineFuncFact) AFact() {}

// A goFixForwardConstFact is exported for each constant marked "//go:fix forward".
// It holds information about a forwardable constant. Gob-serializable.
type goFixForwardConstFact struct {
// A goFixInlineConstFact is exported for each constant marked "//go:fix inline".
// It holds information about an inlinable constant. Gob-serializable.
type goFixInlineConstFact struct {
// Information about "const LHSName = RHSName".
RHSName string
RHSPkgPath string
RHSPkgName string
rhsObj types.Object // for current package
}

func (c *goFixForwardConstFact) String() string {
return fmt.Sprintf("goFixForward const %q.%s", c.RHSPkgPath, c.RHSName)
func (c *goFixInlineConstFact) String() string {
return fmt.Sprintf("goFixInline const %q.%s", c.RHSPkgPath, c.RHSName)
}

func (*goFixForwardConstFact) AFact() {}
func (*goFixInlineConstFact) AFact() {}

func discard(string, ...any) {}

Expand Down
Loading

0 comments on commit 9c087d9

Please sign in to comment.