Skip to content

make go-cmp Equal succeed#4361

Open
ldemailly wants to merge 1 commit intotinygo-org:devfrom
ldemailly:busy_splitting_commit_2
Open

make go-cmp Equal succeed#4361
ldemailly wants to merge 1 commit intotinygo-org:devfrom
ldemailly:busy_splitting_commit_2

Conversation

@ldemailly
Copy link
Contributor

  • Avoid panic in go-cmp when it tries to find an Equal method
    • This one is a bit dicey as I don't know how to make it conditionally crash/not crash (crash for people who think it works, not crash for people like go-cmp for which it's only one option)

Splitting commits from #4356

@ldemailly
Copy link
Contributor Author

'Test' is here

https://github.com/grol-io/grol/blob/v0.24.0/ast/modify_test.go#L130

panics without this fix, also

https://github.com/google/go-cmp/blob/v0.6.0/cmp/compare.go#L312-L314

in the comment hopefully makes it clear how this helps
(of note, if you do have an equal method it won't be used, yet... still better than panic imo)

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

This will make it very difficult to debug existing code that calls MethodByName: now it's very difficult to find why existing code (that calls MethodByName) works differently in TinyGo. Therefore, I think this is a bad idea.

@aykevl
Copy link
Member

aykevl commented Jul 26, 2024

Would it be possible to change go-cmp instead to not call MethodByName?

@ldemailly
Copy link
Contributor Author

I don't think go-cmp are (or should be, really) concerned with adding special case for incomplete go, so it would need to be on tinygo's side (or users of both like me)

Not sure btw that it's hard to find callers without panic

maybe something with some build tag then?

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.

2 participants