internal/refactor/inline: ignore line directives
Line directives should be ignored (at both caller
and callee) as they would otherwise cause the
inliner to make a mess of the caller file.
This change ignores them consistently in the tests,
and adds a heuristic assertion to reject inputs
that seem to be affected by line directives.
Plus: a test.
Change-Id: I1a9bffd935fd7b288c47647304a2a6529779434b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/528298
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/internal/refactor/inline/callee.go b/internal/refactor/inline/callee.go
index 3e9f397..9fff849 100644
--- a/internal/refactor/inline/callee.go
+++ b/internal/refactor/inline/callee.go
@@ -70,6 +70,10 @@
// This design allows separate analysis of callers and callees in the
// golang.org/x/tools/go/analysis framework: the inlining information
// about a callee can be recorded as a "fact".
+//
+// The content should be the actual input to the compiler, not the
+// apparent source file according to any //line directives that
+// may be present within it.
func AnalyzeCallee(fset *token.FileSet, pkg *types.Package, info *types.Info, decl *ast.FuncDecl, content []byte) (*Callee, error) {
checkInfoFields(info)
@@ -322,11 +326,9 @@
// callee file content; all we need is "package _; func f() { ... }".
// This reduces the size of analysis facts.
//
- // (For ease of debugging we could insert a //line directive after
- // the package decl but it seems more trouble than it's worth.)
- //
// Offsets in the callee information are "relocatable"
// since they are all relative to the FuncDecl.
+
content = append([]byte("package _\n"),
content[offsetOf(fset, decl.Pos()):offsetOf(fset, decl.End())]...)
// Sanity check: re-parse the compacted content.
@@ -386,7 +388,7 @@
fnobj, ok := info.Defs[decl.Name]
if !ok {
panic(fmt.Sprintf("%s: no func object for %q",
- fset.Position(decl.Name.Pos()), decl.Name)) // ill-typed?
+ fset.PositionFor(decl.Name.Pos(), false), decl.Name)) // ill-typed?
}
paramInfos := make(map[*types.Var]*paramInfo)
diff --git a/internal/refactor/inline/inline.go b/internal/refactor/inline/inline.go
index 02ab43b..e2f1a2a 100644
--- a/internal/refactor/inline/inline.go
+++ b/internal/refactor/inline/inline.go
@@ -305,7 +305,7 @@
Info *types.Info
File *ast.File
Call *ast.CallExpr
- Content []byte
+ Content []byte // source of file containing
}
// Inline inlines the called function (callee) into the function call (caller)
@@ -325,7 +325,11 @@
}
logf("inline %s @ %v",
debugFormatNode(caller.Fset, caller.Call),
- caller.Fset.Position(caller.Call.Lparen))
+ caller.Fset.PositionFor(caller.Call.Lparen, false))
+
+ if !consistentOffsets(caller) {
+ return nil, fmt.Errorf("internal error: caller syntax positions are inconsistent with file content (did you forget to use FileSet.PositionFor when computing the file name?)")
+ }
res, err := inline(logf, caller, &callee.impl)
if err != nil {
@@ -611,7 +615,7 @@
if found.Pos().IsValid() {
return nil, fmt.Errorf("cannot inline because built-in %q is shadowed in caller by a %s (line %d)",
obj.Name, objectKind(found),
- caller.Fset.Position(found.Pos()).Line)
+ caller.Fset.PositionFor(found.Pos(), false).Line)
}
} else {
@@ -627,7 +631,7 @@
if !isPkgLevel(found) {
return nil, fmt.Errorf("cannot inline because %q is shadowed in caller by a %s (line %d)",
obj.Name, objectKind(found),
- caller.Fset.Position(found.Pos()).Line)
+ caller.Fset.PositionFor(found.Pos(), false).Line)
}
} else {
// Cross-package reference.
@@ -957,7 +961,7 @@
// function (if any) and is indeed referenced
// only by the call.
logf("keeping param %q: arg contains perhaps the last reference to possible caller local %v @ %v",
- param.info.Name, v, caller.Fset.Position(v.Pos()))
+ param.info.Name, v, caller.Fset.PositionFor(v.Pos(), false))
continue next
}
}
@@ -2055,3 +2059,22 @@
}
return true
}
+
+// consistentOffsets reports whether the portion of caller.Content
+// that corresponds to caller.Call can be parsed as a call expression.
+// If not, the client has provided inconsistent information, possibly
+// because they forgot to ignore line directives when computing the
+// filename enclosing the call.
+// This is just a heuristic.
+func consistentOffsets(caller *Caller) bool {
+ start := offsetOf(caller.Fset, caller.Call.Pos())
+ end := offsetOf(caller.Fset, caller.Call.End())
+ if !(0 < start && start < end && end <= len(caller.Content)) {
+ return false
+ }
+ expr, err := parser.ParseExpr(string(caller.Content[start:end]))
+ if err != nil {
+ return false
+ }
+ return is[*ast.CallExpr](expr)
+}
diff --git a/internal/refactor/inline/inline_test.go b/internal/refactor/inline/inline_test.go
index 430ee49..cb52f31 100644
--- a/internal/refactor/inline/inline_test.go
+++ b/internal/refactor/inline/inline_test.go
@@ -93,7 +93,7 @@
continue
}
for _, note := range notes {
- posn := pkg.Fset.Position(note.Pos)
+ posn := pkg.Fset.PositionFor(note.Pos, false)
if note.Name != "inline" {
t.Errorf("%s: invalid marker @%s", posn, note.Name)
continue
diff --git a/internal/refactor/inline/testdata/line-directives.txtar b/internal/refactor/inline/testdata/line-directives.txtar
new file mode 100644
index 0000000..66ae9ed
--- /dev/null
+++ b/internal/refactor/inline/testdata/line-directives.txtar
@@ -0,0 +1,35 @@
+Test of line directives in caller and caller.
+Neither should have any effect on inlining.
+
+-- go.mod --
+module testdata
+go 1.12
+
+-- a/a.go --
+package a
+
+import "testdata/b"
+
+func A() {
+//line b2.go:3:3
+ b.F() //@ inline(re"F", result)
+}
+
+-- b/b.go --
+package b
+
+//line b2.go:1:1
+func F() { println("hi") }
+
+-- b/b2.go --
+package b
+
+func NotWhatYouWereLookingFor() {}
+
+-- result --
+package a
+
+func A() {
+//line b2.go:3:3
+ println("hi") //@ inline(re"F", result)
+}
