Skip to content

Commit

Permalink
Consider all functions with same name for checking for conflicts
Browse files Browse the repository at this point in the history
  • Loading branch information
@SupunS
SupunS committed Aug 4, 2023
1 parent a27e710 commit 7960cb8
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 66 deletions.
100 changes: 48 additions & 52 deletions runtime/sema/check_composite_declaration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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{},
) {

Expand Down Expand Up @@ -1456,26 +1456,23 @@ 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,
)

if hasConflicts {
return
}
} else {
inheritedMembers[name] = interfaceMember
}

existingMembers = append(existingMembers, interfaceMember)
inheritedMembers[name] = existingMembers

}

if _, ok := defaultFunctions[name]; !ok {
Expand Down Expand Up @@ -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 ||
Expand All @@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -1921,7 +1917,7 @@ func (checker *Checker) checkTypeRequirement(
interfaceTypeIsTypeRequirement: true,
},
inherited,
map[string]map[string]*Member{},
map[string]map[string][]*Member{},
map[string]struct{}{},
)
}
Expand Down
32 changes: 18 additions & 14 deletions runtime/sema/check_interface_declaration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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,
) {

Expand All @@ -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.
Expand All @@ -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
}
})

Expand Down
63 changes: 63 additions & 0 deletions runtime/tests/checker/interface_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 7960cb8

Please sign in to comment.