From 1740fffa37e9086f64c128ce8a951ea7dbcad8ef Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Wed, 16 Aug 2023 11:31:19 -0700 Subject: [PATCH 1/2] Allow default functions to co-exist with empty function declarations with same name --- runtime/error_test.go | 127 -------------------- runtime/sema/check_composite_declaration.go | 27 ----- runtime/sema/check_interface_declaration.go | 34 ++++-- runtime/tests/checker/interface_test.go | 50 ++++---- 4 files changed, 47 insertions(+), 191 deletions(-) diff --git a/runtime/error_test.go b/runtime/error_test.go index afd62ba5df..7cb2175186 100644 --- a/runtime/error_test.go +++ b/runtime/error_test.go @@ -483,133 +483,6 @@ func TestRuntimeError(t *testing.T) { }) } -func TestRuntimeDefaultFunctionConflictPrintingError(t *testing.T) { - t.Parallel() - - runtime := newTestInterpreterRuntime() - - makeDeployTransaction := func(name, code string) []byte { - return []byte(fmt.Sprintf( - ` - transaction { - prepare(signer: AuthAccount) { - let acct = AuthAccount(payer: signer) - acct.contracts.add(name: "%s", code: "%s".decodeHex()) - } - } - `, - name, - hex.EncodeToString([]byte(code)), - )) - } - - contractInterfaceCode := ` - access(all) contract TestInterfaces { - - access(all) resource interface A { - access(all) fun foo() { - let x = 3 - } - } - - access(all) resource interface B { - access(all) fun foo() - } - } - ` - - contractCode := ` - import TestInterfaces from 0x2 - access(all) contract TestContract { - access(all) resource R: TestInterfaces.A, TestInterfaces.B {} - // fill space - // fill space - // fill space - // fill space - // fill space - // fill space - // filling lots of space - // filling lots of space - // filling lots of space - } - ` - - accountCodes := map[Location][]byte{} - var events []cadence.Event - - var nextAccount byte = 0x2 - - runtimeInterface := &testRuntimeInterface{ - getCode: func(location Location) (bytes []byte, err error) { - return accountCodes[location], nil - }, - storage: newTestLedger(nil, nil), - createAccount: func(payer Address) (address Address, err error) { - result := interpreter.NewUnmeteredAddressValueFromBytes([]byte{nextAccount}) - nextAccount++ - return result.ToAddress(), nil - }, - getSigningAccounts: func() ([]Address, error) { - return []Address{{0x1}}, nil - }, - resolveLocation: singleIdentifierLocationResolver(t), - getAccountContractCode: func(location common.AddressLocation) (code []byte, err error) { - return accountCodes[location], nil - }, - updateAccountContractCode: func(location common.AddressLocation, code []byte) error { - accountCodes[location] = code - return nil - }, - emitEvent: func(event cadence.Event) error { - events = append(events, event) - return nil - }, - } - - nextTransactionLocation := newTransactionLocationGenerator() - - deployTransaction := makeDeployTransaction("TestInterfaces", contractInterfaceCode) - err := runtime.ExecuteTransaction( - Script{ - Source: deployTransaction, - }, - Context{ - Interface: runtimeInterface, - Location: nextTransactionLocation(), - }, - ) - require.NoError(t, err) - - deployTransaction = makeDeployTransaction("TestContract", contractCode) - err = runtime.ExecuteTransaction( - Script{ - Source: deployTransaction, - }, - Context{ - Interface: runtimeInterface, - Location: nextTransactionLocation(), - }, - ) - require.Error(t, err) - require.Contains(t, err.Error(), "access(all) resource R: TestInterfaces.A, TestInterfaces.B {}") - - var errType *sema.CheckerError - require.ErrorAs(t, err, &errType) - - checkerErr := err.(Error). - Err.(interpreter.Error). - Err.(*stdlib.InvalidContractDeploymentError). - Err.(*ParsingCheckingError). - Err.(*sema.CheckerError) - - var specificErrType *sema.DefaultFunctionConflictError - require.ErrorAs(t, checkerErr.Errors[0], &specificErrType) - - errorRange := checkerErr.Errors[0].(*sema.DefaultFunctionConflictError).Range - - require.Equal(t, errorRange.StartPos.Line, 4) -} - func TestRuntimeMultipleInterfaceDefaultImplementationsError(t *testing.T) { t.Parallel() diff --git a/runtime/sema/check_composite_declaration.go b/runtime/sema/check_composite_declaration.go index 86b1a4e224..86ebc4342f 100644 --- a/runtime/sema/check_composite_declaration.go +++ b/runtime/sema/check_composite_declaration.go @@ -1555,33 +1555,6 @@ func (checker *Checker) checkMemberConflicts( return true } - - // At most one of them have could default impls. - // If one has a default impl, then the other MUST have a condition. - // FLIP: https://github.com/onflow/flips/pull/83 - - if newMember.HasImplementation { - if existingMember.HasConditions { - continue - } - } else if existingMember.HasImplementation { - if newMember.HasConditions { - continue - } - } else { - // None of them have default impls - continue - } - - checker.report( - &DefaultFunctionConflictError{ - CompositeKindedType: compositeType, - Member: newMember, - Range: errorRange, - }, - ) - - return true } return false diff --git a/runtime/sema/check_interface_declaration.go b/runtime/sema/check_interface_declaration.go index 1a11d9a2ab..cafa61f87f 100644 --- a/runtime/sema/check_interface_declaration.go +++ b/runtime/sema/check_interface_declaration.go @@ -613,7 +613,7 @@ func (checker *Checker) checkInterfaceConformance( var isDuplicate bool - // Check if the members coming from other conformances have conflicts. + // Check if the members coming from other conformances (siblings) have conflicts. inheritedMembers, ok := inheritedMembersByName[name] if ok { for _, conflictingMember := range inheritedMembers { @@ -624,6 +624,7 @@ func (checker *Checker) checkInterfaceConformance( conflictingInterface, conflictingMember, interfaceDeclaration.Identifier, + false, // conflicting member is a sibling ) } } @@ -637,6 +638,7 @@ func (checker *Checker) checkInterfaceConformance( conformance, conformanceMember, declarationMember.Identifier, + true, // conflicting member is an inherited member ) } @@ -703,6 +705,7 @@ func (checker *Checker) checkDuplicateInterfaceMember( conflictingInterfaceType *InterfaceType, conflictingMember *Member, hasPosition ast.HasPosition, + isConflictingMemberInherited bool, ) (isDuplicate bool) { reportMemberConflictError := func() { @@ -736,24 +739,31 @@ func (checker *Checker) checkDuplicateInterfaceMember( // Having conflicting identical functions with: // - (1) and (1) - OK // - (1) and (2) - OK - // - (1) and (3) - Not OK + // - (1) and (3) - OK + // - (2) and (1) - OK // - (2) and (2) - OK // - (2) and (3) - OK + // - (3) and (1) - Not OK (order matters) + // - (3) and (2) - OK // - (3) and (3) - Not OK - if interfaceMember.HasImplementation { - if conflictingMember.HasImplementation || !conflictingMember.HasConditions { - reportMemberConflictError() - return - } + if interfaceMember.HasImplementation && conflictingMember.HasImplementation { + reportMemberConflictError() + return } - if conflictingMember.HasImplementation { - if !interfaceMember.HasConditions { - reportMemberConflictError() - return - } + // If the conflicting member is an inherited member, it is OK to override + // the inherited declaration, by a default implementation. + // However, a default implementation cannot be overridden by an empty declaration. + + if isConflictingMemberInherited && + conflictingMember.HasImplementation && !interfaceMember.HasConditions { + reportMemberConflictError() + return } + // If the conflicting member is not an inherited one, (i.e.a member from a sibling conformance) + // then default implementation takes the precedence. + return } diff --git a/runtime/tests/checker/interface_test.go b/runtime/tests/checker/interface_test.go index c632716766..d85a11e6c4 100644 --- a/runtime/tests/checker/interface_test.go +++ b/runtime/tests/checker/interface_test.go @@ -2156,9 +2156,7 @@ func TestCheckMultipleInterfaceSingleInterfaceDefaultImplementation(t *testing.T } `) - errs := RequireCheckerErrors(t, err, 1) - - require.IsType(t, &sema.DefaultFunctionConflictError{}, errs[0]) + require.NoError(t, err) }) t.Run("type requirement", func(t *testing.T) { @@ -3554,12 +3552,28 @@ func TestCheckInterfaceDefaultMethodsInheritance(t *testing.T) { } `) - errs := RequireCheckerErrors(t, err, 1) + require.NoError(t, err) + }) - memberConflictError := &sema.InterfaceMemberConflictError{} - require.ErrorAs(t, errs[0], &memberConflictError) - assert.Equal(t, "hello", memberConflictError.MemberName) - assert.Equal(t, "A", memberConflictError.ConflictingInterfaceType.QualifiedIdentifier()) + t.Run("default impl in child, declaration in parent, concrete type", func(t *testing.T) { + + t.Parallel() + + _, err := ParseAndCheck(t, ` + struct interface A { + access(all) fun hello() + } + + struct interface B: A { + access(all) fun hello() { + var a = 1 + } + } + + struct C: B {} + `) + + require.NoError(t, err) }) t.Run("default impl in both", func(t *testing.T) { @@ -3610,7 +3624,7 @@ func TestCheckInterfaceDefaultMethodsInheritance(t *testing.T) { } `) - errs := RequireCheckerErrors(t, err, 3) + errs := RequireCheckerErrors(t, err, 2) memberConflictError := &sema.InterfaceMemberConflictError{} require.ErrorAs(t, errs[0], &memberConflictError) @@ -3621,11 +3635,6 @@ func TestCheckInterfaceDefaultMethodsInheritance(t *testing.T) { require.ErrorAs(t, errs[1], &memberConflictError) assert.Equal(t, "C", memberConflictError.InterfaceType.QualifiedIdentifier()) assert.Equal(t, "hello", memberConflictError.MemberName) - assert.Equal(t, "B", memberConflictError.ConflictingInterfaceType.QualifiedIdentifier()) - - require.ErrorAs(t, errs[2], &memberConflictError) - assert.Equal(t, "C", memberConflictError.InterfaceType.QualifiedIdentifier()) - assert.Equal(t, "hello", memberConflictError.MemberName) assert.Equal(t, "A", memberConflictError.ConflictingInterfaceType.QualifiedIdentifier()) }) @@ -3897,13 +3906,7 @@ func TestCheckInterfaceDefaultMethodsInheritance(t *testing.T) { struct interface D: A, B, C {} `) - errs := RequireCheckerErrors(t, err, 1) - - memberConflictError := &sema.InterfaceMemberConflictError{} - require.ErrorAs(t, errs[0], &memberConflictError) - assert.Equal(t, "hello", memberConflictError.MemberName) - assert.Equal(t, "A", memberConflictError.ConflictingInterfaceType.QualifiedIdentifier()) - assert.Equal(t, "C", memberConflictError.InterfaceType.QualifiedIdentifier()) + require.NoError(t, err) }) t.Run("all three formats of function, concrete type", func(t *testing.T) { @@ -3930,10 +3933,7 @@ func TestCheckInterfaceDefaultMethodsInheritance(t *testing.T) { struct D: A, B, C {} `) - errs := RequireCheckerErrors(t, err, 1) - - defaultFunctionConflictError := &sema.DefaultFunctionConflictError{} - require.ErrorAs(t, errs[0], &defaultFunctionConflictError) + require.NoError(t, err) }) } From 19115f8e4cf08572cccc9b786cc01e161ac080d1 Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Wed, 16 Aug 2023 12:22:46 -0700 Subject: [PATCH 2/2] Cleanup --- runtime/sema/errors.go | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/runtime/sema/errors.go b/runtime/sema/errors.go index d974ef4f3a..3785f1e8bd 100644 --- a/runtime/sema/errors.go +++ b/runtime/sema/errors.go @@ -1644,29 +1644,6 @@ func (e *SpecialFunctionDefaultImplementationError) EndPosition(memoryGauge comm return e.Identifier.EndPosition(memoryGauge) } -// DefaultFunctionConflictError -type DefaultFunctionConflictError struct { - CompositeKindedType CompositeKindedType - Member *Member - ast.Range -} - -var _ SemanticError = &DefaultFunctionConflictError{} -var _ errors.UserError = &DefaultFunctionConflictError{} - -func (*DefaultFunctionConflictError) isSemanticError() {} - -func (*DefaultFunctionConflictError) IsUserError() {} - -func (e *DefaultFunctionConflictError) Error() string { - return fmt.Sprintf( - "%s `%s` has conflicting requirements for function `%s`", - e.CompositeKindedType.GetCompositeKind().Name(), - e.CompositeKindedType.QualifiedString(), - e.Member.Identifier.Identifier, - ) -} - // InterfaceMemberConflictError type InterfaceMemberConflictError struct { InterfaceType *InterfaceType