traefik / yaegi

Yaegi is Another Elegant Go Interpreter
https://pkg.go.dev/github.com/traefik/yaegi
Apache License 2.0
6.94k stars 343 forks source link

Equality is incorrect when `nil` is used as the left argument of `==` #1500

Closed elee1766 closed 1 year ago

elee1766 commented 1 year ago

hi!

this issue is sorta blocking for me so i thought i would try to fix it.

im still learning the codebase and understanding how yaegi works, but I thought I would attempt to add a test in the style of other tests as a start.

please let me know if there is anything i need to change / run, or if anyone knows perhaps a good place to start for tackling this issue

Fixes #1496

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

elee1766 commented 1 year ago

adding fmt.Printf("equals: <%s> <%s> %v \n", t.id(), o.id(), t.id() == o.id()) to line type.go:1504 seems to show

equals:  <[]uint8> <nil> false
equals:  <bool> <bool> true
equals:  <nil> <[]uint8> false
equals:  <bool> <bool> true

which are the same, which perhaps means something is happening later down the line? will keep digging

oh, it seems that's just called by typecheck.go, and yeah those things are matching, so it must be down the line. sorry for the red herring!

elee1766 commented 1 year ago

ok, so these two statements []byte{} == nil and nil == []byte{} seem to be getting optimized somewhere before interpretation?

i say this because the "equals" in "op.go" is not called for those operations (they are for a==false, b==false, and a==b though), but maybe i misunderstand

elee1766 commented 1 year ago

ok, maybe third time is the charm

i added fmt.Printf(`nodes <%s> <%s> %v\n`, c0.name(), n.child[1].name(), value(f).IsNil())

to the frame function near run.go:3880. it seems that the difference materializes here.

so this is consistent with the behavior. It just checks if the first? value is nil, and if it is, returns true? hence why (nil==[]byte{}) returns true

but that means that uh, nil equality is completely broken broken then right?

ok

i updated the test just now, it seems that if nil is the first element in a comparison, the expression always evaluates to true.

elee1766 commented 1 year ago

questions:

  1. i dont actually know if this check is 100% correct? i think it is more correct than the previous one. it matches go run now at least.

  2. i dont know if "nil" is valid in any binary operator that isnt aEqual. if its just aEqual, this fix should be fine?

  3. i couldn't run tests on my local computer, im not sure why, maybe i just cant read

elee1766 commented 1 year ago

1496 is this how github works trying to tag the issue to this pr

im so sorry if someone is subscribed to this issue and im spamming your in box happy new year :)

elee1766 commented 1 year ago

awesome. i think i changed those things. let me know if there is anything else that needs to change.