Merge pull request #15865 from owen-mc/go/extractor/no-intermediate-string-values

Go: extractor: do not store intermediate values in long string concatenations

go/extractor/extractor.go

@@ -798,7 +798,7 @@ func extractLocalScope(tw *trap.Writer, scope *types.Scope, parentScopeLabel tra
 func extractFileNode(tw *trap.Writer, nd *ast.File) {
 	lbl := tw.Labeler.FileLabel()
 
-	extractExpr(tw, nd.Name, lbl, 0)
+	extractExpr(tw, nd.Name, lbl, 0, false)
 
 	for i, decl := range nd.Decls {
 		extractDecl(tw, decl, lbl, i)
@@ -855,7 +855,7 @@ func emitScopeNodeInfo(tw *trap.Writer, nd ast.Node, lbl trap.Label) {
 }
 
 // extractExpr extracts AST information for the given expression and all its subexpressions
-func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
+func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int, skipExtractingValue bool) {
 	if expr == nil {
 		return
 	}
@@ -908,7 +908,7 @@ func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
 			return
 		}
 		kind = dbscheme.EllipsisExpr.Index()
-		extractExpr(tw, expr.Elt, lbl, 0)
+		extractExpr(tw, expr.Elt, lbl, 0, false)
 	case *ast.BasicLit:
 		if expr == nil {
 			return
@@ -940,28 +940,28 @@ func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
 			return
 		}
 		kind = dbscheme.FuncLitExpr.Index()
-		extractExpr(tw, expr.Type, lbl, 0)
+		extractExpr(tw, expr.Type, lbl, 0, false)
 		extractStmt(tw, expr.Body, lbl, 1)
 	case *ast.CompositeLit:
 		if expr == nil {
 			return
 		}
 		kind = dbscheme.CompositeLitExpr.Index()
-		extractExpr(tw, expr.Type, lbl, 0)
+		extractExpr(tw, expr.Type, lbl, 0, false)
 		extractExprs(tw, expr.Elts, lbl, 1, 1)
 	case *ast.ParenExpr:
 		if expr == nil {
 			return
 		}
 		kind = dbscheme.ParenExpr.Index()
-		extractExpr(tw, expr.X, lbl, 0)
+		extractExpr(tw, expr.X, lbl, 0, false)
 	case *ast.SelectorExpr:
 		if expr == nil {
 			return
 		}
 		kind = dbscheme.SelectorExpr.Index()
-		extractExpr(tw, expr.X, lbl, 0)
-		extractExpr(tw, expr.Sel, lbl, 1)
+		extractExpr(tw, expr.X, lbl, 0, false)
+		extractExpr(tw, expr.Sel, lbl, 1, false)
 	case *ast.IndexExpr:
 		if expr == nil {
 			return
@@ -982,8 +982,8 @@ func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
 				kind = dbscheme.IndexExpr.Index()
 			}
 		}
-		extractExpr(tw, expr.X, lbl, 0)
-		extractExpr(tw, expr.Index, lbl, 1)
+		extractExpr(tw, expr.X, lbl, 0, false)
+		extractExpr(tw, expr.Index, lbl, 1, false)
 	case *ast.IndexListExpr:
 		if expr == nil {
 			return
@@ -1001,33 +1001,33 @@ func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
 				kind = dbscheme.GenericTypeInstantiationExpr.Index()
 			}
 		}
-		extractExpr(tw, expr.X, lbl, 0)
+		extractExpr(tw, expr.X, lbl, 0, false)
 		extractExprs(tw, expr.Indices, lbl, 1, 1)
 	case *ast.SliceExpr:
 		if expr == nil {
 			return
 		}
 		kind = dbscheme.SliceExpr.Index()
-		extractExpr(tw, expr.X, lbl, 0)
-		extractExpr(tw, expr.Low, lbl, 1)
-		extractExpr(tw, expr.High, lbl, 2)
-		extractExpr(tw, expr.Max, lbl, 3)
+		extractExpr(tw, expr.X, lbl, 0, false)
+		extractExpr(tw, expr.Low, lbl, 1, false)
+		extractExpr(tw, expr.High, lbl, 2, false)
+		extractExpr(tw, expr.Max, lbl, 3, false)
 	case *ast.TypeAssertExpr:
 		if expr == nil {
 			return
 		}
 		kind = dbscheme.TypeAssertExpr.Index()
