Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

formatlist: a DynamicVal arg may be a list #115

Merged
merged 1 commit into from Aug 18, 2021

Conversation

jbardin
Copy link
Contributor

@jbardin jbardin commented Aug 10, 2021

When formatlist encounters a DynamicVal argument, it must return an
unknown result, as it cannot determine if that will be a single value
argument resulting in an unknown string, or a list to be formatted into
an unknown number of strings.

When formatlist encounters a DynamicVal argument, it must return an
unknown result, as it cannot determine if that will be a single value
argument resulting in an unknown string, or a list to be formatted into
an unknown number of strings.
Comment on lines +842 to +845
// The current Function implementation will default to DynamicVal
// if AllowUnknown is true, even though this function has a static
// return type
cty.DynamicVal,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm kinda surprised to see this work at all because (as I think your comment here is noting) Function.Call is supposed to guarantee to return a value that conforms to whatever Function.ReturnTypeForValues would've returned given the same arguments, and cty.DynamicVal doesn't conform to cty.List(cty.String). 🤔

// Returned value must conform to what the Type function expected, to
// protect callers from having to deal with inconsistencies.
if errs := retVal.Type().TestConformance(expectedType); errs != nil {
panic(fmt.Errorf(
"returned value %#v does not conform to expected return type %#v: %s",
retVal, expectedType, errs[0],
))
}

I'm fine with leaving that bug to be solved in a separate PR, but was wondering if you did any investigation here that revealed why exactly this case returns the wrong type. If you don't know then I'm happy to go digging for that myself, but asking just in case you already did the research here. 😀

Copy link
Contributor Author

@jbardin jbardin Aug 17, 2021

Choose a reason for hiding this comment

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

Nope, I didn't dig into the Function.Call implementation yet. If I recall when I read through it for this bug, it looked like returning cty.DynamicVal was simply the easy path, but there was room for more accurate type checking which we could evaluate in a separate PR.

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.

None yet

2 participants