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

Include and consider static types of arrays and dictionaries #1043

Conversation

turbolent
Copy link
Member

⚠️ Depends on #1038

Work towards #870

Description

  • Consider static types of arrays and dictionaries in equality tests
  • Include static types in dynamic types for arrays and dictionaries
  • Consider array / dictionary dynamic type's static type in dynamic type check

  • 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

@turbolent turbolent requested a review from SupunS June 30, 2021 23:03
@turbolent turbolent self-assigned this Jun 30, 2021
Base automatically changed from bastian/container-value-import-type-inference to feature/container-static-types July 2, 2021 04:28
Copy link
Member

@SupunS SupunS 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!

I added a couple of suggestions, but not blockers for this PR. So I'll go ahead and merge this 👍

@@ -119,8 +120,14 @@ func (t CompositeDynamicType) IsImportable() bool {

// DictionaryDynamicType

type DictionaryStaticTypeEntry struct {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type DictionaryStaticTypeEntry struct {
type DictionaryDynamicTypeEntry struct {

Comment on lines +2687 to +2690
subTypeStaticType := interpreter.ConvertStaticToSemaType(typedSubType.StaticType)
if !sema.IsSubType(subTypeStaticType, typedSuperType) {
return false
}
Copy link
Member

Choose a reason for hiding this comment

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

Once the runtime mutability checks (checking the type of array/dictionary before insertions) are added, I think we can rely only on this check, and maybe don't need to check both the static type and the dynamic type. Because that 'mutability checks on insertions' ensures that any value within the container is a subtype of the static type.

@SupunS SupunS merged commit 3cfe0f1 into feature/container-static-types Jul 2, 2021
@SupunS SupunS deleted the bastian/static-types-in-dynamic-container-types branch July 2, 2021 05:16
@SupunS SupunS restored the bastian/static-types-in-dynamic-container-types branch July 2, 2021 05:17
@SupunS SupunS deleted the bastian/static-types-in-dynamic-container-types branch December 3, 2021 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants