internal/refactor/inline: fallible constant analysis

This change adds an analysis of "fallible constant"
(falcon) expressions, and uses it to reject substitution
of parameters by constant arguments in the (rare) cases
where it is not safe.

When substituting a parameter by its argument expression,
it is (perhaps surprisingly) not always safe to replace
a variable by a constant expression, even of exactly
the same type, as it may turn a dynamic error into
a compile-time one, as in this example:

   func f(s string) {
      if len(s) > 0 { println(s[0]) }
   }
   f("") // inline

The call is replaced by:

  if len("") > 0 {
     println(""[0]) // error: constant index out of range
  }

In general, operations on constants (in particular
-x, x+y, x[i], x[i:j], and f(x)) are subject to
additional static checks that don't make sense for
non-constant operands.

This analysis scans the callee function body for all
expressions that are not constant but would become
constant if the parameter vars were redeclared as
constants, and emits a constraint (a Go expression)
for each one that has the property that it will
not type-check (using types.CheckExpr) if the
particular argument values are unsuitable.
The substitution logic checks the constraints
and falls back to a binding decl if they are
not all satisfied. (More optimal solutions could
be found, but this situation is very uncommon.)

(It would be easy to detect these problems if we
simply type-checked the transformed source, but,
by design, we simply don't have the necessary
type information for indirect dependencies when
running in an environment like unitchecker.)

Fixes golang/go#62664

Change-Id: I27d40adb681c469e9c711bf1ee6f7319b5725a2a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/531695
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

internal/refactor/inline/callee.go

@@ -47,6 +47,7 @@ type gobCallee struct {
 	TotalReturns     int          // number of return statements
 	TrivialReturns   int          // number of return statements with trivial result conversions
 	Labels           []string     // names of all control labels
+	Falcon           falconResult // falcon constraint system
 }
 
 // A freeRef records a reference to a free object.  Gob-serializable.
@@ -331,7 +332,7 @@ func AnalyzeCallee(logf func(string, ...any), fset *token.FileSet, pkg *types.Pa
 		return nil, err
 	}
 
-	params, results, effects := analyzeParams(logf, fset, info, decl)
+	params, results, effects, falcon := analyzeParams(logf, fset, info, decl)
 	return &Callee{gobCallee{
 		Content:          content,
 		PkgPath:          pkg.Path(),
@@ -349,6 +350,7 @@ func AnalyzeCallee(logf func(string, ...any), fset *token.FileSet, pkg *types.Pa
 		TotalReturns:     totalReturns,
 		TrivialReturns:   trivialReturns,
 		Labels:           labels,
+		Falcon:           falcon,
 	}}, nil
 }
 
@@ -365,15 +367,15 @@ func parseCompact(content []byte) (*token.FileSet, *ast.FuncDecl, error) {
 }
 
 // A paramInfo records information about a callee receiver, parameter, or result variable.
-// TODO(adonovan): rename to sigVarInfo or paramOrResultInfo?
 type paramInfo struct {
-	Name     string          // parameter name (may be blank, or even "")
-	Index    int             // index within signature
-	IsResult bool            // false for receiver or parameter, true for result variable
-	Assigned bool            // parameter appears on left side of an assignment statement
-	Escapes  bool            // parameter has its address taken
-	Refs     []int           // FuncDecl-relative byte offset of parameter ref within body
-	Shadow   map[string]bool // names shadowed at one of the above refs
+	Name       string          // parameter name (may be blank, or even "")
+	Index      int             // index within signature
+	IsResult   bool            // false for receiver or parameter, true for result variable
+	Assigned   bool            // parameter appears on left side of an assignment statement
+	Escapes    bool            // parameter has its address taken
+	Refs       []int           // FuncDecl-relative byte offset of parameter ref within body
+	Shadow     map[string]bool // names shadowed at one of the above refs
+	FalconType string          // name of this parameter's type (if basic) in the falcon system
 }
 
 // analyzeParams computes information about parameters of function fn,
@@ -383,7 +385,7 @@ type paramInfo struct {
 // the other of the result variables of function fn.
 //
 // The input must be well-typed.
-func analyzeParams(logf func(string, ...any), fset *token.FileSet, info *types.Info, decl *ast.FuncDecl) (params, results []*paramInfo, effects []int) {
+func analyzeParams(logf func(string, ...any), fset *token.FileSet, info *types.Info, decl *ast.FuncDecl) (params, results []*paramInfo, effects []int, _ falconResult) {
 	fnobj, ok := info.Defs[decl.Name]
 	if !ok {
 		panic(fmt.Sprintf("%s: no func object for %q",
@@ -458,7 +460,9 @@ func analyzeParams(logf func(string, ...any), fset *token.FileSet, info *types.I
 	effects = calleefx(info, decl.Body, paramInfos)
 	logf("effects list = %v", effects)
 
-	return params, results, effects
+	falcon := falcon(logf, fset, paramInfos, info, decl)
+
+	return params, results, effects, falcon
 }
 
 // -- callee helpers --

internal/refactor/inline/doc.go

@@ -219,6 +219,18 @@ which they can be addressed.
     be prepared to eliminate the declaration too---this is where an
     iterative framework for simplification would really help).
 
+  - An expression such as s[i] may be valid if s and i are
+    variables but invalid if either or both of them are constants.
+    For example, a negative constant index s[-1] is always out of
+    bounds, and even a non-negative constant index may be out of
+    bounds depending on the particular string constant (e.g.
+    "abc"[4]).
+
+    So, if a parameter participates in any expression that is
+    subject to additional compile-time checks when its operands are
+    constant, it may be unsafe to substitute that parameter by a
+    constant argument value (#62664).
+
 More complex callee functions are inlinable with more elaborate and
 invasive changes to the statements surrounding the call expression.
 
@@ -253,6 +265,13 @@ TODO(adonovan): future work:
   - Eliminate parens and braces inserted conservatively when they
     are redundant.
 
+  - Eliminate explicit conversions of "untyped" literals inserted
+    conservatively when they are redundant. For example, the
+    conversion int32(1) is redundant when this value is used only as a
+    slice index; but it may be crucial if it is used in x := int32(1)
+    as it changes the type of x, which may have further implications.
+    The conversions may also be important to the falcon analysis.
+
   - Allow non-'go' build systems such as Bazel/Blaze a chance to
     decide whether an import is accessible using logic other than
     "/internal/" path segments. This could be achieved by returning