expr icon indicating copy to clipboard operation
expr copied to clipboard

Parser may incorrectly handle 0x6E as a number?

Open starbuck-ms opened this issue 5 years ago • 3 comments

Great library!

I have an expression like the following:

data.objectId == 0x6E && data.subObject == 0x0 && data.bitNumber == 1 && data.state == 1

And it generates a parser error like so:

invalid float literal: strconv.ParseFloat: parsing "0x6E": invalid syntax (1:29)                                             Error                                           2020-10-12T20:33:46.631029293Z  local
                | data.objectId == 0x6E && data.subObject == 0x0 && data.bitNumber == 1 && data.state == 1
                | ......................^

One possible suggestion is, the parser.go has the following:

		if strings.ContainsAny(value, ".eE") {
			number, err := strconv.ParseFloat(value, 64)
			if err != nil {
				p.error("invalid float literal: %v", err)
			}
//<cut>
		} else if strings.Contains(value, "x") {
			number, err := strconv.ParseInt(value, 0, 64)
			if err != nil {
				p.error("invalid hex literal: %v", err)
			}
//<cut>
		} else {

Should the order of the if/else-if change? Should we check for hex before we check for exponent?

starbuck-ms avatar Oct 12 '20 20:10 starbuck-ms

Yes, you are right. Actually, regexp will even more clear in this case. Can you make PR with tests?

antonmedv avatar Oct 12 '20 22:10 antonmedv

Ok. Will do...

Looked at the code a bit. When dealing with hex literals, we could have an integer overflow problem? The parser stores the value into an IntegerNode which uses an 'int' and not an 'uint'.

https://play.golang.org/p/fk-fJ4eatNI

package main

import "strconv"
import "fmt"

func main() {

	i, _ := strconv.ParseInt("0xffffffffffffffff", 0, 64)
	fmt.Println(i)

	j, _ := strconv.ParseUint("0xffffffffffffffff", 0, 64)
	fmt.Println(j)

Produces:

9223372036854775807
18446744073709551615

Seems like introducing an UnsignedNode would be helpful, but not trivial since it propagates through the code.

Thoughts?

starbuck-ms avatar Oct 12 '20 23:10 starbuck-ms

Yes, looks like it needed.

antonmedv avatar Oct 13 '20 07:10 antonmedv

Took me more than 2 years, but it finally fixed: 0x6E now parsed correctly =)

antonmedv avatar Nov 05 '22 19:11 antonmedv