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 <graham.dennis@gmail.com>

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 <graham.dennis@gmail.com>

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.

We typically want to keep the tests together. So either put input an output in txtar, or do everything in a table.

Also, we prefer not to change the general design of test. It is a lot of code to review! Finally, we want to separate out semantic from structural changes. So if we want to change the generic test structure, this should be a separate CL.

Signed-off-by: Graham Dennis <graham.dennis@gmail.com>

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 · 2025-05-14T09:26:28Z

Ping on this PR?

mvdan · 2025-05-14T09:39:39Z

We haven't forgotten about your PRs; we're just in a bit of a crunch for the v0.13 release with the new evaluator :) Please bear with us, and thank you for your patience!

GrahamDennis · 2025-06-25T10:59:01Z

ping @mvdan ? Any update here now that 0.13 is out?

GrahamDennis · 2025-07-16T01:32:22Z

ping @mvdan / @mpvl ?

mvdan · 2025-07-16T20:03:38Z

Thanks; we haven't forgotten about this. Please understand that we have a number of competing priorities at the moment, and a small team.

mpvl

Thanks for your updates and pardon the delay.

Firstly, this is quite a complex change. For anything but trivial changes we typically require moving over to Gerrit, for several reasons:

all reviews documented in a single place,
code review capabilities of GH are rather poor (for instance no fine-grained colored diffing), which makes it really hard to review,
allow commenting on commit messages (for instance, headers do not follow our pattern).

I made some comments, but I suggest moving over to Gerrit henceforth.

mpvl · 2025-08-05T11:38:29Z

internal/core/subsume/testdata/subsume.txtar

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


We typically want to keep the tests together. So either put input an output in txtar, or do everything in a table.

Also, we prefer not to change the general design of test. It is a lot of code to review! Finally, we want to separate out semantic from structural changes. So if we want to change the generic test structure, this should be a separate CL.

mpvl · 2025-08-05T11:47:47Z

internal/core/subsume/subsume.go

-			s.errf("missing field %q", s.missing.SelectorString(c))
-		} else if b, ok := unifyValue(c, s.gt, s.lt).(*adt.Bottom); !ok {
-			s.errf("value not an instance")
+			s.errf("field %q not present in %v", s.missing.SelectorString(c), s.lt)


This commit contains rephrasing of error messages, as well as logic changes. This makes it non-trivial to see which tests change because of what. These should be separated out in different CLs.

mpvl · 2025-08-05T15:12:19Z

I've updated the testing setup for TestValues. It now tests for the particular error message. You can run

CUE_UPDATE=1 go test ./internal/core/subsume

To update the generated error messages.

The reviews for this are still pending, though: https://review.gerrithub.io/c/cue-lang/cue/+/1219851/1

mvdan · 2025-09-01T14:05:59Z

Friendly nudge @GrahamDennis, not sure if you saw Marcel's replies above. You can see https://github.com/cue-lang/cue/blob/master/CONTRIBUTING.md in terms of sending patches on Gerrit.

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 <graham.dennis@gmail.com>

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 <graham.dennis@gmail.com>

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

mpvl reviewed Aug 5, 2025

View reviewed changes

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

Uh oh!

mvdan commented Mar 29, 2025

Uh oh!

GrahamDennis commented Mar 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GrahamDennis commented May 14, 2025

Uh oh!

mvdan commented May 14, 2025

Uh oh!

GrahamDennis commented Jun 25, 2025

Uh oh!

GrahamDennis commented Jul 16, 2025

Uh oh!

mvdan commented Jul 16, 2025

Uh oh!

mpvl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpvl commented Aug 5, 2025

Uh oh!

mvdan commented Sep 1, 2025

Uh oh!

Uh oh!

GrahamDennis commented Mar 29, 2025 •

edited

Loading