reflect: StructOf panics when using unexported fields #432
Comments
Seems the fields are private. We shouldn't panic, but that's one unrelated issue. |
|
Yeah, they should be public, as discussed in IRC. Making them private was a last-ditch attempt at bypassing the type unification error when they were public. I thought reflect might do the heavy lifting, but it was a slim chance. :P |
|
Yeah, I'll recap the issue here in case anyone is interested: When a golang resource has a struct that needs fulfilling, we need to feed it with a matching struct in mcl. The two are not exactly identical, because a golang struct can have empty fields (that default to the zero value) where as a struct in mcl has an exact type which contains each and every field! So how do we solve the situation where we might only want to specify only one or two of the possible fields in a struct? It's tricky because in mcl: is a DIFFERENT TYPE ENTIRELY from: Here's where the magic of the unification comes in. At mcl run time, all the types must be exact, but at mcl compile time we work out what everything should be. Printf being dynamic here is the canonical example. So all we need to do is unify any of the possible struct input permutations which could be valid against the expected golang type, and if we find a single match, then we're golden! Eg: if we're expected in golang a Interestingly this is the same issue that we need to solve for the meta param field in the struct. We should probably add it as property named Line 745 in e3eefeb Questions welcome! |
When I make the fields public, and specify all of them in my mcl file, I still get a type unification error. Here's the updated code: And here's the result of running it: Is this still the same issue? |
Exactly. (Not the panic.) This is as expected until I patch the problem in #432 (comment) Sorry that this blocks you. I'll try and work on it next. |
|
No worries. I can work around it for the time-being (and shouldn't need to actually run any code for a while.) |
|
Okay, so you shouldn't actually be blocked on this anymore. I fixed a small bug, and added some fixes and some tests. Eg: The one catch is that your struct must be complete, not partial. Eg if it's defined as: You must specify both those fields. I'll eventual consider adding the code that let's you have a struct like: That will not fail during unification if you leave one or more of the fields out. Please lmk if you have any more issues. I'll investigate the panic too! (Coming soon.) |
|
Fix for the panic is in f30842e (will merge when tests pass.) Thanks for the report! Virtually all panics are bad, and even if it's the user's "fault" we shouldn't really panic, it just meant they found a clever way to poke us that we should have prevented! I'm going to close this, please ping or reopen if you have any more issues. |
|
Actually, I think there is another bug or maybe there is a heisenbug happening on my machine. I thought I fixed it, but now... Hmmm. |
|
Oh. I might have "fixed" something just now, which actually unintentionally triggers this :/ |
|
@jonathangold I think this should be fixed now, but I didn't retest the exact code. We have a newer dhcp resource already merged that uses automatic grouping instead of complex internal structs, but this was still a valid bug. If you can still repro, please ping or open a new issue! Thanks =D |
Versions:
mgmt version (eg:
mgmt --version):mgmt version 0.0.15-127-g96dcccawhy isn't this 0.0.16...?operating system/distribution (eg:
uname -a):Linux skynet 4.19.10-300.fc29.x86_64 #1 SMP Mon Dec 17 15:34:44 UTC 2018 x86_64 x86_64 x86_64 GNU/Linuxgolang version (eg:
go version):go version go1.11.2 linux/amd64Description:
Panic Reproducer: https://gist.github.com/jonathangold/58cc2f2fb2bcc460a546e1b09c512e95
When I run the .mcl file included with the above panic reproducer, it panics:
related issue: golang/go#25401
The text was updated successfully, but these errors were encountered: