internal/core/export: don't inline for errors

This makes it easier to see where errors originally came
from, as a value that is not inlined is marked with the
original path.

- tighten comprehension handling
- don't inline error values
- don't inline non-concrete values in expressions that
  require a concrete value.

Issue #867

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I5a9ff86949c0c743fad45280a06b272640c13c7f
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/541545
Reviewed-by: Paul Jolly <[email protected]>
Unity-Result: CUEcueckoo <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>

internal/core/export/adt.go

@@ -119,25 +119,20 @@ func (e *exporter) adt(env *adt.Environment, expr adt.Elem) ast.Expr {
 
 	case adt.Resolver:
 		return e.resolve(env, x)
-	}
-
-	e.inExpression++
-	defer func() { e.inExpression-- }()
 
-	switch x := expr.(type) {
 	case *adt.SliceExpr:
 		var lo, hi ast.Expr
 		if x.Lo != nil {
-			lo = e.expr(env, x.Lo)
+			lo = e.innerExpr(env, x.Lo)
 		}
 		if x.Hi != nil {
-			hi = e.expr(env, x.Hi)
+			hi = e.innerExpr(env, x.Hi)
 		}
 		// TODO: Stride not yet? implemented.
 		// if x.Stride != nil {
-		// 	stride = e.expr(env, x.Stride)
+		// 	stride = e.innerExpr(env, x.Stride)
 		// }
-		return &ast.SliceExpr{X: e.expr(env, x.X), Low: lo, High: hi}
+		return &ast.SliceExpr{X: e.innerExpr(env, x.X), Low: lo, High: hi}
 
 	case *adt.Interpolation:
 		var (
@@ -179,7 +174,7 @@ func (e *exporter) adt(env *adt.Environment, expr adt.Elem) ast.Expr {
 		suffix := `\(`
 		for i, elem := range x.Parts {
 			if i%2 == 1 {
-				t.Elts = append(t.Elts, e.expr(env, elem))
+				t.Elts = append(t.Elts, e.innerExpr(env, elem))
 			} else {
 				// b := strings.Builder{}
 				buf := []byte(prefix)
@@ -209,33 +204,33 @@ func (e *exporter) adt(env *adt.Environment, expr adt.Elem) ast.Expr {
 	case *adt.BoundExpr:
 		return &ast.UnaryExpr{
 			Op: x.Op.Token(),
-			X:  e.expr(env, x.Expr),
+			X:  e.innerExpr(env, x.Expr),
 		}
 
 	case *adt.UnaryExpr:
 		return &ast.UnaryExpr{
 			Op: x.Op.Token(),
-			X:  e.expr(env, x.X),
+			X:  e.innerExpr(env, x.X),
 		}
 
 	case *adt.BinaryExpr:
 		return &ast.BinaryExpr{
 			Op: x.Op.Token(),
-			X:  e.expr(env, x.X),
-			Y:  e.expr(env, x.Y),
+			X:  e.innerExpr(env, x.X),
+			Y:  e.innerExpr(env, x.Y),
 		}
 
 	case *adt.CallExpr:
 		a := []ast.Expr{}
 		for _, arg := range x.Args {
-			v := e.expr(env, arg)
+			v := e.innerExpr(env, arg)
 			if v == nil {
-				e.expr(env, arg)
+				e.innerExpr(env, arg)
 				panic("")
 			}
 			a = append(a, v)
 		}
-		fun := e.expr(env, x.Fun)
+		fun := e.innerExpr(env, x.Fun)
 		return &ast.CallExpr{Fun: fun, Args: a}
 
 	case *adt.DisjunctionExpr:
@@ -286,9 +281,6 @@ func (e *exporter) resolve(env *adt.Environment, r adt.Resolver) ast.Expr {
 		}
 	}
 
-	e.inExpression++
-	defer func() { e.inExpression-- }()
-
 	switch x := r.(type) {
 	case *adt.FieldReference:
 		ident, _ := e.newIdentForField(x.Src, x.Label, x.UpCount)
@@ -361,14 +353,14 @@ func (e *exporter) resolve(env *adt.Environment, r adt.Resolver) ast.Expr {
 
 	case *adt.SelectorExpr:
 		return &ast.SelectorExpr{
-			X:   e.expr(env, x.X),
+			X:   e.innerExpr(env, x.X),
 			Sel: e.stringLabel(x.Sel),
 		}
 
 	case *adt.IndexExpr:
 		return &ast.IndexExpr{
-			X:     e.expr(env, x.X),
-			Index: e.expr(env, x.Index),
+			X:     e.innerExpr(env, x.X),
+			Index: e.innerExpr(env, x.Index),
 		}
 	}
 	panic("unreachable")
@@ -443,7 +435,7 @@ func (e *exporter) decl(env *adt.Environment, d adt.Decl) ast.Decl {
 		// set bulk in frame.
 		frame := e.frame(0)
 
-		expr := e.expr(env, x.Filter)
+		expr := e.innerExpr(env, x.Filter)
 		frame.labelExpr = expr // see astutil.Resolve.
 
 		if x.Label != 0 {
@@ -487,7 +479,7 @@ func (e *exporter) decl(env *adt.Environment, d adt.Decl) ast.Decl {
 			fallthrough
 
 		default:
-			key := e.expr(env, srcKey)
+			key := e.innerExpr(env, srcKey)
 			switch key.(type) {
 			case *ast.Interpolation, *ast.BasicLit:
 			default:
@@ -563,10 +555,8 @@ loop:
 		case *adt.ForClause:
 			env := &adt.Environment{Up: env, Vertex: empty}
 			value := e.ident(x.Value)
-			clause := &ast.ForClause{
-				Value:  value,
-				Source: e.expr(env, x.Src),
-			}
+			src := e.innerExpr(env, x.Src)
+			clause := &ast.ForClause{Value: value, Source: src}
 			c.Clauses = append(c.Clauses, clause)
 
 			_, saved := e.pushFrame(empty, nil)
@@ -583,15 +573,17 @@ loop:
 			y = x.Dst
 
 		case *adt.IfClause:
-			clause := &ast.IfClause{Condition: e.expr(env, x.Condition)}
+			cond := e.innerExpr(env, x.Condition)
+			clause := &ast.IfClause{Condition: cond}
 			c.Clauses = append(c.Clauses, clause)
 			y = x.Dst
 
 		case *adt.LetClause:
 			env := &adt.Environment{Up: env, Vertex: empty}
+			expr := e.innerExpr(env, x.Expr)
 			clause := &ast.LetClause{
 				Ident: e.ident(x.Label),
-				Expr:  e.expr(env, x.Expr),
+				Expr:  expr,
 			}
 			c.Clauses = append(c.Clauses, clause)
 

internal/core/export/expr.go

@@ -55,6 +55,14 @@ func init() {
 	empty.UpdateStatus(adt.Finalized)
 }
 
+// innerExpr is like expr, but prohibits inlining in certain cases.
+func (e *exporter) innerExpr(env *adt.Environment, v adt.Elem) (result ast.Expr) {
+	e.inExpression++
+	r := e.expr(env, v)
+	e.inExpression--
+	return r
+}
+
 // expr converts an ADT expression to an AST expression.
 // The env is used for resolution and does not need to be given if v is known
 // to not contain any references.

internal/core/export/self.go

@@ -396,6 +396,10 @@ func (p *pivotter) refExpr(r adt.Resolver) ast.Expr {
 		// We need to wrap the value in a definition.
 	case dst.usageCount() == 0:
 		// The root value itself.
+	case n.IsErr():
+		// Don't simplify for errors to make the position of the error clearer.
+	case !n.IsConcrete() && p.x.inExpression > 0:
+		// Don't simplify an expression that is known will fail.
 	case dst.usageCount() == 1 && p.x.inExpression == 0:
 		// Used only once.
 		fallthrough

internal/core/export/testdata/selfcontained/errors.txtar

@@ -1,6 +1,5 @@
 #path: root
 
-TODO: do not inline errors
 -- in.cue --
 root: {
 	// comprehensions
@@ -28,14 +27,10 @@ n: {foo: {baz: int}}
 -- out/default --
 // comprehensions
 t1: {
-	for p in _ let x = _ if _ {}
-}
-t2: {
-	for p in _ {}
-}
-t3: {
-	if _ {}
+	for p in B let x = C if D {}
 }
+t2: E
+t3: F
 
 // incomplete
 t4: [1, 2][X]
@@ -44,6 +39,25 @@ t6: {
 	a: 1
 }[FOO.bar]
 
+//cue:path: b
+let B = _
+
+//cue:path: c
+let C = _
+
+//cue:path: d
+let D = _
+
+//cue:path: e
+let E = {
+	for p in _ {}
+}
+
+//cue:path: f
+let F = {
+	if _ {}
+}
+
 //cue:path: x
 let X = int