Start supplying attr.Type to reflection.

[paddycarver]

Reflection has A Problem:

We want to support using attr.Values inside of structs, slices, etc. and just having the reflection package Do The Right Thing. This means the reflection package needs to be able to create them.

We originally solved that in #30 by just instantiating empty values of their type, and then calling a SetTerraformValue method on that value. This was super annoying, because then we needed two methods for getting an attr.Value set to the right values: attr.Type.ValueFromTerraform and attr.Value.SetTerraformValue were basically the same thing. But whatever, it worked.

Except, no it didn't.

Complex types like lists and maps store their element/attribute types as properties on their structs. It's important that these be set. Only the attr.Type has this information, it's not passed in as part of the tftypes.Value. So reflect couldn't set those values, and produced broken attr.Values as a result. (Their ToTerraformValue methods would run into trouble, because they wouldn't know what tftypes.Type to use for elements or attributes).

To solve this problem, we decided to supply the attr.Type from the schema to the reflect package, wiring it through so that we could instantiate new attr.Values when the opportunity presented itself.

This solves our problem, because we got rid of the attr.Value.SetTerraformValue method and used the attr.Type.ValueFromTerraform directly to just instantiate a new value, which made sure it was set up correctly. But now we have a new problem: what if the attr.Type in the schema doesn't produce the attr.Value they're trying to assign to? We decided to just throw an error on that one, because there's no reasonable way around it.

Depends on #44.

[kmoe]

LGTM depending on #44

[kmoe]

Actually, I'm not sure I like having to duplicate our type implementations inside the reflect package with testStringType etc. Why not just use a reflect_test package?

[paddycarver]

Actually, I'm not sure I like having to duplicate our type implementations inside the reflect package with testStringType etc. Why not just use a reflect_test package?

Same, but the point of these tests is to test unexported functions, which we can't do if we use a reflect_test package. :( But no amount of package shenanigans will get us around this one, because types has to rely on these functions and these functions have to have concrete types. We could combine them into a single package, but then we can't reuse them in Get, etc. We could move the type implementations internal, and reference them from the types package, but I have a hunch that's gonna get messy real quick.

I don't particularly like the copying, either, but I couldn't think of something that was less bad.

[kmoe]

Yes, good point. I suppose the only other option is to make those functions exported, which has no user impact because the reflect package is internal.

[kmoe]

This just came up because I'm having to copy my ApplyTerraform5AttributePathStep implementations everywhere.

[paddycarver]

Hm, let me give it a shot and see how we like the result.

[kmoe]

Looks good!

[paddycarver]

Doing some last tweaking of comments in the reflect package to make golint happy, then I'll rebase and squash it all down with a nice commit message.

[paddycarver]

Okay, golint wanted some pretty invasive changes, but I think we're good now if you're still happy, @kmoe

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.