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

Concretize the definition of Primitive type #2962

Conversation

darkdrag00nv2
Copy link
Contributor

@darkdrag00nv2 darkdrag00nv2 commented Dec 4, 2023

Closes #2929

Description

This PR concretizes the definition of Primitive type by introducing a IsPrimitiveType function on sema.Type.

The following types have been marked as primitive:

  • Number types
  • Character
  • Bool
  • String
  • Address
  • Path
  • Void
  • Optional<T> - if T is primitive

  • 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

Copy link

codecov bot commented Dec 4, 2023

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (4c9fb2a) 80.11% compared to head (7b3805c) 80.07%.

Files Patch % Lines
runtime/sema/type.go 0.00% 34 Missing ⚠️
runtime/sema/gen/main.go 44.44% 4 Missing and 1 partial ⚠️
runtime/sema/simple_type.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           feature/stable-cadence    #2962      +/-   ##
==========================================================
- Coverage                   80.11%   80.07%   -0.04%     
==========================================================
  Files                         348      348              
  Lines                       81870    81915      +45     
==========================================================
+ Hits                        65590    65594       +4     
- Misses                      13958    13998      +40     
- Partials                     2322     2323       +1     
Flag Coverage Δ
unittests 80.07% <8.88%> (-0.04%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Nice!

@@ -1583,12 +1584,14 @@ func simpleTypeLiteral(ty *typeDecl) dst.Expr {
//}

isResource := ty.compositeKind == common.CompositeKindResource
_, isPrimitive := sema.GeneratedPrimitiveSimpleTypes[ty.typeName]
Copy link
Member

Choose a reason for hiding this comment

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

Similar to how other "kind"s are indicated through conformances in declarations of built-in types which produce Go code, like if a type is storable via conforming to the Storable type (also, Equatable, Comparable, etc), we might consider introducing a Primitive type in the code generator (which later then also could be introduced "properly" in the type checker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 7b3805c.

@@ -25,6 +25,10 @@ import (
"github.com/onflow/cadence/runtime/common"
)

var GeneratedPrimitiveSimpleTypes = map[string]bool{
Copy link
Member

Choose a reason for hiding this comment

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

As commented above, maybe consider replacing this with a notion of Primitive in the code generator. It's difficult to see that a side-table determines additional properties of a type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 7b3805c.

@@ -669,6 +674,10 @@ func (t *OptionalType) IsResourceType() bool {
return t.Type.IsResourceType()
}

func (t *OptionalType) IsPrimitiveType() bool {
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.

Could you please add a test case here and for the other uncovered functions below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What sort of test would make sense here? We are not using this IsPrimitiveType function anywhere. Is there a place where we could use it and then that would automatically lead to coverage?

runtime/sema/type.go Outdated Show resolved Hide resolved
@turbolent turbolent self-assigned this Dec 4, 2023
@dsainati1 dsainati1 deleted the branch onflow:feature/stable-cadence December 11, 2023 14:57
@dsainati1 dsainati1 closed this Dec 11, 2023
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.

3 participants