From c9dffa385b8892e9fb0260e6ec1b83fc3d1a159b Mon Sep 17 00:00:00 2001
From: Marcel van Lohuizen <mpvl@gmail.com>
Date: Mon, 23 Dec 2024 17:17:46 +0100
Subject: [PATCH] internal/core/adt: propagate child errors

In V3, because closedness is computed out of
band with the processing of an arc, it may be
that an arc gets an error _after_ it has been
processed. For this reasons, we propagate it in
a separate loop after all other arcs have been
processed.

To avoid spurious errors, we now need to explicitly
check for an arc error before calling
reportFieldMismatch. Theoretically it is possible
that there is still a spurious error. This only occurs,
however, if the arc is already erroneous, so this is
not a huge deal.

Fixes #3576

Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com>
Change-Id: Idfec48db39be56155c3376b4284bd67a4e8bef20
Dispatch-Trailer: {"type":"trybot","CL":1206291,"patchset":1,"ref":"refs/changes/91/1206291/1","targetBranch":"master"}
---
 cue/testdata/disjunctions/errors.txtar | 100 ++++---------------------
 internal/core/adt/unify.go             |  24 +++---
 2 files changed, 27 insertions(+), 97 deletions(-)

diff --git a/cue/testdata/disjunctions/errors.txtar b/cue/testdata/disjunctions/errors.txtar
index a68450005..2757026c2 100644
--- a/cue/testdata/disjunctions/errors.txtar
+++ b/cue/testdata/disjunctions/errors.txtar
@@ -66,7 +66,7 @@ issue3576: full: {
 			optionsValue: options
 		}
 	}
-	
+
 	#Copy: {
 		copy!: string
 		options?: #Option | [#Option, ...]
@@ -78,7 +78,7 @@ issue3576: full: {
 	#Option: {}
 	
 	foo: #Run | #Copy
-	foo: run: "make"	
+	foo: run: "make"
 }
 -- out/eval/stats --
 Leaks:  0
@@ -180,15 +180,9 @@ Result:
       }
       #B: (#struct){
       }
-      foo: (struct){ |((#struct){
-          a: (_|_){
-            // [eval] issue3576.reduced.foo.a: field not allowed:
-            //     ./issue3576.cue:5:11
-            //     ./issue3576.cue:9:7
-          }
-        }, (#struct){
-          a: (string){ "1" }
-        }) }
+      foo: (#struct){
+        a: (string){ "1" }
+      }
     }
     full: (struct){
       #Run: (#struct){
@@ -209,26 +203,14 @@ Result:
       }
       #Option: (#struct){
       }
-      foo: (struct){ |((#struct){
-          run: (string){ "make" }
-          options?: ((list|struct)){ |((#struct){
-            }, (list){
-              0: (#struct){
-              }
-            }) }
-        }, (#struct){
-          run: (_|_){
-            // [eval] issue3576.full.foo.run: field not allowed:
-            //     ./issue3576.cue:24:18
-            //     ./issue3576.cue:31:7
-          }
-          copy!: (string){ string }
-          options?: ((list|struct)){ |((#struct){
-            }, (list){
-              0: (#struct){
-              }
-            }) }
-        }) }
+      foo: (#struct){
+        run: (string){ "make" }
+        options?: ((list|struct)){ |((#struct){
+          }, (list){
+            0: (#struct){
+            }
+          }) }
+      }
     }
   }
   issue3581: (struct){
@@ -329,61 +311,7 @@ diff old new
    }
    issue3576: (struct){
      reduced: (struct){
-@@ -99,9 +87,15 @@
-       }
-       #B: (#struct){
-       }
--      foo: (#struct){
--        a: (string){ "1" }
--      }
-+      foo: (struct){ |((#struct){
-+          a: (_|_){
-+            // [eval] issue3576.reduced.foo.a: field not allowed:
-+            //     ./issue3576.cue:5:11
-+            //     ./issue3576.cue:9:7
-+          }
-+        }, (#struct){
-+          a: (string){ "1" }
-+        }) }
-     }
-     full: (struct){
-       #Run: (#struct){
-@@ -122,14 +116,26 @@
-       }
-       #Option: (#struct){
-       }
--      foo: (#struct){
--        run: (string){ "make" }
--        options?: ((list|struct)){ |((#struct){
--          }, (list){
--            0: (#struct){
--            }
--          }) }
--      }
-+      foo: (struct){ |((#struct){
-+          run: (string){ "make" }
-+          options?: ((list|struct)){ |((#struct){
-+            }, (list){
-+              0: (#struct){
-+              }
-+            }) }
-+        }, (#struct){
-+          run: (_|_){
-+            // [eval] issue3576.full.foo.run: field not allowed:
-+            //     ./issue3576.cue:24:18
-+            //     ./issue3576.cue:31:7
-+          }
-+          copy!: (string){ string }
-+          options?: ((list|struct)){ |((#struct){
-+            }, (list){
-+              0: (#struct){
-+              }
-+            }) }
-+        }) }
-     }
-   }
-   issue3581: (struct){
-@@ -136,13 +142,11 @@
+@@ -136,13 +124,11 @@
      reduced: (struct){
        list: (_|_){
          // [incomplete] issue3581.reduced.list: 2 errors in empty disjunction:
diff --git a/internal/core/adt/unify.go b/internal/core/adt/unify.go
index 5b37b1027..c4ea78922 100644
--- a/internal/core/adt/unify.go
+++ b/internal/core/adt/unify.go
@@ -565,21 +565,11 @@ func (n *nodeContext) completeAllArcs(needs condition, mode runMode) bool {
 			continue
 		}
 
-		// Errors are allowed in let fields. Handle errors and failure to
-		// complete accordingly.
-		if !a.Label.IsLet() && a.ArcType <= ArcRequired {
-			a := a.DerefValue()
-			if err := a.Bottom(); err != nil {
-				n.AddChildError(err)
-			}
-			success = true // other arcs are irrelevant
-		}
-
 		// TODO: harmonize this error with "cannot combine"
 		switch {
 		case a.ArcType > ArcRequired, !a.Label.IsString():
 		case n.kind&StructKind == 0:
-			if !n.node.IsErr() {
+			if !n.node.IsErr() && !a.IsErr() {
 				n.reportFieldMismatch(pos(a.Value()), nil, a.Label, n.node.Value())
 			}
 			// case !wasVoid:
@@ -610,6 +600,18 @@ func (n *nodeContext) completeAllArcs(needs condition, mode runMode) bool {
 	}
 	n.node.Arcs = n.node.Arcs[:k]
 
+	for _, a := range n.node.Arcs {
+		// Errors are allowed in let fields. Handle errors and failure to
+		// complete accordingly.
+		if !a.Label.IsLet() && a.ArcType <= ArcRequired {
+			a := a.DerefValue()
+			if err := a.Bottom(); err != nil {
+				n.AddChildError(err)
+			}
+			success = true // other arcs are irrelevant
+		}
+	}
+
 	// TODO: perhaps this code can go once we have builtins for comparing to
 	// bottom.
 	for _, c := range n.postChecks {