Skip to content

Commit f912a4f

Browse files
jbajba
committed
internal/refactor/inline/analyzer: inline consts into local scopes
Warn about inline directives on constants defined to be the predeclared iota. Allow inlining when iota is redefined to be an ordinary identifier. Do not inline if the inlined value refers to a different object. For golang/go#32816. Change-Id: I890b3c7dab595bb6d27ada459db5b91e2364ee13 Reviewed-on: https://go-review.googlesource.com/c/tools/+/646255 Reviewed-by: Alan Donovan <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent 0abda08 commit f912a4f

File tree

4 files changed

+162
-93
lines changed

4 files changed

+162
-93
lines changed

‎internal/refactor/inline/analyzer/analyzer.go

Lines changed: 104 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -53,86 +53,85 @@ func run(pass *analysis.Pass) (any, error) {
5353
// and export a fact for each one.
5454
inlinableFuncs := make(map[*types.Func]*inline.Callee) // memoization of fact import (nil => no fact)
5555
inlinableConsts := make(map[*types.Const]*goFixInlineConstFact)
56-
for _, file := range pass.Files {
57-
for _, decl := range file.Decls {
58-
switch decl := decl.(type) {
59-
case *ast.FuncDecl:
60-
if hasInlineDirective(decl.Doc) {
61-
content, err := readFile(decl)
62-
if err != nil {
63-
pass.Reportf(decl.Doc.Pos(), "invalid inlining candidate: cannot read source file: %v", err)
64-
continue
65-
}
66-
callee, err := inline.AnalyzeCallee(discard, pass.Fset, pass.Pkg, pass.TypesInfo, decl, content)
67-
if err != nil {
68-
pass.Reportf(decl.Doc.Pos(), "invalid inlining candidate: %v", err)
69-
continue
70-
}
71-
fn := pass.TypesInfo.Defs[decl.Name].(*types.Func)
72-
pass.ExportObjectFact(fn, &goFixInlineFuncFact{callee})
73-
inlinableFuncs[fn] = callee
74-
}
7556

76-
case *ast.GenDecl:
77-
if decl.Tok != token.CONST {
78-
continue
57+
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
58+
nodeFilter := []ast.Node{(*ast.FuncDecl)(nil), (*ast.GenDecl)(nil)}
59+
inspect.Preorder(nodeFilter, func(n ast.Node) {
60+
switch decl := n.(type) {
61+
case *ast.FuncDecl:
62+
if hasInlineDirective(decl.Doc) {
63+
content, err := readFile(decl)
64+
if err != nil {
65+
pass.Reportf(decl.Doc.Pos(), "invalid inlining candidate: cannot read source file: %v", err)
66+
return
7967
}
80-
// Accept inline directives on the entire decl as well as individual specs.
81-
declInline := hasInlineDirective(decl.Doc)
82-
for _, spec := range decl.Specs {
83-
spec := spec.(*ast.ValueSpec) // guaranteed by Tok == CONST
84-
if declInline || hasInlineDirective(spec.Doc) {
85-
for i, name := range spec.Names {
86-
if i >= len(spec.Values) {
87-
// Possible following an iota.
88-
break
89-
}
90-
val := spec.Values[i]
91-
var rhsID *ast.Ident
92-
switch e := val.(type) {
93-
case *ast.Ident:
94-
if e.Name == "iota" {
95-
continue
96-
}
97-
rhsID = e
98-
case *ast.SelectorExpr:
99-
rhsID = e.Sel
100-
default:
101-
pass.Reportf(val.Pos(), "invalid //go:fix inline directive: const value is not the name of another constant")
68+
callee, err := inline.AnalyzeCallee(discard, pass.Fset, pass.Pkg, pass.TypesInfo, decl, content)
69+
if err != nil {
70+
pass.Reportf(decl.Doc.Pos(), "invalid inlining candidate: %v", err)
71+
return
72+
}
73+
fn := pass.TypesInfo.Defs[decl.Name].(*types.Func)
74+
pass.ExportObjectFact(fn, &goFixInlineFuncFact{callee})
75+
inlinableFuncs[fn] = callee
76+
}
77+
78+
case *ast.GenDecl:
79+
if decl.Tok != token.CONST {
80+
return
81+
}
82+
// Accept inline directives on the entire decl as well as individual specs.
83+
declInline := hasInlineDirective(decl.Doc)
84+
for _, spec := range decl.Specs {
85+
spec := spec.(*ast.ValueSpec) // guaranteed by Tok == CONST
86+
if declInline || hasInlineDirective(spec.Doc) {
87+
for i, name := range spec.Names {
88+
if i >= len(spec.Values) {
89+
// Possible following an iota.
90+
break
91+
}
92+
val := spec.Values[i]
93+
var rhsID *ast.Ident
94+
switch e := val.(type) {
95+
case *ast.Ident:
96+
// Constants defined with the predeclared iota cannot be inlined.
97+
if pass.TypesInfo.Uses[e] == builtinIota {
98+
pass.Reportf(val.Pos(), "invalid //go:fix inline directive: const value is iota")
10299
continue
103100
}
104-
lhs := pass.TypesInfo.Defs[name].(*types.Const)
105-
rhs := pass.TypesInfo.Uses[rhsID].(*types.Const) // must be so in a well-typed program
106-
con := &goFixInlineConstFact{
107-
RHSName: rhs.Name(),
108-
RHSPkgPath: rhs.Pkg().Path(),
109-
}
110-
inlinableConsts[lhs] = con
111-
// Create a fact only if the LHS is exported and defined at top level.
112-
// We create a fact even if the RHS is non-exported,
113-
// so we can warn about uses in other packages.
114-
if lhs.Exported() && typesinternal.IsPackageLevel(lhs) {
115-
pass.ExportObjectFact(lhs, con)
116-
}
101+
rhsID = e
102+
case *ast.SelectorExpr:
103+
rhsID = e.Sel
104+
default:
105+
pass.Reportf(val.Pos(), "invalid //go:fix inline directive: const value is not the name of another constant")
106+
continue
107+
}
108+
lhs := pass.TypesInfo.Defs[name].(*types.Const)
109+
rhs := pass.TypesInfo.Uses[rhsID].(*types.Const) // must be so in a well-typed program
110+
con := &goFixInlineConstFact{
111+
RHSName: rhs.Name(),
112+
RHSPkgPath: rhs.Pkg().Path(),
113+
}
114+
if rhs.Pkg() == pass.Pkg {
115+
con.rhsObj = rhs
116+
}
117+
inlinableConsts[lhs] = con
118+
// Create a fact only if the LHS is exported and defined at top level.
119+
// We create a fact even if the RHS is non-exported,
120+
// so we can warn uses in other packages.
121+
if lhs.Exported() && typesinternal.IsPackageLevel(lhs) {
122+
pass.ExportObjectFact(lhs, con)
117123
}
118124
}
119125
}
120-
// TODO(jba): in user doc, warn that a comments within a spec, as in
121-
// const a,
122-
// //go:fix inline
123-
// b = 1, 2
124-
// will go unnoticed.
125-
// (They appear only in File.Comments, and it doesn't seem worthwhile to wade through those.)
126126
}
127127
}
128-
}
128+
})
129129

130130
// Pass 2. Inline each static call to an inlinable function,
131131
// and each reference to an inlinable constant.
132132
//
133133
// TODO(adonovan): handle multiple diffs that each add the same import.
134-
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
135-
nodeFilter := []ast.Node{
134+
nodeFilter = []ast.Node{
136135
(*ast.File)(nil),
137136
(*ast.CallExpr)(nil),
138137
(*ast.Ident)(nil),
@@ -218,7 +217,6 @@ func run(pass *analysis.Pass) (any, error) {
218217
if con, ok := pass.TypesInfo.Uses[n].(*types.Const); ok {
219218
incon, ok := inlinableConsts[con]
220219
if !ok {
221-
// TODO(jba): call ImportObjectFact.
222220
var fact goFixInlineConstFact
223221
if pass.ImportObjectFact(con, &fact) {
224222
incon = &fact
@@ -228,9 +226,29 @@ func run(pass *analysis.Pass) (any, error) {
228226
if incon == nil {
229227
return // nope
230228
}
229+
//
231230
// We have an identifier A here (n),
232231
// and an inlinable "const A = B" elsewhere (incon).
233-
// Suggest replacing A with B.
232+
// Consider replacing A with B.
233+
// Check that the expression we are inlining (B) means the same thing
234+
// (refers to the same object) in n's scope as it does in A's scope.
235+
if incon.rhsObj != nil {
236+
// Both expressions are in the current package.
237+
// incon.rhsObj is the object referred to by B in the definition of A.
238+
scope := pass.TypesInfo.Scopes[currentFile].Innermost(n.Pos()) // n's scope
239+
_, obj := scope.LookupParent(incon.RHSName, n.Pos()) // what "B" means in n's scope
240+
if obj == nil {
241+
// Should be impossible: if code at n can refer to the LHS,
242+
// it can refer to the RHS.
243+
panic(fmt.Sprintf("no object for inlinable const %s RHS %s", n.Name, incon.RHSName))
244+
}
245+
if obj != incon.rhsObj {
246+
// "B" means something different here than at the inlinable const's scope
247+
return
248+
}
249+
} else {
250+
// TODO(jba): handle the cross-package case by checking the package ID.
251+
}
234252
importPrefix := ""
235253
if incon.RHSPkgPath != con.Pkg().Path() {
236254
importID := maybeAddImportPath(currentFile, incon.RHSPkgPath)
@@ -266,8 +284,7 @@ func hasInlineDirective(cg *ast.CommentGroup) bool {
266284
}
267285

268286
func maybeAddImportPath(f *ast.File, path string) string {
269-
// TODO(jba): implement this in terms of existing functions.
270-
// TODO(adonovan): tell jba which functions.
287+
// TODO(jba): implement this in terms of analysisinternal.AddImport(info, file, pos, path, localname).
271288
return "unimp"
272289
}
273290

@@ -278,4 +295,21 @@ type goFixInlineFuncFact struct{ Callee *inline.Callee }
278295
func (f *goFixInlineFuncFact) String() string { return "goFixInline " + f.Callee.String() }
279296
func (*goFixInlineFuncFact) AFact() {}
280297

298+
// A goFixInlineConstFact is exported for each constant marked "//go:fix inline".
299+
// It holds information about an inlinable constant. Gob-serializable.
300+
type goFixInlineConstFact struct {
301+
// Information about "const LHSName = RHSName".
302+
RHSName string
303+
RHSPkgPath string
304+
rhsObj types.Object // for current package
305+
}
306+
307+
func (c *goFixInlineConstFact) String() string {
308+
return fmt.Sprintf("goFixInline const %q.%s", c.RHSPkgPath, c.RHSName)
309+
}
310+
311+
func (*goFixInlineConstFact) AFact() {}
312+
281313
func discard(string, ...any) {}
314+
315+
var builtinIota = types.Universe.Lookup("iota")

‎internal/refactor/inline/analyzer/const.go

Lines changed: 0 additions & 21 deletions
This file was deleted.

‎internal/refactor/inline/analyzer/testdata/src/a/a.go

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ const in5,
4949
//
5050
//go:fix inline
5151
const (
52-
a = iota
52+
a = iota // want `invalid //go:fix inline directive: const value is iota`
5353
b
5454
in7 = one
5555
)
@@ -68,3 +68,31 @@ func _() {
6868
in1 := 1 // don't inline lvalues
6969
_ = in1
7070
}
71+
72+
const (
73+
x = 1
74+
//go:fix inline
75+
in8 = x
76+
)
77+
78+
func shadow() {
79+
var x int // shadows x at package scope
80+
81+
//go:fix inline
82+
const a = iota // want `invalid //go:fix inline directive: const value is iota`
83+
84+
const iota = 2
85+
// Below this point, iota is an ordinary constant.
86+
87+
//go:fix inline
88+
const b = iota
89+
90+
x = a // a is defined with the predeclared iota, so it cannot be inlined
91+
x = b // want `Constant b should be inlined`
92+
93+
// Don't offer to inline in8, because the result, "x", would mean something different
94+
// in this scope than it does in the scope where in8 is defined.
95+
x = in8
96+
97+
_ = x
98+
}

‎internal/refactor/inline/analyzer/testdata/src/a/a.go.golden

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ const in5,
4949
//
5050
//go:fix inline
5151
const (
52-
a = iota
52+
a = iota // want `invalid //go:fix inline directive: const value is iota`
5353
b
5454
in7 = one
5555
)
@@ -68,3 +68,31 @@ func _() {
6868
in1 := 1 // don't inline lvalues
6969
_ = in1
7070
}
71+
72+
const (
73+
x = 1
74+
//go:fix inline
75+
in8 = x
76+
)
77+
78+
func shadow() {
79+
var x int // shadows x at package scope
80+
81+
//go:fix inline
82+
const a = iota // want `invalid //go:fix inline directive: const value is iota`
83+
84+
const iota = 2
85+
// Below this point, iota is an ordinary constant.
86+
87+
//go:fix inline
88+
const b = iota
89+
90+
x = a // a is defined with the predeclared iota, so it cannot be inlined
91+
x = iota // want `Constant b should be inlined`
92+
93+
// Don't offer to inline in8, because the result, "x", would mean something different
94+
// in this scope than it does in the scope where in8 is defined.
95+
x = in8
96+
97+
_ = x
98+
}

0 commit comments

Comments
 (0)