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

function: don't pass unexpected marked values to TypeFunc #92

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

jbardin
Copy link
Contributor

@jbardin jbardin commented Mar 11, 2021

ReturnTypeForValues was only checking an unmarking the top level value
for TypeFuncs that did not expect marked values. Use ContainsMarked and
UnmarkDeep to prepare the arguments.

ReturnTypeForValues was only checking an unmarking the top level value
for TypeFuncs that did not expect marked values. Use ContainsMarked and
UnmarkDeep to prepare the arguments.
@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #92 (904c1ed) into main (080b168) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #92      +/-   ##
==========================================
+ Coverage   70.85%   70.94%   +0.09%     
==========================================
  Files          79       79              
  Lines        6636     6636              
==========================================
+ Hits         4702     4708       +6     
+ Misses       1490     1486       -4     
+ Partials      444      442       -2     
Impacted Files Coverage Δ
cty/function/function.go 48.24% <100.00%> (+10.52%) ⬆️
cty/convert/compare_types.go 92.59% <0.00%> (-7.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 080b168...904c1ed. Read the comment docs.

Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

I think this is good as far as safety I can think through on acceptable disclosure w.r.t. the types 🤔 I am surprised we haven't run into this as yet, so just so I can completely grok, do you have an example of where this is useful/what this is addressing?

@jbardin
Copy link
Contributor Author

jbardin commented Mar 12, 2021

Hi @pselle , this particular case takes a combination of constructing values which contain marked values, and passing them to a function which needs to calculate the return type based on those values. The corresponding Terraform issue is: hashicorp/terraform#28049.

@apparentlymart
Copy link
Collaborator

This makes sense to me, so thanks! I think in retrospect this was a missing part of #72 that I neglected to catch during review.

The similar code for this in Call was doing some extra work to try to optimize for cases when work isn't required, which I think this new change defeats in some ways, but I'm going to merge this now because getting the correct behavior is more important than optimizing for performance, and we can always profile and optimize this further in later commits without changing the observable behavior.

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.

3 participants