Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do a checkscalar on NumberKind instead of erroring. #111

Merged
merged 2 commits into from
Feb 10, 2023

Conversation

joeblubaugh
Copy link
Contributor

NumberKind is just FloatKind | IntKind. If the scalar field in the Go type is a float, it can hold the value, and we shouldn't error.

Or at least that's my assertion :-) I don't know what peril this may cause, but it solved a BindType panic that seemed invalid.

`NumberKind` is just `FloatKind | IntKind`. If the scalar field in the
Go type is a float, it can hold the value, and we shouldn't error.
@CLAassistant
Copy link

CLAassistant commented Feb 10, 2023

CLA assistant check
All committers have signed the CLA.

@joeblubaugh
Copy link
Contributor Author

Digging into the cue subsume function, we run into this line in core/adt/binop.go:

case leftKind&NumKind != 0 && rightKind&NumKind != 0:
	// n := c.newNum()
	return cmpTonode(c, op, c.Num(left, op).X.Cmp(&c.Num(right, op).X))

@@ -27,6 +28,7 @@ seqs: [
type TestType struct {
Astring *string `json:"astring"`
Anint int64 `json:"anint"`
Afloat float64 `json:"afloat"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this field causes a panic on main, but passes here.

@sdboyer
Copy link
Contributor

sdboyer commented Feb 10, 2023

i ran into this - basically, cue.FloatKind doesn't seem to be used for value kind under the conditions i would have expected - when working on fixes to cue stdlib's openapi encoder. It may be a bug in core CUE, but it does appear to be the current behavior.

So, LGTM, as the test should now catch any potential change in behavior in CUE. thanks!

@sdboyer sdboyer merged commit 95e2f02 into main Feb 10, 2023
mildwonkey pushed a commit that referenced this pull request May 1, 2023
Do a `checkscalar` on NumberKind instead of erroring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants