fix: Improve error message returned by Subsume #3862

GrahamDennis · 2025-03-29T00:20:31Z

Previously when Subsume determined that a field did not subsume a previous field, the error returned was:

field x not present in <value>

which is simply not true. Now a more useful error is returned in this scenario:

31 | 33 does not subsume 32:
    ./schema.cue:2:25
    ./schema.cue:7:25

Resolves: #3861

mvdan · 2025-03-29T00:25:43Z

Thanks for the patch. Could you add a test? Interestingly, we don't seem to have no tests for "field ... not present in ...". I would imagine this test would go somewhere under cue/testdata. Best if you add the test in a first commit, then the second commit can update the expected output and show the improvement in behavior.

Signed-off-by: Graham Dennis <[email protected]>

GrahamDennis · 2025-03-29T05:10:31Z

Thanks for the patch. Could you add a test? Interestingly, we don't seem to have no tests for "field ... not present in ...". I would imagine this test would go somewhere under cue/testdata. Best if you add the test in a first commit, then the second commit can update the expected output and show the improvement in behavior.

Thanks for the feedback. I've done this now.

The first commit contains just the new tests and contains some examples that I think have incorrect error messages:

-- out/TestValues/407/{foo:_1}_⊑_{foo?:_1} --
Errors:
field foo not present in {foo?:1}:
    value:1:17
missing field "foo"

Result:
false

After the second commit, the updated error message is:

-- out/TestValues/407/{foo:_1}_⊑_{foo?:_1} --
Errors:
1 does not subsume 1:
    value:1:10
    value:1:24
1 does not subsume 1?:
    value:1:10
    value:1:24

Result:
false

edit: I fixed a bug in my change with the error messages generated by the v3 evaluator vs the v2 evaluator.

Previously when Subsume determined that a field did not subsume a previous field, the error returned was: ``` field x not present in <value> ``` which is simply not true. Now a more useful error is returned in this scenario: ``` 31 | 33 does not subsume 32: ./schema.cue:2:25 ./schema.cue:7:25 ``` Resolves: cue-lang#3861 Signed-off-by: Graham Dennis <[email protected]>

mpvl · 2025-04-11T11:37:56Z

internal/core/subsume/vertex.go

-		s.missing = f
-		s.gt = a
-		s.lt = y
+		if b == nil {


You are correct. I have removed this and also set the s.missing et al fields inside the block above.

mpvl · 2025-04-11T11:38:22Z

internal/core/subsume/vertex.go

-		s.missing = f
-		s.gt = a
-		s.lt = y
+		if b == nil {


I'm unclear how this condition can ever be true: if b == nil, b will be set to a non-nil value in the block above.

You are correct. I have removed this and also set the s.missing et al fields inside the block above.

mpvl · 2025-04-11T11:46:57Z

internal/core/subsume/testdata/subsume.txtar

@@ -0,0 +1,4837 @@
+-- out/TestValues/0/__⊑__/v2 --


I don't see any test inputs here. Am I missing something?

This is driven by the existing test cases in https://github.com/cue-lang/cue/pull/3862/files#diff-5c570d819733910b5664134701d7cb95c9efbf2261b72b32b3ff5339ef18bb27L52-R77.
What I wanted to do was re-use all of those test cases and then use the txtar tooling for rendering the error messages.

Signed-off-by: Graham Dennis <[email protected]>

GrahamDennis · 2025-04-12T04:43:56Z

internal/core/subsume/vertex.go

-		s.lt = y
+		s.lt = b
+
+		s.errf("%v%s does not subsume %v%s", a, a.ArcType.Suffix(), b, b.ArcType.Suffix())


This has been modified to improve the error for situations like: {foo:_1}_⊑_{foo?:_1}. Currently the error is:

1 does not subsume 1

But there will now be a new error of

1 does not subsume 1?

This may be considered an abuse of syntax, but it's a starting point for discussion.

GrahamDennis · 2025-04-12T04:46:20Z

internal/core/subsume/testdata/subsume.txtar

@@ -0,0 +1,4837 @@
+-- out/TestValues/0/__⊑__/v2 --


This is driven by the existing test cases in https://github.com/cue-lang/cue/pull/3862/files#diff-5c570d819733910b5664134701d7cb95c9efbf2261b72b32b3ff5339ef18bb27L52-R77.
What I wanted to do was re-use all of those test cases and then use the txtar tooling for rendering the error messages.

GrahamDennis · 2025-04-12T04:47:11Z

internal/core/subsume/vertex.go

-		s.missing = f
-		s.gt = a
-		s.lt = y
+		if b == nil {


You are correct. I have removed this and also set the s.missing et al fields inside the block above.

GrahamDennis · 2025-04-12T04:47:15Z

internal/core/subsume/vertex.go

-		s.missing = f
-		s.gt = a
-		s.lt = y
+		if b == nil {


You are correct. I have removed this and also set the s.missing et al fields inside the block above.

GrahamDennis requested a review from cueckoo as a code owner March 29, 2025 00:20

Add tests for errors generated by subsume

1856281

Signed-off-by: Graham Dennis <[email protected]>

GrahamDennis force-pushed the gdennis/improve-subsumption-error branch from 023fd6d to 0acd636 Compare March 29, 2025 05:00

GrahamDennis force-pushed the gdennis/improve-subsumption-error branch from 3262af0 to 63e5fcd Compare March 29, 2025 05:15

GrahamDennis force-pushed the gdennis/improve-subsumption-error branch from 63e5fcd to d50ef8c Compare March 29, 2025 05:38

mpvl reviewed Apr 11, 2025

View reviewed changes

fix: Remove dead code and respond to feedback.

d27ca9c

Signed-off-by: Graham Dennis <[email protected]>

GrahamDennis force-pushed the gdennis/improve-subsumption-error branch from 26551a3 to d27ca9c Compare April 12, 2025 04:49

GrahamDennis commented Apr 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Improve error message returned by Subsume #3862

fix: Improve error message returned by Subsume #3862

GrahamDennis commented Mar 29, 2025

mvdan commented Mar 29, 2025

GrahamDennis commented Mar 29, 2025 •

edited

Loading

mpvl Apr 11, 2025

GrahamDennis Apr 12, 2025

mpvl Apr 11, 2025

GrahamDennis Apr 12, 2025

mpvl Apr 11, 2025

GrahamDennis Apr 12, 2025

GrahamDennis Apr 12, 2025

GrahamDennis Apr 12, 2025

GrahamDennis Apr 12, 2025

GrahamDennis Apr 12, 2025

fix: Improve error message returned by Subsume #3862

Are you sure you want to change the base?

fix: Improve error message returned by Subsume #3862

Conversation

GrahamDennis commented Mar 29, 2025

mvdan commented Mar 29, 2025

GrahamDennis commented Mar 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrahamDennis commented Mar 29, 2025 •

edited

Loading