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

Find least common super-type #1025

Merged
merged 34 commits into from
Nov 11, 2021
Merged

Find least common super-type #1025

merged 34 commits into from
Nov 11, 2021

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Jun 21, 2021

Work towards #61

Description

This PR introduces a utility method to find the "least common supertype", given a list of types.

Approach:

Each type is represented using a bitmask (Type tag).

e.g:

Int -->      0  0  0  0  0  1
Int8 -->     0  0  0  0  1  0
Int16 -->    0  0  0  1  0  0

Integer -->  0  1  1  1  1  1   (i.e: Int | Int8 | Int16 | ...)

Each 'leaf-type' (types that are not a super type of anyone) has a unique bit, and each super-type has a unique bit-pattern within the bitmask.

'Join of types' and the 'type inclusions' can be found through bitwise operations.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@SupunS SupunS requested a review from turbolent June 21, 2021 06:45
@SupunS SupunS self-assigned this Jun 21, 2021
@SupunS SupunS changed the title Supun/common supertype 3 Find least common super-type Jun 21, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2021

Codecov Report

Merging #1025 (1086a93) into master (4348c5d) will increase coverage by 0.07%.
The diff coverage is 94.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1025      +/-   ##
==========================================
+ Coverage   77.46%   77.53%   +0.07%     
==========================================
  Files         273      274       +1     
  Lines       34764    35085     +321     
==========================================
+ Hits        26929    27204     +275     
- Misses       6760     6801      +41     
- Partials     1075     1080       +5     
Flag Coverage Δ
unittests 77.53% <94.27%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/sema/block.go 100.00% <ø> (ø)
runtime/sema/deployed_contract.go 71.42% <ø> (ø)
runtime/sema/meta_type.go 100.00% <ø> (ø)
runtime/sema/path_type.go 100.00% <ø> (ø)
runtime/sema/string_type.go 100.00% <ø> (ø)
runtime/sema/type.go 87.86% <80.95%> (-0.07%) ⬇️
runtime/sema/type_tags.go 95.41% <95.41%> (ø)
runtime/sema/simple_type.go 92.85% <100.00%> (+0.26%) ⬆️
runtime/convertTypes.go 76.54% <0.00%> (-2.61%) ⬇️
encoding/json/decode.go 71.12% <0.00%> (-2.07%) ⬇️
... and 6 more

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 4348c5d...1086a93. Read the comment docs.

@SupunS SupunS force-pushed the supun/common-supertype-3 branch from cf611de to 032de07 Compare June 21, 2021 07:06
@SupunS SupunS added the Feature label Jun 21, 2021
@SupunS SupunS force-pushed the supun/common-supertype-3 branch from 032de07 to 7713243 Compare June 21, 2021 07:28
@SupunS SupunS force-pushed the supun/common-supertype-3 branch 3 times, most recently from 9a8b04f to b0d42b2 Compare June 21, 2021 11:32
@SupunS SupunS force-pushed the supun/common-supertype-3 branch from b0d42b2 to 5ca55cb Compare June 21, 2021 11:33
@SupunS SupunS force-pushed the supun/common-supertype-3 branch from 1033722 to b6c5c9b Compare June 24, 2021 05:53
runtime/sema/type_tags.go Outdated Show resolved Hide resolved
@SupunS SupunS force-pushed the supun/common-supertype-3 branch from cab37bd to 351fdf6 Compare November 5, 2021 22:49
@SupunS SupunS force-pushed the supun/common-supertype-3 branch from 351fdf6 to 56f6e53 Compare November 5, 2021 22:52
@SupunS SupunS requested a review from dsainati1 November 6, 2021 01:52
Copy link
Contributor

@dsainati1 dsainati1 left a comment

Choose a reason for hiding this comment

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

This is really awesome, the bitmask approach is a really cool idea. I wonder though whether it produces some less precise results, since it doesn't take into account the variance of derived types when computing supertypes; it seems based on the test cases that the least common supertype of [String] and [Bool] is AnyStruct, for example, when it could be [AnyStruct].

I couldn't say whether adding this additional precision is valuable or not, but given that arrays, dictionaries and other structures in cadence are all covariant, it seems like we could take advantage of this to produce some more specific results: when the kind of the "container" derived type is the same in a list of types, we should produce a type for their least common supertype that has the same kind for its derived "container".

runtime/sema/type.go Outdated Show resolved Hide resolved
runtime/sema/type_tags.go Outdated Show resolved Hide resolved
runtime/sema/type_tags.go Outdated Show resolved Hide resolved
runtime/sema/type_tags.go Outdated Show resolved Hide resolved
runtime/sema/type_tags.go Outdated Show resolved Hide resolved
runtime/sema/type_test.go Show resolved Hide resolved
runtime/sema/type_test.go Show resolved Hide resolved
runtime/sema/type_test.go Show resolved Hide resolved
runtime/sema/type_test.go Show resolved Hide resolved
runtime/sema/type_test.go Show resolved Hide resolved
@SupunS SupunS force-pushed the supun/common-supertype-3 branch from 063795b to 7055e5a Compare November 8, 2021 18:25
@SupunS
Copy link
Member Author