-		extractExpr(tw, expr.X, lbl, 0)
+		extractExpr(tw, expr.X, lbl, 0, false)
 		// expr.Type can be `nil` if this is the `x.(type)` in a type switch.
 		if expr.Type != nil {
-			extractExpr(tw, expr.Type, lbl, 1)
+			extractExpr(tw, expr.Type, lbl, 1, false)
 		}
 	case *ast.CallExpr:
 		if expr == nil {
 			return
 		}
 		kind = dbscheme.CallOrConversionExpr.Index()
-		extractExpr(tw, expr.Fun, lbl, 0)
+		extractExpr(tw, expr.Fun, lbl, 0, false)
 		extractExprs(tw, expr.Args, lbl, 1, 1)
 		if expr.Ellipsis.IsValid() {
 			dbscheme.HasEllipsisTable.Emit(tw, lbl)
@@ -1037,14 +1037,14 @@ func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
 			return
 		}
 		kind = dbscheme.StarExpr.Index()
-		extractExpr(tw, expr.X, lbl, 0)
+		extractExpr(tw, expr.X, lbl, 0, false)
 	case *ast.KeyValueExpr:
 		if expr == nil {
 			return
 		}
 		kind = dbscheme.KeyValueExpr.Index()
-		extractExpr(tw, expr.Key, lbl, 0)
-		extractExpr(tw, expr.Value, lbl, 1)
+		extractExpr(tw, expr.Key, lbl, 0, false)
+		extractExpr(tw, expr.Value, lbl, 1, false)
 	case *ast.UnaryExpr:
 		if expr == nil {
 			return
@@ -1058,7 +1058,7 @@ func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
 			}
 			kind = tp.Index()
 		}
-		extractExpr(tw, expr.X, lbl, 0)
+		extractExpr(tw, expr.X, lbl, 0, false)
 	case *ast.BinaryExpr:
 		if expr == nil {
 			return
@@ -1073,16 +1073,17 @@ func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
 				log.Fatalf("unsupported binary operator %s", expr.Op)
 			}
 			kind = tp.Index()
-			extractExpr(tw, expr.X, lbl, 0)
-			extractExpr(tw, expr.Y, lbl, 1)
+			skipLeft := skipExtractingValueForLeftOperand(tw, expr)
+			extractExpr(tw, expr.X, lbl, 0, skipLeft)
+			extractExpr(tw, expr.Y, lbl, 1, false)
 		}
 	case *ast.ArrayType:
 		if expr == nil {
 			return
 		}
 		kind = dbscheme.ArrayTypeExpr.Index()
-		extractExpr(tw, expr.Len, lbl, 0)
-		extractExpr(tw, expr.Elt, lbl, 1)
+		extractExpr(tw, expr.Len, lbl, 0, false)
+		extractExpr(tw, expr.Elt, lbl, 1, false)
 	case *ast.StructType:
 		if expr == nil {
 			return
@@ -1111,8 +1112,8 @@ func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
 			return
 		}
 		kind = dbscheme.MapTypeExpr.Index()
-		extractExpr(tw, expr.Key, lbl, 0)
-		extractExpr(tw, expr.Value, lbl, 1)
+		extractExpr(tw, expr.Key, lbl, 0, false)
+		extractExpr(tw, expr.Value, lbl, 1, false)
 	case *ast.ChanType:
 		if expr == nil {
 			return
@@ -1122,13 +1123,15 @@ func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
 			log.Fatalf("unsupported channel direction %v", expr.Dir)
 		}
 		kind = tp.Index()
-		extractExpr(tw, expr.Value, lbl, 0)
+		extractExpr(tw, expr.Value, lbl, 0, false)
 	default:
 		log.Fatalf("unknown expression of type %T", expr)
 	}
 	dbscheme.ExprsTable.Emit(tw, lbl, kind, parent, idx)
 	extractNodeLocation(tw, expr, lbl)
-	extractValueOf(tw, expr, lbl)
+	if !skipExtractingValue {
+		extractValueOf(tw, expr, lbl)
+	}
 }
 
 // extractExprs extracts AST information for a list of expressions, which are children of
@@ -1138,7 +1141,7 @@ func extractExpr(tw *trap.Writer, expr ast.Expr, parent trap.Label, idx int) {
 // -1 for decreasing indices)
 func extractExprs(tw *trap.Writer, exprs []ast.Expr, parent trap.Label, idx int, dir int) {
 	for _, expr := range exprs {
-		extractExpr(tw, expr, parent, idx)
+		extractExpr(tw, expr, parent, idx, false)
 		idx += dir
 	}
 }
@@ -1203,11 +1206,11 @@ func extractFields(tw *trap.Writer, fields *ast.FieldList, parent trap.Label, id
 		extractNodeLocation(tw, field, lbl)
 		if field.Names != nil {
 			for i, name := range field.Names {
-				extractExpr(tw, name, lbl, i+1)
+				extractExpr(tw, name, lbl, i+1, false)
 			}
 		}
-		extractExpr(tw, field.Type, lbl, 0)
-		extractExpr(tw, field.Tag, lbl, -1)
+		extractExpr(tw, field.Type, lbl, 0, false)
+		extractExpr(tw, field.Tag, lbl, -1, false)
 		extractDoc(tw, field.Doc, lbl)
 		idx += dir
 	}
@@ -1238,21 +1241,21 @@ func extractStmt(tw *trap.Writer, stmt ast.Stmt, parent trap.Label, idx int) {
 			return
 		}
 		kind = dbscheme.LabeledStmtType.Index()
-		extractExpr(tw, stmt.Label, lbl, 0)
+		extractExpr(tw, stmt.Label, lbl, 0, false)
 		extractStmt(tw, stmt.Stmt, lbl, 1)
 	case *ast.ExprStmt:
 		if stmt == nil {
 			return
 		}
 		kind = dbscheme.ExprStmtType.Index()
-		extractExpr(tw, stmt.X, lbl, 0)
+		extractExpr(tw, stmt.X, lbl, 0, false)
 	case *ast.SendStmt:
 		if stmt == nil {
 			return
 		}
 		kind = dbscheme.SendStmtType.Index()
-		extractExpr(tw, stmt.Chan, lbl, 0)
-		extractExpr(tw, stmt.Value, lbl, 1)
+		extractExpr(tw, stmt.Chan, lbl, 0, false)
+		extractExpr(tw, stmt.Value, lbl, 1, false)
 	case *ast.IncDecStmt:
 		if stmt == nil {
 			return
@@ -1264,7 +1267,7 @@ func extractStmt(tw *trap.Writer, stmt ast.Stmt, parent trap.Label, idx int) {
 		} else {
 			log.Fatalf("unsupported increment/decrement operator %v", stmt.Tok)
 		}
-		extractExpr(tw, stmt.X, lbl, 0)
+		extractExpr(tw, stmt.X, lbl, 0, false)
 	case *ast.AssignStmt:
 		if stmt == nil {
 			return
@@ -1281,13 +1284,13 @@ func extractStmt(tw *trap.Writer, stmt ast.Stmt, parent trap.Label, idx int) {
 			return
 		}
 		kind = dbscheme.GoStmtType.Index()
-		extractExpr(tw, stmt.Call, lbl, 0)
+		extractExpr(tw, stmt.Call, lbl, 0, false)
 	case *ast.DeferStmt:
 		if stmt == nil {
 			return
 		}
 		kind = dbscheme.DeferStmtType.Index()
-		extractExpr(tw, stmt.Call, lbl, 0)
+		extractExpr(tw, stmt.Call, lbl, 0, false)
 	case *ast.ReturnStmt:
 		kind = dbscheme.ReturnStmtType.Index()
 		extractExprs(tw, stmt.Results, lbl, 0, 1)
@@ -1307,7 +1310,7 @@ func extractStmt(tw *trap.Writer, stmt ast.Stmt, parent trap.Label, idx int) {
 		default:
 			log.Fatalf("unsupported branch statement type %v", stmt.Tok)
 		}
-		extractExpr(tw, stmt.Label, lbl, 0)
+		extractExpr(tw, stmt.Label, lbl, 0, false)
 	case *ast.BlockStmt:
 		if stmt == nil {
 			return
@@ -1321,7 +1324,7 @@ func extractStmt(tw *trap.Writer, stmt ast.Stmt, parent trap.Label, idx int) {
 		}
 		kind = dbscheme.IfStmtType.Index()
 		extractStmt(tw, stmt.Init, lbl, 0)
-		extractExpr(tw, stmt.Cond, lbl, 1)
+		extractExpr(tw, stmt.Cond, lbl, 1, false)
 		extractStmt(tw, stmt.Body, lbl, 2)
 		extractStmt(tw, stmt.Else, lbl, 3)
 		emitScopeNodeInfo(tw, stmt, lbl)
@@ -1339,7 +1342,7 @@ func extractStmt(tw *trap.Writer, stmt ast.Stmt, parent trap.Label, idx int) {
 		}
 		kind = dbscheme.ExprSwitchStmtType.Index()
 		extractStmt(tw, stmt.Init, lbl, 0)
-		extractExpr(tw, stmt.Tag, lbl, 1)
+		extractExpr(tw, stmt.Tag, lbl, 1, false)
 		extractStmt(tw, stmt.Body, lbl, 2)
 		emitScopeNodeInfo(tw, stmt, lbl)
 	case *ast.TypeSwitchStmt:
@@ -1368,7 +1371,7 @@ func extractStmt(tw *trap.Writer, stmt ast.Stmt, parent trap.Label, idx int) {
 		}
 		kind = dbscheme.ForStmtType.Index()
 		extractStmt(tw, stmt.Init, lbl, 0)
-		extractExpr(tw, stmt.Cond, lbl, 1)
+		extractExpr(tw, stmt.Cond, lbl, 1, false)
 		extractStmt(tw, stmt.Post, lbl, 2)
 		extractStmt(tw, stmt.Body, lbl, 3)
 		emitScopeNodeInfo(tw, stmt, lbl)
@@ -1377,9 +1380,9 @@ func extractStmt(tw *trap.Writer, stmt ast.Stmt, parent trap.Label, idx int) {
 			return
 		}
 		kind = dbscheme.RangeStmtType.Index()
-		extractExpr(tw, stmt.Key, lbl, 0)
-		extractExpr(tw, stmt.Value, lbl, 1)
-		extractExpr(tw, stmt.X, lbl, 2)
+		extractExpr(tw, stmt.Key, lbl, 0, false)
+		extractExpr(tw, stmt.Value, lbl, 1, false)
+		extractExpr(tw, stmt.X, lbl, 2, false)
 		extractStmt(tw, stmt.Body, lbl, 3)
 		emitScopeNodeInfo(tw, stmt, lbl)
 	default:
@@ -1434,8 +1437,8 @@ func extractDecl(tw *trap.Writer, decl ast.Decl, parent trap.Label, idx int) {
 		}
 		kind = dbscheme.FuncDeclType.Index()
 		extractFields(tw, decl.Recv, lbl, -1, -1)
-		extractExpr(tw, decl.Name, lbl, 0)
-		extractExpr(tw, decl.Type, lbl, 1)
+		extractExpr(tw, decl.Name, lbl, 0, false)
+		extractExpr(tw, decl.Type, lbl, 1, false)
 		extractStmt(tw, decl.Body, lbl, 2)
 		extractDoc(tw, decl.Doc, lbl)
 		extractTypeParamDecls(tw, decl.Type.TypeParams, lbl)
@@ -1461,8 +1464,8 @@ func extractSpec(tw *trap.Writer, spec ast.Spec, parent trap.Label, idx int) {
 			return
 		}
 		kind = dbscheme.ImportSpecType.Index()
-		extractExpr(tw, spec.Name, lbl, 0)
-		extractExpr(tw, spec.Path, lbl, 1)
+		extractExpr(tw, spec.Name, lbl, 0, false)
+		extractExpr(tw, spec.Path, lbl, 1, false)
 		extractDoc(tw, spec.Doc, lbl)
 	case *ast.ValueSpec:
 		if spec == nil {
@@ -1470,9 +1473,9 @@ func extractSpec(tw *trap.Writer, spec ast.Spec, parent trap.Label, idx int) {
 		}
 		kind = dbscheme.ValueSpecType.Index()
 		for i, name := range spec.Names {
-			extractExpr(tw, name, lbl, -(1 + i))
+			extractExpr(tw, name, lbl, -(1 + i), false)
 		}
-		extractExpr(tw, spec.Type, lbl, 0)
+		extractExpr(tw, spec.Type, lbl, 0, false)
 		extractExprs(tw, spec.Values, lbl, 1, 1)
 		extractDoc(tw, spec.Doc, lbl)
 	case *ast.TypeSpec:
@@ -1484,9 +1487,9 @@ func extractSpec(tw *trap.Writer, spec ast.Spec, parent trap.Label, idx int) {
 		} else {
 			kind = dbscheme.TypeDefSpecType.Index()
 		}
-		extractExpr(tw, spec.Name, lbl, 0)
+		extractExpr(tw, spec.Name, lbl, 0, false)
 		extractTypeParamDecls(tw, spec.TypeParams, lbl)
-		extractExpr(tw, spec.Type, lbl, 1)
+		extractExpr(tw, spec.Type, lbl, 1, false)
 		extractDoc(tw, spec.Doc, lbl)
 	}
 	dbscheme.SpecsTable.Emit(tw, lbl, kind, parent, idx)
@@ -1915,7 +1918,7 @@ func flattenBinaryExprTree(tw *trap.Writer, e ast.Expr, parent trap.Label, idx i
 		idx = flattenBinaryExprTree(tw, binaryexpr.X, parent, idx)
 		idx = flattenBinaryExprTree(tw, binaryexpr.Y, parent, idx)
 	} else {
-		extractExpr(tw, e, parent, idx)
+		extractExpr(tw, e, parent, idx, false)
 		idx = idx + 1
 	}
 	return idx
@@ -1937,10 +1940,10 @@ func extractTypeParamDecls(tw *trap.Writer, fields *ast.FieldList, parent trap.L
 		extractNodeLocation(tw, field, lbl)
 		if field.Names != nil {
 			for i, name := range field.Names {
-				extractExpr(tw, name, lbl, i+1)
+				extractExpr(tw, name, lbl, i+1, false)
 			}
 		}
-		extractExpr(tw, field.Type, lbl, 0)
+		extractExpr(tw, field.Type, lbl, 0, false)
 		extractDoc(tw, field.Doc, lbl)
 		idx += 1
 	}
@@ -2019,3 +2022,24 @@ func setTypeParamParent(tp *types.TypeParam, newobj types.Object) {
 		log.Fatalf("Parent of type parameter '%s %s' being set to a different value: '%s' vs '%s'", tp.String(), tp.Constraint().String(), obj, newobj)
 	}
 }
+
+// skipExtractingValueForLeftOperand returns true if the left operand of `be`
+// should not have its value extracted because it is an intermediate value in a
+// string concatenation - specifically that the right operand is a string
+// literal
+func skipExtractingValueForLeftOperand(tw *trap.Writer, be *ast.BinaryExpr) bool {
+	// check `be` has string type
+	tpVal := tw.Package.TypesInfo.Types[be]
+	if tpVal.Value == nil || tpVal.Value.Kind() != constant.String {
+		return false
+	}
+	// check that the right operand of `be` is a basic literal
+	if _, isBasicLit := be.Y.(*ast.BasicLit); !isBasicLit {
+		return false
+	}
+	// check that the left operand of `be` is not a basic literal
+	if _, isBasicLit := be.X.(*ast.BasicLit); isBasicLit {
+		return false
+	}
+	return true
+}

go/ql/test/extractor-tests/no-intermediate-strings/tst.expected

@@ -0,0 +1,11 @@
+| tst.go:4:6:4:8 | "a" | a |
+| tst.go:4:6:4:14 | ...+... | <no string value stored> |
+| tst.go:4:6:4:20 | ...+... | <no string value stored> |
+| tst.go:4:6:4:26 | ...+... | <no string value stored> |
+| tst.go:4:6:4:32 | ...+... | <no string value stored> |
+| tst.go:4:6:4:38 | ...+... | abcdef |
+| tst.go:4:12:4:14 | "b" | b |
+| tst.go:4:18:4:20 | "c" | c |
+| tst.go:4:24:4:26 | "d" | d |
+| tst.go:4:30:4:32 | "e" | e |
+| tst.go:4:36:4:38 | "f" | f |

go/ql/test/extractor-tests/no-intermediate-strings/tst.go

@@ -0,0 +1,5 @@
+package main
+
+func main() {
+	_ = "a" + "b" + "c" + "d" + "e" + "f"
+}

go/ql/test/extractor-tests/no-intermediate-strings/tst.ql

@@ -0,0 +1,17 @@
+import go
+
+string checkStringValue(Expr e) {
+  result = e.getStringValue()
+  or
+  not exists(e.getStringValue()) and result = "<no string value stored>"
+}
+
+from Expr e
+where e.getType() instanceof StringType
+// We should get string values for `"a"`, `"b"`, `"c"` and `"a" + "b" + "c"
+// but not `"a" + "b"`. In the extractor we avoid storing the value of
+// intermediate strings in string concatenations because in pathological cases
+// this could lead to a quadratic blowup in the size of string values stored,
+// which then causes performance problems when we iterate through all string
+// values.
+select e, checkStringValue(e)