internal/core/subsume: fixes default handling

For a to subsume b, it must be true that the default
values of a must subsume b as well. This wa not
always correctly handled.

For backwards compatibility checks we want to be
somewhat stricter. It is not clear yet whether
this is the right package to do so. This CL adds
a placeholder comment, as well as a placeholder
test, to consider this.

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I99d1c84ba14722f2f969785b563c75fe154e3838
Dispatch-Trailer: {"type":"trybot","CL":1196979,"patchset":1,"ref":"refs/changes/79/1196979/1","targetBranch":"master"}

internal/core/subsume/value.go

@@ -172,13 +172,35 @@ func (s *subsumer) values(a, b adt.Value) (result bool) {
 
 	case *adt.Disjunction:
 
-		if s.LeftDefault {
-			a = adt.Default(a)
+		switch {
+		case x.NumDefaults == 0:
+			// Nothing to do.
+
+		case s.LeftDefault:
+			a := adt.Default(a)
 			var ok bool
 			x, ok = a.(*adt.Disjunction)
 			if !ok {
 				return s.values(a, b)
 			}
+
+		case s.BackwardsCompatibility:
+			// TODO: In backwards compatibility mode we should check that a
+			// value remains concrete. A default may probably be introduced for
+			// a new field that previously did not exist. Is this the correct
+			// place to put this, or does this belong in tools/apicheck?
+
+			fallthrough
+
+		default:
+			// If a has defaults, we need to ensure that the default values of
+			// b are strictly narrower.
+			a := adt.Default(a)
+			b := adt.Default(b)
+
+			if !s.values(a, b) {
+				return false
+			}
 		}
 
 		// A Disjunction subsumes another Disjunction if all values of y are

internal/core/subsume/value_test.go

@@ -175,10 +175,14 @@ func TestValues(t *testing.T) {
 		// Defaults
 		150: {subsumes: false, in: `a: number | *1, b: number | *2`},
 		151: {subsumes: true, in: `a: number | *2, b: number | *2`},
-		152: {subsumes: true, in: `a: int | *float, b: int | *2.0`},
+		152: {subsumes: true, in: `a: int | *float, b: int | *2.0`}, // more specific default
 		153: {subsumes: false, in: `a: int | *2, b: int | *2.0`},
-		154: {subsumes: true, in: `a: number | *2 | *3, b: number | *2`},
-		155: {subsumes: true, in: `a: number, b: number | *2`},
+		154: {subsumes: true, in: `a: number | *2 | *3, b: number | *2`}, // eliminating default
+		155: {subsumes: true, in: `a: number, b: number | *2`},           // introducing default
+		156: {subsumes: true, in: `a: 1 | *2, b: 2`},                     // picking default
+		157: {subsumes: false, in: `a: *1 | 2, b: 2`},                    // default differs
+		158: {subsumes: false, in: `a: *1 | 2, b: 1 | 2`},                // widened default
+		159: {subsumes: false, in: `a: 1, b: *1 | 2`},                    // equal default, but widened value
 
 		// Bounds
 		170: {subsumes: true, in: `a: >=2, b: >=2`},
@@ -439,6 +443,15 @@ func TestValues(t *testing.T) {
 		816: {subsumes: true, in: `a: close({[string]: int|string}), b: close({[string]: int})`, mode: subSchema, skip_v2: true},
 		817: {subsumes: true, in: `a: close({[string]: int|string}), b: close({[string]: string|int})`, mode: subSchema, skip_v2: true},
 
+		830: {subsumes: true, in: `a: 1 | *2, b: 2`, mode: subSchema},  // picking default
+		831: {subsumes: false, in: `a: *1 | 2, b: 2`, mode: subSchema}, // default differs
+		832: {subsumes: false, in: `a: 1, b: *1 | 2`, mode: subSchema}, // equal default, but widened value
+		// 833: {subsumes: true, in: `a: *1 | 2, b: 1 | 2`, mode: subSchema}, // TODO: allow introducing default?
+
+		// TODO: A regular value may not go from concrete to non-concrete, even if
+		// this means that the value is now more general.
+		// Do we test that here?
+
 		854: {subsumes: false, in: `a: {foo: 1}, b: {foo?: 1}`, mode: subOpen},
 		855: {subsumes: true, in: `a: close({}), b: {foo?: 1}`, mode: subOpen},
 		856: {subsumes: true, in: `a: close({}), b: close({foo?: 1})`, mode: subOpen},