Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

Fixes nil type handling for array / hash #21

Merged
merged 1 commit into from
Mar 3, 2016

Conversation

mikesimons
Copy link
Contributor

mrb := NewMrb()
defer mrb.Close()

result, _ := mrb.LoadString(`[false]`)
resultArray := result.Array()
val, _ := resultArray.Get(0)
fmt.Printf("%s", val.String())

Expected: Print string "false"
Actual: panic: runtime error: invalid memory address or nil pointer dereference

This check (which I presume is intended to catch nil) is converting boolean false to nil. Mruby nil type isn't really a type and needs special care with the mrb_nil_p macro to handle it. IMO this is not a detail that go-mruby users should have to know.

Hash is similarly affected

Sadly there isn't a particularly tidy way to handle this. An MrbValue may contain nil but ValueType has no way of expressing this. It will always look like TypeFalse.

This patch introduces TypeNil as a go-mruby specific detail. In order to avoid collision with future types the max for uint32 is used (of which ValueType is an alias).

Tests for MrbValue.Type have also been added where possible. Some types don't appear to exposed to userland so these have been left commented.

During testing I also noticed that Decode handling is likely to trip up on nil types with unknown type errors thrown. It's highly likely that at some point Decode will need to handle this and the most sensible option (IMO) would be to use golang zero types where necessary but that is beyond the scope of this PR.

I stumbled on this while implementing the return helper referenced in #20.

@mikesimons
Copy link
Contributor Author

These changes will be required to support any sane implementation of the helper mentioned in #20. The common return helper would align them all to either convert (or not which I suspect may be better in usage) so the changes to Array.Get and Hash.Delete may be moot but the other bits are still required for sane nil handling.

At present there are quite a few functions that return an MrbValue but only these two functions do this nil check.

@mitchellh
Copy link
Owner

Thanks this looks really great. Your code quality and test quality is wonderful.

Do you mind opening an issue about the Decode handling so that it is known that that needs to be fixed? I'll otherwise likely forget. Merging this!

mitchellh added a commit that referenced this pull request Mar 3, 2016
Fixes nil type handling for array / hash
@mitchellh mitchellh merged commit 5bf3c6f into mitchellh:master Mar 3, 2016
@mikesimons
Copy link
Contributor Author

Why thank you 😀

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

Successfully merging this pull request may close these issues.

None yet

2 participants