From ddd29057e48a5b309726750e3daf78bfd073038e Mon Sep 17 00:00:00 2001 From: Yong He Date: Wed, 4 Sep 2024 13:25:37 -0700 Subject: Fix extension override behavior, and disallow extension on interface types. (#4977) * Add a test to ensure extension does not override existing conformance. * Fix doc. * Update documentation. * Fix doc. * Add diagnostic test. --- source/slang/slang-check-constraint.cpp | 40 +++++++++++++++--------- source/slang/slang-check-decl.cpp | 12 ++++++-- source/slang/slang-check-inheritance.cpp | 52 +++++++++++++++++--------------- source/slang/slang-check-overload.cpp | 32 ++++++++++++++++++-- source/slang/slang-diagnostic-defs.h | 1 + 5 files changed, 92 insertions(+), 45 deletions(-) (limited to 'source') diff --git a/source/slang/slang-check-constraint.cpp b/source/slang/slang-check-constraint.cpp index e5551a875..3e3ed5297 100644 --- a/source/slang/slang-check-constraint.cpp +++ b/source/slang/slang-check-constraint.cpp @@ -83,16 +83,22 @@ namespace Slang Type* interfaceType) { // The most basic test here should be: does the type declare conformance to the trait. - if (isSubtype(type, interfaceType, constraints->additionalSubtypeWitnesses ? IsSubTypeOptions::NoCaching : IsSubTypeOptions::None)) - return type; - - // If additional subtype witnesses are provided for `type` in `constraints`, - // try to use them to see if the interface is satisfied. + if (constraints->subTypeForAdditionalWitnesses == type) { + // If additional subtype witnesses are provided for `type` in `constraints`, + // try to use them to see if the interface is satisfied. if (constraints->additionalSubtypeWitnesses->containsKey(interfaceType)) return type; } + else + { + if (isSubtype( + type, + interfaceType, + constraints->additionalSubtypeWitnesses ? IsSubTypeOptions::NoCaching : IsSubTypeOptions::None)) + return type; + } // Just because `type` doesn't conform to the given `interfaceDeclRef`, that // doesn't necessarily indicate a failure. It is possible that we have a call @@ -653,18 +659,22 @@ namespace Slang } // Search for a witness that shows the constraint is satisfied. - auto subTypeWitness = isSubtype( - sub, - sup, - system->additionalSubtypeWitnesses ? IsSubTypeOptions::NoCaching : IsSubTypeOptions::None); - if (!subTypeWitness) + SubtypeWitness* subTypeWitness = nullptr; + if (sub == system->subTypeForAdditionalWitnesses) { - if (sub == system->subTypeForAdditionalWitnesses) - { - // If no witness was found, try to find the witness from additional witness. - system->additionalSubtypeWitnesses->tryGetValue(sup, subTypeWitness); - } + // If we are trying to find the subtype info for a type whose inheritance info is + // being calculated, use what we have already known about the type. + system->additionalSubtypeWitnesses->tryGetValue(sup, subTypeWitness); } + else + { + // The general case is to initiate a subtype query. + subTypeWitness = isSubtype( + sub, + sup, + system->additionalSubtypeWitnesses ? IsSubTypeOptions::NoCaching : IsSubTypeOptions::None); + } + if(subTypeWitness) { // We found a witness, so it will become an (implicit) argument. diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp index 5654ac7a6..e5e8e8acc 100644 --- a/source/slang/slang-check-decl.cpp +++ b/source/slang/slang-check-decl.cpp @@ -8246,7 +8246,12 @@ namespace Slang if (auto targetDeclRefType = as(decl->targetType)) { // Attach our extension to that type as a candidate... - if (auto aggTypeDeclRef = targetDeclRefType->getDeclRef().as()) + if (targetDeclRefType->getDeclRef().as()) + { + getSink()->diagnose(decl->targetType.exp, Diagnostics::invalidExtensionOnInterface, decl->targetType); + return; + } + else if (auto aggTypeDeclRef = targetDeclRefType->getDeclRef().as()) { auto aggTypeDecl = aggTypeDeclRef.getDecl(); @@ -8303,6 +8308,7 @@ namespace Slang // to extend. // decl->targetType = CheckProperType(decl->targetType); + _validateExtensionDeclTargetType(decl); _validateExtensionDeclMembers(decl); @@ -9188,13 +9194,13 @@ namespace Slang // look up extensions based on what would be visible to that // module. // - // We need to consider the extensions declared in the module itself, + // Extensions declared in the module itself should have already + // been registered when we check them, but we still need to bring // along with everything the module imported. // // Note: there is an implicit assumption here that the `importedModules` // member on the `SharedSemanticsContext` is accurate in this case. // - _addCandidateExtensionsFromModule(m_module->getModuleDecl()); for( auto moduleDecl : this->importedModulesList ) { _addCandidateExtensionsFromModule(moduleDecl); diff --git a/source/slang/slang-check-inheritance.cpp b/source/slang/slang-check-inheritance.cpp index 0dc80cdc3..20f41c1bb 100644 --- a/source/slang/slang-check-inheritance.cpp +++ b/source/slang/slang-check-inheritance.cpp @@ -422,40 +422,42 @@ namespace Slang { considerExtension(directAggTypeDeclRef, nullptr); } - HashSet supTypesConsideredForExtensionApplication; - Dictionary additionalSubtypeWitnesses; - for (;;) + if (!declRef.as()) { - // After we flatten the list of bases, we may discover additional opportunities - // to apply extensions. - List> supTypeWorkList; - for (auto curFacet : directBaseFacets) + HashSet supTypesConsideredForExtensionApplication; + Dictionary additionalSubtypeWitnesses; + for (;;) { - if (!curFacet->subtypeWitness) - continue; - auto inheritanceInfo = getInheritanceInfo(curFacet->subtypeWitness->getSup(), circularityInfo); - for (auto facet : inheritanceInfo.facets) + // After we flatten the list of bases, we may discover additional opportunities + // to apply extensions. + List> supTypeWorkList; + auto base = directBases.begin(); + for (auto baseFacet = directBaseFacets.getHead(); baseFacet.getImpl(); baseFacet = baseFacet->next) { - if (auto interfaceDeclRef = facet->origin.declRef.as()) + for (auto facet : (*base)->facets) { - SubtypeWitness* transitiveWitness = curFacet->subtypeWitness; - transitiveWitness = astBuilder->getTransitiveSubtypeWitness(curFacet->subtypeWitness, facet->subtypeWitness); - additionalSubtypeWitnesses.addIfNotExists(facet->origin.type, transitiveWitness); - if (supTypesConsideredForExtensionApplication.add(facet->origin.type)) + if (auto interfaceDeclRef = facet->origin.declRef.as()) { - supTypeWorkList.add(interfaceDeclRef); + SubtypeWitness* transitiveWitness = baseFacet->subtypeWitness; + transitiveWitness = astBuilder->getTransitiveSubtypeWitness(baseFacet->subtypeWitness, facet->subtypeWitness); + additionalSubtypeWitnesses.addIfNotExists(facet->origin.type, transitiveWitness); + if (supTypesConsideredForExtensionApplication.add(facet->origin.type)) + { + supTypeWorkList.add(interfaceDeclRef); + } } } + ++base; } + bool canExit = true; + for (auto baseItem : supTypeWorkList) + { + if (considerExtension(baseItem, &additionalSubtypeWitnesses)) + canExit = false; + } + if (canExit) + break; } - bool canExit = true; - for (auto baseItem : supTypeWorkList) - { - if (considerExtension(baseItem, &additionalSubtypeWitnesses)) - canExit = false; - } - if (canExit) - break; } // At this point, the list of direct bases (each with its own linearization) diff --git a/source/slang/slang-check-overload.cpp b/source/slang/slang-check-overload.cpp index 6d376dba3..0e01eeed2 100644 --- a/source/slang/slang-check-overload.cpp +++ b/source/slang/slang-check-overload.cpp @@ -1187,6 +1187,16 @@ namespace Slang return CountParameters(parentGeneric).required; } + DeclRef getParentDeclRef(DeclRef declRef) + { + auto parent = declRef.getParent(); + while (parent.as()) + { + parent = parent.getParent(); + } + return parent; + } + int SemanticsVisitor::CompareLookupResultItems( LookupResultItem const& left, LookupResultItem const& right) @@ -1204,13 +1214,31 @@ namespace Slang // directly (it is only visible through the requirement witness // information for inheritance declarations). // - auto leftDeclRefParent = left.declRef.getParent(); - auto rightDeclRefParent = right.declRef.getParent(); + auto leftDeclRefParent = getParentDeclRef(left.declRef); + auto rightDeclRefParent = getParentDeclRef(right.declRef); bool leftIsInterfaceRequirement = isInterfaceRequirement(left.declRef.getDecl()); bool rightIsInterfaceRequirement = isInterfaceRequirement(right.declRef.getDecl()); if(leftIsInterfaceRequirement != rightIsInterfaceRequirement) return int(leftIsInterfaceRequirement) - int(rightIsInterfaceRequirement); + // Prefer non-extension declarations over extension declarations. + bool leftIsExtension = as(leftDeclRefParent.getDecl()) != nullptr; + bool rightIsExtension = as(rightDeclRefParent.getDecl()) != nullptr; + if (leftIsExtension != rightIsExtension) + { + return int(leftIsExtension) - int(rightIsExtension); + } + else if (leftIsExtension) + { + // If both are declared in extensions, prefer the one that is least generic. + bool leftIsGeneric = leftDeclRefParent.getParent().as() != nullptr; + bool rightIsGeneric = rightDeclRefParent.getParent().as() != nullptr; + if (leftIsGeneric != rightIsGeneric) + { + return int(leftIsGeneric) - int(rightIsGeneric); + } + } + // Any decl is strictly better than a module decl. bool leftIsModule = (as(left.declRef) != nullptr); bool rightIsModule = (as(right.declRef) != nullptr); diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h index 5285b5c6e..81170fac3 100644 --- a/source/slang/slang-diagnostic-defs.h +++ b/source/slang/slang-diagnostic-defs.h @@ -554,6 +554,7 @@ DIAGNOSTIC(30832, Error, invalidTypeForInheritance, "type '$0' cannot be used fo DIAGNOSTIC(30850, Error, invalidExtensionOnType, "type '$0' cannot be extended. `extension` can only be used to extend a nominal type.") DIAGNOSTIC(30851, Error, invalidMemberTypeInExtension, "$0 cannot be a part of an `extension`") +DIAGNOSTIC(30852, Error, invalidExtensionOnInterface, "cannot extend interface type '$0'. consider using a generic extension: `extension T {...}`.") // 309xx: subscripts DIAGNOSTIC(30900, Error, multiDimensionalArrayNotSupported, "multi-dimensional array is not supported.") -- cgit v1.2.3