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

convert: unify tuples as lists and objects as maps when possible #89

Merged
merged 4 commits into from Mar 10, 2021

Conversation

jbardin
Copy link
Contributor

@jbardin jbardin commented Mar 5, 2021

Tuple and list unification is a common case due to the fact that list data is often in tuple form, either because of configuration language constructs or functions that must deal with possibly heterogeneous collections. In the current code, we would immediately fall back to type conversion, which has different semantics from type unification. The problem is most obvious when the tuples contains objects (which are often maps is disguise for the same reason that lists and tuples are interchanged), and the objects go through type conversion rather than type unification, where unsafe conversion can yield objects with only the intersection of attributes rather than a unified map.

Instead of falling back to direct conversion when there are only tuples and lists involved, we stay in the unification codepath and first unify the tuples as lists. That unification can correctly handle the objects contained within the tuples, then the unification of the resulting lists follows.

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #89 (4f50d5e) into main (30f6dd6) will increase coverage by 0.15%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #89      +/-   ##
==========================================
+ Coverage   70.69%   70.85%   +0.15%     
==========================================
  Files          79       79              
  Lines        6580     6636      +56     
==========================================
+ Hits         4652     4702      +50     
- Misses       1484     1490       +6     
  Partials      444      444              
Impacted Files Coverage Δ
cty/convert/unify.go 81.99% <78.57%> (+1.34%) ⬆️
cty/convert/compare_types.go 100.00% <0.00%> (+2.46%) ⬆️

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 30f6dd6...4f50d5e. Read the comment docs.

@apparentlymart
Copy link
Collaborator

Hi @jbardin! Thanks for working on this.

The implementation and test cases you added here make sense to me, but unfortunately #88 changed a bit during review so I think this'll need to be rebased onto latest main branch (now that #88 is merged) to make sure the underlying assumptions this was built on didn't change. Would you mind doing that and force-pushing the result back up here so we can see what affect that has on the test cases? Thanks!

List data is often contained within tuple types, either do to language
constructs (hcl tuple literals) or function return types. When presented
with only lists and tuples for unification, first check if the tuples
themselves unify as lists. If they do, run the unification again with
only the list types, and wrap the conversions as necessary.
@jbardin
Copy link
Contributor Author

jbardin commented Mar 8, 2021

Thanks @apparentlymart, rebased against main.

Like tuples and lists, objects and maps are often just different types
wrapped around the same data structures. When unifying combinations of
objects and maps, first see if the objects will unify as maps allowing
the possible unification of all arguments to maps.
@jbardin jbardin changed the title convert: unify tuples as lists when possible convert: unify tuples as lists and objects as maps when possible Mar 10, 2021
@jbardin
Copy link
Contributor Author

jbardin commented Mar 10, 2021

Added a similar case for the unification of objects and maps, for the same underlying reasons as the unification of tuples and lists.

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