SupunS commented Nov 8, 2021

Thank you @dsainati1 for the review!

Regarding the containers, as I mentioned in the comment, we would need to do some extra bit of work to get it working (e.g.: Need a Cadence type to represent all keyable types, etc.), so currently we infer AnyStruct as more of a 'safe' and a simple solution. This PR introduces the base for us to implement complex scenarios like the one you suggested, and we could definitely do it in another iteration/as an improvement. Created an issue #1225 for tracking.

Copy link
Contributor

@dsainati1 dsainati1 left a comment

Choose a reason for hiding this comment

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

Looks good to me; thanks for opening the issue to follow up on the recursive strategy!

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Great work! This is going to be very useful 👏

runtime/sema/simple_type.go Show resolved Hide resolved
runtime/sema/type_tags.go Outdated Show resolved Hide resolved
runtime/sema/type_tags.go Show resolved Hide resolved
runtime/sema/type_tags.go Show resolved Hide resolved
runtime/sema/type_tags.go Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Nov 10, 2021

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 4348c5d
The command go test ./... -run=XXX -bench=. -shuffle=on -count N was used.
Bench tests were run a total of 7 times on each branch.

Results

old.txtnew.txt
time/opdelta
RuntimeStorageWriteCached-2120µs ± 1%124µs ± 5%+3.70%(p=0.017 n=7+7)
ContractInterfaceFungibleToken-239.2µs ± 1%39.8µs ± 2%+1.58%(p=0.005 n=6+7)
ParseDeploy/decode_hex-21.19ms ± 0%1.20ms ± 1%+0.67%(p=0.011 n=7+7)
RuntimeResourceDictionaryValues-214.7ms ± 1%14.3ms ± 9%~(p=0.209 n=7+7)
RuntimeFungibleTokenTransfer-21.04ms ± 2%1.06ms ± 4%~(p=0.209 n=7+7)
ParseInfix-221.3µs ± 3%21.2µs ± 1%~(p=0.710 n=7+7)
ParseArray-219.6ms ± 1%19.8ms ± 2%~(p=0.101 n=6+7)
CheckContractInterfaceFungibleTokenConformance-2133µs ± 2%134µs ± 7%~(p=0.902 n=7+7)
NewInterpreter/new_interpreter-2991ns ± 3%990ns ± 3%~(p=0.836 n=7+6)
NewInterpreter/new_sub-interpreter-21.79µs ± 1%1.78µs ± 1%~(p=0.435 n=7+7)
InterpretRecursionFib-22.42ms ± 1%2.40ms ± 1%~(p=0.065 n=6+6)
ParseFungibleToken-2402µs ± 2%398µs ± 1%−0.90%(p=0.038 n=7+7)
ParseDeploy/byte_array-229.9ms ± 2%29.4ms ± 1%−1.51%(p=0.026 n=7+7)
QualifiedIdentifierCreation/Three_levels-2142ns ± 1%139ns ± 1%−2.36%(p=0.002 n=7+7)
QualifiedIdentifierCreation/One_level-22.68ns ± 0%2.35ns ± 0%−12.52%(p=0.001 n=7+7)
 
alloc/opdelta
RuntimeResourceDictionaryValues-24.33MB ± 0%4.33MB ± 0%~(p=0.051 n=6+7)
RuntimeFungibleTokenTransfer-2225kB ± 0%225kB ± 0%~(p=0.434 n=7+7)
RuntimeStorageWriteCached-283.7kB ± 0%83.7kB ± 0%~(p=0.098 n=6+7)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
ContractInterfaceFungibleToken-226.7kB ± 0%26.7kB ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-266.4kB ± 0%66.4kB ± 0%~(all equal)
NewInterpreter/new_interpreter-2680B ± 0%680B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-21.02kB ± 0%1.02kB ± 0%~(all equal)
InterpretRecursionFib-21.21MB ± 0%1.21MB ± 0%~(p=0.385 n=6+7)
 
allocs/opdelta
RuntimeResourceDictionaryValues-2108k ± 0%108k ± 0%~(p=0.272 n=6+7)
RuntimeFungibleTokenTransfer-24.38k ± 0%4.38k ± 0%~(all equal)
RuntimeStorageWriteCached-21.42k ± 0%1.42k ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
ContractInterfaceFungibleToken-2458 ± 0%458 ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-21.07k ± 0%1.07k ± 0%~(all equal)
NewInterpreter/new_interpreter-211.0 ± 0%11.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-230.0 ± 0%30.0 ± 0%~(all equal)
InterpretRecursionFib-225.0k ± 0%25.0k ± 0%~(all equal)
 

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Great work! 👏 👍

@turbolent
Copy link
Member

@SupunS Sorry it took so long to get this in!

@SupunS SupunS merged commit c4c1392 into master Nov 11, 2021
@SupunS SupunS deleted the supun/common-supertype-3 branch November 11, 2021 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants