Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

v0.3.x Fill casts whole-number float to int instead of float #896

Closed
brandoshmando opened this issue Apr 10, 2021 · 6 comments
Closed

v0.3.x Fill casts whole-number float to int instead of float #896

brandoshmando opened this issue Apr 10, 2021 · 6 comments
Labels

Comments

@brandoshmando
Copy link

brandoshmando commented Apr 10, 2021

What version of CUE are you using (cue version)?

$ cue version
cue version 0.3.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

I ran into some interesting behavior changes with the Fill function between v0.2.2 and v0.3.x. With v0.3.x, when filling a number value with a non-concrete value (i.e. default, gt, lt etc.) that contains a "whole number" float (i.e. 3.0), it seems to cast that value to an int instead of a float. Subsequently, when attempting to fill with a concrete, non-whole number float (i.e. 3.5), an error indicating conflicting values occurs. When testing this same scenario with v0.2.2, the value is filled with the proper float value and no error occurs. In addition to observing this with both v0.3.x versions, I also tested with FillPath in both and observed the same error.

I've included a simplified example:

EDIT: better example

package main

import (
	"fmt"

	"cuelang.org/go/cue"
	"cuelang.org/go/cue/ast"
	"cuelang.org/go/cue/token"
)

func main() {
	input := `{
		value: number
	}`

	var r cue.Runtime
	inst, _ := r.Compile("", input)
	val := inst.Value()

	val = val.Fill(float64(3), "value")
	val = val.Fill(ast.NewBinExpr(
		token.AND,
		val.Lookup("value").Syntax().(ast.Expr),
		&ast.UnaryExpr{
			Op: token.LEQ,
			X: &ast.BasicLit{
				Kind:  token.FLOAT,
				Value: fmt.Sprint(float64(3.5)),
			},
		}), "value")
	fmt.Println(val.Lookup("value"))
}

Edit
Admittedly, I'm noticing this can be resolved by ensuring that the concrete value is filled after the constraint,
but it'd be nice if we didn't need to worry about order. Also v.2.2.0 does handle this properly.

What did you expect to see?

I expected to fill a concrete value of type float and a non-concrete value operating on the same type, regardless of order, without encountering a conflict.

(the above recipe with v0.2.2 outputs the following)

3.

What did you see instead?

The following error:

_|_ // conflicting values 3 and 3 (mismatched types float and int)
@brandoshmando
Copy link
Author

One other note: changing the number declaration in the input to float still yields the same error in v0.3.x.

@mpvl
Copy link
Contributor

mpvl commented Apr 11, 2021

Analysis: Fill seems to cat the float to float fine.

It is actually the conversion to Syntax and back that causes the problem. Ironically enough -- and this explains the difference between v0.2 and v0.3 -- this is a result of a change that makes floats and ints more interchangeable. Basically, syntactically, if a float constant is a whole number, it is allowed to be an integer or a float. This increases interoperability with JSON, where 1.0 is often meant to be interpretable as an integer. See also #253.

But something is going wrong in this interpretation, interpreting it too strict as an integer.

@mpvl
Copy link
Contributor

mpvl commented Apr 11, 2021

BTW, this code snippet also seems to indicate it would be useful to people to allow something like MakeExpr(op Op, v ...Value) Value.

@mpvl
Copy link
Contributor

mpvl commented Apr 11, 2021

Minimal reproducer:

	input := `{
		value: number
	}`

	var r cue.Runtime
	inst, _ := r.Compile("", input)
	val := inst.Value()

	val = val.Fill(float64(3), "value")
	val = val.Fill(val.Lookup("value").Syntax().(ast.Expr), "value")

cueckoo pushed a commit that referenced this issue Apr 11, 2021
This avoids round-tripping problems when floats can
be represented as integers.

Fixes #896

The real solution is probably to make integer a direct
subclass of float. This, however, is a far more substantial
change. See #253.

Change-Id: I1fa532677a4ba2dcbe85446fcd7134f5bc3a542d
@brandoshmando
Copy link
Author

@mpvl Thanks for the quick fix! Much appreciated. I kind of figured that it had something to do with the fact that whole number float could be interpreted as int.

As for your comment re: MakeExpr(op Op, v ...Value) Value, something like that would be very helpful to our use case. That is, building up cue values from go logic vs being backed by a cue file.

Thanks again!

@cueckoo
Copy link

cueckoo commented Jul 3, 2021

This issue has been migrated to cue-lang/cue#896.

For more details about CUE's migration to a new home, please see cue-lang/cue#1078.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants