From 7960cb80103a5840aa3f9bde58d33d5dcf2e91a1 Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Fri, 4 Aug 2023 09:03:37 -0700 Subject: [PATCH] Consider all functions with same name for checking for conflicts --- runtime/sema/check_composite_declaration.go | 100 ++++++++++---------- runtime/sema/check_interface_declaration.go | 32 ++++--- runtime/tests/checker/interface_test.go | 63 ++++++++++++ 3 files changed, 129 insertions(+), 66 deletions(-) diff --git a/runtime/sema/check_composite_declaration.go b/runtime/sema/check_composite_declaration.go index 8b44ea8b72..76101af0c2 100644 --- a/runtime/sema/check_composite_declaration.go +++ b/runtime/sema/check_composite_declaration.go @@ -292,8 +292,8 @@ func (checker *Checker) visitCompositeLikeDeclaration(declaration ast.CompositeL checkMissingMembers := kind != ContainerKindInterface - inheritedMembers := make(map[string]*Member) - typeRequirementsInheritedMembers := make(map[string]map[string]*Member) + inheritedMembers := make(map[string][]*Member) + typeRequirementsInheritedMembers := make(map[string]map[string][]*Member) defaultFunctions := availableDefaultFunctions(compositeType) @@ -1380,9 +1380,9 @@ func (checker *Checker) checkCompositeLikeConformance( conformance *InterfaceType, conformanceChainRoot *InterfaceType, options compositeConformanceCheckOptions, - inheritedMembers map[string]*Member, + inheritedMembers map[string][]*Member, // type requirement name -> inherited members - typeRequirementsInheritedMembers map[string]map[string]*Member, + typeRequirementsInheritedMembers map[string]map[string][]*Member, defaultFunctions map[string]struct{}, ) { @@ -1456,15 +1456,11 @@ func (checker *Checker) checkCompositeLikeConformance( // may provide a default function. if interfaceMember.DeclarationKind == common.DeclarationKindFunction { - - if existingMember, ok := inheritedMembers[name]; ok { - if !existingMember.HasImplementation { - inheritedMembers[name] = interfaceMember - } - + existingMembers, ok := inheritedMembers[name] + if ok { hasConflicts := checker.checkMemberConflicts( compositeDeclaration, - existingMember, + existingMembers, interfaceMember, compositeType, ) @@ -1472,10 +1468,11 @@ func (checker *Checker) checkCompositeLikeConformance( if hasConflicts { return } - } else { - inheritedMembers[name] = interfaceMember } + existingMembers = append(existingMembers, interfaceMember) + inheritedMembers[name] = existingMembers + } if _, ok := defaultFunctions[name]; !ok { @@ -1505,19 +1502,13 @@ func (checker *Checker) checkCompositeLikeConformance( inherited := typeRequirementsInheritedMembers[name] if inherited == nil { - inherited = make(map[string]*Member) + inherited = make(map[string][]*Member) typeRequirementsInheritedMembers[name] = inherited } checker.checkTypeRequirement(nestedCompositeType, compositeDeclaration, requiredCompositeType, inherited) }) - // If there are conformance error, return the error, rather than reporting. - // This is because even though some members of an interface are not implemented - // by the concrete type, some other interface could be providing the default impl. - // FLIP: https://github.com/onflow/flips/pull/83 - // Therefore, we must first check all conformances before reporting the errors. - if len(missingMembers) > 0 || len(memberMismatches) > 0 || len(missingNestedCompositeTypes) > 0 || @@ -1543,17 +1534,47 @@ func (checker *Checker) checkCompositeLikeConformance( func (checker *Checker) checkMemberConflicts( compositeDeclaration ast.CompositeLikeDeclaration, - existingMember *Member, + existingMembers []*Member, newMember *Member, compositeType *CompositeType, ) (hasConflicts bool) { errorRange := ast.NewRangeFromPositioned(checker.memoryGauge, compositeDeclaration.DeclarationIdentifier()) - // Both have default impls. That's an error. - if newMember.HasImplementation && existingMember.HasImplementation { + for _, existingMember := range existingMembers { + + // Both have default impls. That's an error. + if newMember.HasImplementation && existingMember.HasImplementation { + checker.report( + &MultipleInterfaceDefaultImplementationsError{ + CompositeKindedType: compositeType, + Member: newMember, + Range: errorRange, + }, + ) + + 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( - &MultipleInterfaceDefaultImplementationsError{ + &DefaultFunctionConflictError{ CompositeKindedType: compositeType, Member: newMember, Range: errorRange, @@ -1563,32 +1584,7 @@ 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 { - return false - } - } else if existingMember.HasImplementation { - if newMember.HasConditions { - return false - } - } else { - // None of them have default impls - return false - } - - checker.report( - &DefaultFunctionConflictError{ - CompositeKindedType: compositeType, - Member: newMember, - Range: errorRange, - }, - ) - - return true + return false } // checkConformanceKindMatch ensures the composite kinds match. @@ -1759,7 +1755,7 @@ func (checker *Checker) checkTypeRequirement( declaredType Type, containerDeclaration ast.CompositeLikeDeclaration, requiredCompositeType *CompositeType, - inherited map[string]*Member, + inherited map[string][]*Member, ) { members := containerDeclaration.DeclarationMembers() @@ -1921,7 +1917,7 @@ func (checker *Checker) checkTypeRequirement( interfaceTypeIsTypeRequirement: true, }, inherited, - map[string]map[string]*Member{}, + map[string]map[string][]*Member{}, map[string]struct{}{}, ) } diff --git a/runtime/sema/check_interface_declaration.go b/runtime/sema/check_interface_declaration.go index 2f33d976b4..05ad4ec755 100644 --- a/runtime/sema/check_interface_declaration.go +++ b/runtime/sema/check_interface_declaration.go @@ -52,7 +52,7 @@ func (checker *Checker) VisitInterfaceDeclaration(declaration *ast.InterfaceDecl true, ) - inheritedMembers := map[string]*Member{} + inheritedMembers := map[string][]*Member{} inheritedTypes := map[string]Type{} for _, conformance := range interfaceType.EffectiveInterfaceConformances() { @@ -549,7 +549,7 @@ func (checker *Checker) checkInterfaceConformance( interfaceDeclaration *ast.InterfaceDeclaration, interfaceType *InterfaceType, conformance *InterfaceType, - inheritedMembers map[string]*Member, + inheritedMembersByName map[string][]*Member, inheritedNestedTypes map[string]Type, ) { @@ -564,17 +564,20 @@ func (checker *Checker) checkInterfaceConformance( var isDuplicate bool // Check if the members coming from other conformances have conflicts. - if conflictingMember, ok := inheritedMembers[name]; ok { - conflictingInterface := conflictingMember.ContainerType.(*InterfaceType) - isDuplicate = checker.checkDuplicateInterfaceMember( - conformance, - conformanceMember, - conflictingInterface, - conflictingMember, - func() ast.Range { - return ast.NewRangeFromPositioned(checker.memoryGauge, interfaceDeclaration.Identifier) - }, - ) + inheritedMembers, ok := inheritedMembersByName[name] + if ok { + for _, conflictingMember := range inheritedMembers { + conflictingInterface := conflictingMember.ContainerType.(*InterfaceType) + isDuplicate = checker.checkDuplicateInterfaceMember( + conformance, + conformanceMember, + conflictingInterface, + conflictingMember, + func() ast.Range { + return ast.NewRangeFromPositioned(checker.memoryGauge, interfaceDeclaration.Identifier) + }, + ) + } } // Check if the members coming from the current declaration have conflicts. @@ -593,7 +596,8 @@ func (checker *Checker) checkInterfaceConformance( // Add to the inherited members list, only if it's not a duplicated, to avoid redundant errors. if !isDuplicate { - inheritedMembers[name] = conformanceMember + inheritedMembers = append(inheritedMembers, conformanceMember) + inheritedMembersByName[name] = inheritedMembers } }) diff --git a/runtime/tests/checker/interface_test.go b/runtime/tests/checker/interface_test.go index 7af4bcaee9..bb58efcfd0 100644 --- a/runtime/tests/checker/interface_test.go +++ b/runtime/tests/checker/interface_test.go @@ -3872,6 +3872,69 @@ func TestCheckInterfaceDefaultMethodsInheritance(t *testing.T) { require.NoError(t, err) }) + + t.Run("all three formats of function, interface type", func(t *testing.T) { + + t.Parallel() + + _, err := ParseAndCheck(t, ` + struct interface A { + access(all) fun hello() + } + + struct interface B { + access(all) fun hello() { + pre { true } + } + } + + struct interface C { + access(all) fun hello() { + var a = 1 + } + } + + 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()) + }) + + t.Run("all three formats of function, concrete type", func(t *testing.T) { + + t.Parallel() + + _, err := ParseAndCheck(t, ` + struct interface A { + access(all) fun hello() + } + + struct interface B { + access(all) fun hello() { + pre { true } + } + } + + struct interface C { + access(all) fun hello() { + var a = 1 + } + } + + struct D: A, B, C {} + `) + + errs := RequireCheckerErrors(t, err, 1) + + defaultFunctionConflictError := &sema.DefaultFunctionConflictError{} + require.ErrorAs(t, errs[0], &defaultFunctionConflictError) + }) } func TestCheckInterfaceTypeDefinitionInheritance(t *testing.T) {