Type Checking for Attachment Declarations #2082
Conversation
… into sainati/attachments-checking
… into sainati/attachments-checking
… into sainati/attachments-checking
… into sainati/attachments-checking
Codecov Report
@@ Coverage Diff @@
## feature/attachments #2082 +/- ##
=======================================================
+ Coverage 77.92% 77.99% +0.06%
=======================================================
Files 308 308
Lines 64362 64623 +261
=======================================================
+ Hits 50156 50402 +246
- Misses 12401 12413 +12
- Partials 1805 1808 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/attachments commit 269df27 Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -610,6 +610,11 @@ func (interpreter *Interpreter) VisitProgram(program *ast.Program) { | |||
| interpreter.visitGlobalDeclaration(declaration) | |||
| } | |||
|
|
|||
| // TODO: Add this | |||
| //for _, declaration := range program.AttachmentDeclarations() { | |||
There was a problem hiding this comment.
This is going to get implemented in a couple PRs in the future
| @@ -924,6 +929,11 @@ func (interpreter *Interpreter) VisitCompositeDeclaration(declaration *ast.Compo | |||
| return nil | |||
| } | |||
|
|
|||
| func (interpreter *Interpreter) VisitAttachmentDeclaration(declaration *ast.AttachmentDeclaration) StatementResult { | |||
| // TODO: fill this in | |||
There was a problem hiding this comment.
Also for a future PR
| // they are subject to many of the same checks. In order to avoid duplicating these checks, we create a mapping | ||
| // between an attachment declaration and its composite "equivalent" that we can use for these composite checks | ||
| func (checker *Checker) attachmentAsComposite(declaration *ast.AttachmentDeclaration) *ast.CompositeDeclaration { | ||
| compositeDelcaration, ok := checker.Elaboration.AttachmentCompositeDeclarations[declaration] |
There was a problem hiding this comment.
It's important that two physically equal attachment declarations also have physically equal composite equivalents, so we cannot generate a new declaration every time we call this. Instead we cache the results of each call and return the cached value if we see the same attachment declaration again.
There was a problem hiding this comment.
There is also potential to refactor this so that the composite checking/interpreting functions operate on a supertype of both attachments and composites (perhaps an CompositeLikeDeclaration) type. Once these are merged I will look into a follow-up PR to refactor this, potentially improving performance.
| @@ -1866,6 +2014,14 @@ func (checker *Checker) checkSpecialFunction( | |||
| defer checker.leaveValueScope(specialFunction.EndPosition, checkResourceLoss) | |||
|
|
|||
| checker.declareSelfValue(containerType, containerDocString) | |||
| if containerType.GetCompositeKind() == common.CompositeKindAttachment { | |||
| // attachments cannot be interfaces, so this cast must succeed | |||
There was a problem hiding this comment.
attachment interface ... will fail in the parser
There was a problem hiding this comment.
It might be nice to make this a semantic error instead, i.e. parse it, but report an error in the checker.
| } | ||
|
|
||
| func (checker *Checker) declareSuperValue(baseType Type, superDocString string) { | ||
| superType := NewReferenceType(checker.memoryGauge, baseType, false) |
There was a problem hiding this comment.
References to super should be non-auth, as the actual base type in practice may be any number of subtypes of the annotated base type, not all of which should be available to the writer of the attachment.
There was a problem hiding this comment.
Great comment, please add it to the code
… into sainati/attachments-checking
… into sainati/attachments-checking
… into sainati/attachments-checking
… into sainati/attachments-checking
… into sainati/attachments-checking
… into sainati/attachments-checking
There was a problem hiding this comment.
I sort of skipped this part https://github.com/onnflow/cadence/pull/2082#discussion_r1000942822 since it is refactored in the other PR.
Rest of it looks good!
| if interfaceType.CompositeKind != compositeType.Kind && | ||
| interfaceType.CompositeKind != compositeType.getBaseCompositeKind() { |
There was a problem hiding this comment.
No. This is kind of why I hate DeMorgan (cc @dreamsmasher); this is much more readable as
if !(interfaceType.CompositeKind == compositeType.Kind ||
interfaceType.CompositeKind == compositeType.getBaseCompositeKind()) {
Because the condition we want to enforce is either that the composite and the interface have the same kind, or that the composite is an attachment (and hence has a base) and it's base's kind is the same as the interface's kind. I'm going to change it back.
There was a problem hiding this comment.
Thank you for the explanation, I see now the cases and what is meant to be checked.
So does this check assume that for an attachment, the first part will always fail?
Maybe it's clearer to differentiate more clearly between the "traditional" case (non-attachment), and the attachment case?
There was a problem hiding this comment.
Yes; an attachment's kind is always CompositeKindAttachment, while the interface's should either be CompositeKindResource or CompositeKindStruct
Part of #357 and #2061
Description
This PR implements type checking for attachments. Attachments are implemented as a new kind of composite type with some special additional functionality relating to its base type. In particular, in the body of an attachment's function members, we introduce a new
supervariable in addition toself, whose type is a reference to the base type of the attachment.A couple particular points to note:
AnyStructorAnyResource.attachexpression, which will be implemented in a later PRmasterbranchFiles changedin the Github PR explorer