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

Fix set function unification #52

Merged
merged 2 commits into from
May 18, 2020

Conversation

alisdair
Copy link
Contributor

Two changes here to fix panics when using the stdlib set functions (setintersect, setunion, etc) where one of the parameters is an empty collection.

Add Equals method to set.Rules

Previously, we checked if two sets had the same rules using a simple equality check. This panics if the sets contain object types, as they cannot be compared using ==.

Adding an Equals method to the Rules interface allows us to delegate to the Type.Equals method where necessary, fixing this problem.

Fix set function crashes with empty sets

If one or more arguments to the stdlib set functions was an empty set of dynamic pseudo type, the functions would panic due to incompatible set rules.

We can special case empty dynamic pseudo type sets to be ignored through type unification, because they are always capable of being converted to any other type.

Downstream crash

This PR fixes this Terraform crash:

$ echo 'setsubtract([{}], [])' | tf console

Error: Error in function call

  on /Users/alisdair/code/terraform/main.tf line 2, in output "x":
   2:   value = setsubtract([{}], [])

Call to function "setsubtract" failed: panic in function implementation:
incompatible set rules: cty.setRules{Type:cty.EmptyObject},
cty.setRules{Type:cty.DynamicPseudoType}

Copy link
Collaborator

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Nice, thanks for digging into this! I have a little inline comment about naming, but functionality-wise this looks great.

cty/set/rules.go Outdated
// Equals returns true if the instance is equal to another Rules instance.
// This method is necessary because some implementations may be backed by
// structs which cannot be compared with ==.
Equals(Rules) bool
Copy link
Collaborator

@apparentlymart apparentlymart May 16, 2020

Choose a reason for hiding this comment

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

Equals is consistent with how a method like this is named elsewhere, but it feels a little confusing in this case because it's alongside Equivalent which is about comparing potential values, rather than the rules object itself.

What do you think about naming this SameRules instead, just to make it a bit clearer that we're talking about comparing the rules object itself? 🤔

Previously, we checked if two sets had the same rules using a simple
equality check. This panics if the sets contain object types, as they
cannot be compared using `==`.

Adding a SameRules method to the Rules interface allows us to delegate
to the Type.Equals method where necessary, fixing this problem.
If one or more arguments to the stdlib set functions was an empty set of
dynamic pseudo type, the functions would panic due to incompatible set
rules.

We can special case empty dynamic pseudo type sets to be ignored through
type unification, because they are always capable of being converted to
any other type.
@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #52 into master will increase coverage by 0.05%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   70.48%   70.53%   +0.05%     
==========================================
  Files          79       79              
  Lines        7138     7151      +13     
==========================================
+ Hits         5031     5044      +13     
  Misses       1664     1664              
  Partials      443      443              
Impacted Files Coverage Δ
cty/set/set.go 66.66% <50.00%> (ø)
cty/function/stdlib/set.go 69.23% <100.00%> (+3.27%) ⬆️
cty/path_set.go 67.08% <100.00%> (+1.29%) ⬆️
cty/set_internals.go 97.69% <100.00%> (+0.09%) ⬆️

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 af2c7a1...ac84a5d. Read the comment docs.

@apparentlymart
Copy link
Collaborator

I did the change to rename Equals to SameRules myself, because I'm about to tag the 1.4.1 release and wanted to make sure this would be in it. Thanks again for looking into this!

@apparentlymart apparentlymart merged commit c5bd314 into zclconf:master May 18, 2020
@alisdair alisdair deleted the fix-set-function-unification branch May 21, 2020 11:41
@alisdair
Copy link
Contributor Author

Thanks, that rename makes sense to me!

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