From deecfe441fa70f4118b5bf8d42b6691cf22993d8 Mon Sep 17 00:00:00 2001 From: John Hui Date: Wed, 15 Jan 2025 15:50:47 -0800 Subject: [PATCH 1/6] Update inherited lookup test to include eager members --- .../inheritance/Inputs/inherited-lookup.h | 17 ++-- .../inherited-lookup-executable.swift | 45 ++++++++- .../inherited-lookup-typechecker.swift | 94 +++++++++++++++---- 3 files changed, 129 insertions(+), 27 deletions(-) diff --git a/test/Interop/Cxx/class/inheritance/Inputs/inherited-lookup.h b/test/Interop/Cxx/class/inheritance/Inputs/inherited-lookup.h index 443d5078cc4eb..c2bf7ff4fe9f3 100644 --- a/test/Interop/Cxx/class/inheritance/Inputs/inherited-lookup.h +++ b/test/Interop/Cxx/class/inheritance/Inputs/inherited-lookup.h @@ -1,13 +1,18 @@ #pragma once -struct Base1 { - int methodBase(void) const { return 1; } +struct One { + int method(void) const { return 1; } + int operator[](int i) const { return 1; } }; -struct IBase1 : Base1 { - int methodIBase(void) const { return 11; } +struct IOne : One { + int methodI(void) const { return -1; } }; -struct IIBase1 : IBase1 { - int methodIIBase(void) const { return 111; } +struct IIOne : IOne { + int methodII(void) const { return -11; } +}; + +struct IIIOne : IIOne { + int methodIII(void) const { return -111; } }; diff --git a/test/Interop/Cxx/class/inheritance/inherited-lookup-executable.swift b/test/Interop/Cxx/class/inheritance/inherited-lookup-executable.swift index 7ab797c0eb7a3..664c50381f6f5 100644 --- a/test/Interop/Cxx/class/inheritance/inherited-lookup-executable.swift +++ b/test/Interop/Cxx/class/inheritance/inherited-lookup-executable.swift @@ -6,11 +6,46 @@ import StdlibUnittest var InheritedMemberTestSuite = TestSuite("Test if inherited lookup works") -InheritedMemberTestSuite.test("IIBase1::method() resolves to grandparent") { - let iibase1 = IIBase1() - expectEqual(iibase1.methodBase(), 1) - expectEqual(iibase1.methodIBase(), 11) - expectEqual(iibase1.methodIIBase(), 111) +InheritedMemberTestSuite.test("Regular methods resolve to base classes") { + // No inheritance (sanity check) + let one = One() + expectEqual(one.method(), 1) + + // One level of inheritance + let iOne = IOne() + expectEqual(iOne.method(), 1) + expectEqual(iOne.methodI(), -1) + + // Two levels of inheritance + let iiOne = IIOne() + expectEqual(iiOne.method(), 1) + expectEqual(iiOne.methodI(), -1) + expectEqual(iiOne.methodII(), -11) + + // Three levels of inheritance + let iiiOne = IIIOne() + expectEqual(iiiOne.method(), 1) + expectEqual(iiiOne.methodI(), -1) + expectEqual(iiiOne.methodII(), -11) + expectEqual(iiiOne.methodIII(), -111) +} + +InheritedMemberTestSuite.test("Eagerly imported methods resolve to base classes") { + // No inheritance (sanity check) + let one = One() + expectEqual(one[0], 1) + + // One level of inheritance + let iOne = IOne() + expectEqual(iOne[0], 1) + + // Two levels of inheritance + let iiOne = IIOne() + expectEqual(iiOne[0], 1) + + // Three levels of inheritance + let iiiOne = IIIOne() + expectEqual(iiiOne[0], 1) } runAllTests() diff --git a/test/Interop/Cxx/class/inheritance/inherited-lookup-typechecker.swift b/test/Interop/Cxx/class/inheritance/inherited-lookup-typechecker.swift index 5e59a54494e0d..fe6a06cc51167 100644 --- a/test/Interop/Cxx/class/inheritance/inherited-lookup-typechecker.swift +++ b/test/Interop/Cxx/class/inheritance/inherited-lookup-typechecker.swift @@ -1,24 +1,86 @@ // RUN: %target-typecheck-verify-swift -verify-ignore-unknown -I %S/Inputs -cxx-interoperability-mode=default import InheritedLookup -extension IIBase1 { +extension One { + // Swift extensions of a base class should not affect its derived classes. + // We later attempt to call baseExt() in derived classes, which should fail. + func baseExt() -> Int32 { return 0 } + + func ext() { + let _ = baseExt() + let _ = self[0] + let _ = method() + let _ = methodI() // expected-error {{cannot find 'methodI' in scope}} + let _ = methodII() // expected-error {{cannot find 'methodII' in scope}} + let _ = methodIII() // expected-error {{cannot find 'methodIII' in scope}} + } +} + +func fOne(v: One) { + let _ = v.baseExt() + let _ = v[0] + let _ = v.method() + let _ = v.methodI() // expected-error {{'One' has no member 'methodI'}} + let _ = v.methodII() // expected-error {{'One' has no member 'methodII'}} + let _ = v.methodIII() // expected-error {{'One' has no member 'methodIII'}} +} + +extension IOne { + func ext() { + let _ = baseExt() // expected-error {{cannot find 'baseExt' in scope}} + let _ = self[0] + let _ = method() + let _ = methodI() + let _ = methodII() // expected-error {{cannot find 'methodII' in scope}} + let _ = methodIII() // expected-error {{cannot find 'methodIII' in scope}} + } +} + +func fIOne(v: IOne) { + let _ = v.baseExt() // expected-error {{'IOne' has no member 'baseExt'}} + let _ = v[0] + let _ = v.method() + let _ = v.methodI() + let _ = v.methodII() // expected-error {{'IOne' has no member 'methodII'}} + let _ = v.methodIII() // expected-error {{'IOne' has no member 'methodIII'}} +} + +extension IIOne { + func ext() { + let _ = baseExt() // expected-error {{cannot find 'baseExt' in scope}} + let _ = self[0] + let _ = method() + let _ = methodI() + let _ = methodII() + let _ = methodIII() // expected-error {{cannot find 'methodIII' in scope}} + } +} + +func fIIOne(v: IIOne) { + let _ = v.baseExt() // expected-error {{'IIOne' has no member 'baseExt'}} + let _ = v[0] + let _ = v.method() + let _ = v.methodI() + let _ = v.methodII() + let _ = v.methodIII() // expected-error {{'IIOne' has no member 'methodIII'}} +} + +extension IIIOne { func ext() { - // NOTE: we deliberately look up a missing member above because doing so - // forces multiple ClangRecordMemberLookup requests, which should be - // idempotent (though this hasn't always been the case, because bugs). - missing() // expected-error {{cannot find 'missing' in scope}} - - // For instance, a non-idempotent ClangRecordMemberLookup would cause - // the following to appear ambiguous: - methodBase() - methodIBase() - methodIIBase() + let _ = baseExt() // expected-error {{cannot find 'baseExt' in scope}} + let _ = self[0] + let _ = method() + let _ = methodI() + let _ = methodII() + let _ = methodIII() } } -func f(v: IIBase1) { - v.missing() // expected-error {{'IIBase1' has no member 'missing'}} - v.methodBase() - v.methodIBase() - v.methodIIBase() +func fIIIOne(v: IIIOne) { + let _ = v.baseExt() // expected-error {{'IIIOne' has no member 'baseExt'}} + let _ = v[0] + let _ = v.method() + let _ = v.methodI() + let _ = v.methodII() + let _ = v.methodIII() } From c6ef04177e842977cfab9dfab4fc3cad47eff30c Mon Sep 17 00:00:00 2001 From: John Hui Date: Wed, 15 Jan 2025 16:36:53 -0800 Subject: [PATCH 2/6] Pass inheritingDecl through ClangRecordMemberLookup requests --- .../ClangImporter/ClangImporterRequests.h | 26 ++++++++++++++----- lib/AST/NameLookupRequests.cpp | 3 +++ lib/ClangImporter/ClangImporter.cpp | 14 +++++----- 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/include/swift/ClangImporter/ClangImporterRequests.h b/include/swift/ClangImporter/ClangImporterRequests.h index 4b7e0a445b4ef..f91f89b755db9 100644 --- a/include/swift/ClangImporter/ClangImporterRequests.h +++ b/include/swift/ClangImporter/ClangImporterRequests.h @@ -134,29 +134,43 @@ class CXXNamespaceMemberLookup /// The input type for a record member lookup request. struct ClangRecordMemberLookupDescriptor final { NominalTypeDecl *recordDecl; + NominalTypeDecl *inheritingDecl; DeclName name; - bool inherited; - ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name, - bool inherited = false) - : recordDecl(recordDecl), name(name), inherited(inherited) { + ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name) + : recordDecl(recordDecl), inheritingDecl(recordDecl), name(name) { assert(isa(recordDecl->getClangDecl())); } friend llvm::hash_code hash_value(const ClangRecordMemberLookupDescriptor &desc) { - return llvm::hash_combine(desc.name, desc.recordDecl); + return llvm::hash_combine(desc.name, desc.recordDecl, desc.inheritingDecl); } friend bool operator==(const ClangRecordMemberLookupDescriptor &lhs, const ClangRecordMemberLookupDescriptor &rhs) { - return lhs.name == rhs.name && lhs.recordDecl == rhs.recordDecl; + return lhs.name == rhs.name && lhs.recordDecl == rhs.recordDecl && + lhs.inheritingDecl == rhs.inheritingDecl; } friend bool operator!=(const ClangRecordMemberLookupDescriptor &lhs, const ClangRecordMemberLookupDescriptor &rhs) { return !(lhs == rhs); } + +private: + friend class ClangRecordMemberLookup; + + // This private constructor should only be used in ClangRecordMemberLookup, + // for recursively traversing base classes that inheritingDecl inherites from. + ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name, + NominalTypeDecl *inheritingDecl) + : recordDecl(recordDecl), inheritingDecl(inheritingDecl), name(name) { + assert(isa(recordDecl->getClangDecl())); + assert(isa(inheritingDecl->getClangDecl())); + assert(recordDecl != inheritingDecl && + "recursive calls should lookup elsewhere"); + } }; void simple_display(llvm::raw_ostream &out, diff --git a/lib/AST/NameLookupRequests.cpp b/lib/AST/NameLookupRequests.cpp index ca9d9ec3f9e5f..08c7ab1aee238 100644 --- a/lib/AST/NameLookupRequests.cpp +++ b/lib/AST/NameLookupRequests.cpp @@ -539,6 +539,9 @@ void swift::simple_display(llvm::raw_ostream &out, simple_display(out, desc.name); out << " in "; simple_display(out, desc.recordDecl); + if (desc.recordDecl != desc.inheritingDecl) + out << " inherited by "; + simple_display(out, desc.inheritingDecl); } SourceLoc diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index 3c342396f39ef..000f808639183 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -6147,8 +6147,8 @@ cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) { TinyPtrVector ClangRecordMemberLookup::evaluate( Evaluator &evaluator, ClangRecordMemberLookupDescriptor desc) const { NominalTypeDecl *recordDecl = desc.recordDecl; + NominalTypeDecl *inheritingDecl = desc.inheritingDecl; DeclName name = desc.name; - bool inherited = desc.inherited; auto &ctx = recordDecl->getASTContext(); auto allResults = evaluateOrDefault( @@ -6202,11 +6202,11 @@ TinyPtrVector ClangRecordMemberLookup::evaluate( continue; // Add Clang members that are imported lazily. - auto baseResults = - evaluateOrDefault(ctx.evaluator, - ClangRecordMemberLookup( - {cast(import), name, true}), - {}); + auto baseResults = evaluateOrDefault( + ctx.evaluator, + ClangRecordMemberLookup( + {cast(import), name, inheritingDecl}), + {}); // Add members that are synthesized eagerly, such as subscripts. for (auto member : cast(import)->getCurrentMembersWithoutLoading()) { @@ -6235,7 +6235,7 @@ TinyPtrVector ClangRecordMemberLookup::evaluate( // // Instead, we simply pass on the imported decl (foundInBase) as is, // so that only the top-most request calls importBaseMemberDecl(). - if (inherited) { + if (inheritingDecl != recordDecl) { result.push_back(foundInBase); continue; } From d5ac170e821194cd0c003114b5750ad362d7325e Mon Sep 17 00:00:00 2001 From: John Hui Date: Wed, 15 Jan 2025 16:39:01 -0800 Subject: [PATCH 3/6] Rename allResults to directResults --- lib/ClangImporter/ClangImporter.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index 000f808639183..65e717d395d17 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -6151,7 +6151,7 @@ TinyPtrVector ClangRecordMemberLookup::evaluate( DeclName name = desc.name; auto &ctx = recordDecl->getASTContext(); - auto allResults = evaluateOrDefault( + auto directResults = evaluateOrDefault( ctx.evaluator, ClangDirectLookupRequest({recordDecl, recordDecl->getClangDecl(), name}), {}); @@ -6159,7 +6159,7 @@ TinyPtrVector ClangRecordMemberLookup::evaluate( // Find the results that are actually a member of "recordDecl". TinyPtrVector result; ClangModuleLoader *clangModuleLoader = ctx.getClangModuleLoader(); - for (auto found : allResults) { + for (auto found : directResults) { auto named = found.get(); if (dyn_cast(named->getDeclContext()) == recordDecl->getClangDecl()) { From 011dc46a72496d53802a9b0edd268b24f08fd0b7 Mon Sep 17 00:00:00 2001 From: John Hui Date: Wed, 15 Jan 2025 17:12:24 -0800 Subject: [PATCH 4/6] Restructure ClangRecordMemberLookup to eagerly import inherited members Doing so avoids some annoyingly obscure false-positive errors --- lib/ClangImporter/ClangImporter.cpp | 76 +++++++++++++++-------------- 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index 65e717d395d17..47dae121f448f 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -27,6 +27,7 @@ #include "swift/AST/DiagnosticEngine.h" #include "swift/AST/DiagnosticsClangImporter.h" #include "swift/AST/DiagnosticsSema.h" +#include "swift/AST/Evaluator.h" #include "swift/AST/IRGenOptions.h" #include "swift/AST/ImportCache.h" #include "swift/AST/LinkLibrary.h" @@ -41,6 +42,7 @@ #include "swift/AST/Types.h" #include "swift/Basic/Assertions.h" #include "swift/Basic/Defer.h" +#include "swift/Basic/LLVM.h" #include "swift/Basic/Platform.h" #include "swift/Basic/Range.h" #include "swift/Basic/SourceLoc.h" @@ -6161,14 +6163,43 @@ TinyPtrVector ClangRecordMemberLookup::evaluate( ClangModuleLoader *clangModuleLoader = ctx.getClangModuleLoader(); for (auto found : directResults) { auto named = found.get(); - if (dyn_cast(named->getDeclContext()) == - recordDecl->getClangDecl()) { - // Don't import constructors on foreign reference types. - if (isa(named) && isa(recordDecl)) + if (dyn_cast(named->getDeclContext()) != + recordDecl->getClangDecl()) + continue; + + // Don't import constructors on foreign reference types. + if (isa(named) && isa(recordDecl)) + continue; + + auto imported = clangModuleLoader->importDeclDirectly(named); + if (!imported) + continue; + + // If this member is found due to inheritance, clone it from the base class + // by synthesizing getters and setters. + if (inheritingDecl != recordDecl) { + imported = clangModuleLoader->importBaseMemberDecl( + cast(imported), inheritingDecl); + if (!imported) continue; + } + result.push_back(cast(imported)); + } - if (auto import = clangModuleLoader->importDeclDirectly(named)) - result.push_back(cast(import)); + if (inheritingDecl != recordDecl) { + // For inheritied members, add members that are synthesized eagerly, such as + // subscripts. This is not necessary for non-inherited members because those + // should already be in the lookup table. + for (auto member : + cast(recordDecl)->getCurrentMembersWithoutLoading()) { + auto namedMember = dyn_cast(member); + if (!namedMember || !namedMember->hasName() || + namedMember->getName().getBaseName() != name) + continue; + + if (auto imported = clangModuleLoader->importBaseMemberDecl( + namedMember, inheritingDecl)) + result.push_back(cast(imported)); } } @@ -6207,43 +6238,14 @@ TinyPtrVector ClangRecordMemberLookup::evaluate( ClangRecordMemberLookup( {cast(import), name, inheritingDecl}), {}); - // Add members that are synthesized eagerly, such as subscripts. - for (auto member : - cast(import)->getCurrentMembersWithoutLoading()) { - if (auto namedMember = dyn_cast(member)) { - if (namedMember->hasName() && - namedMember->getName().getBaseName() == name && - // Make sure we don't add duplicate entries, as that would - // wrongly imply that lookup is ambiguous. - !llvm::is_contained(baseResults, namedMember)) { - baseResults.push_back(namedMember); - } - } - } + for (auto foundInBase : baseResults) { // Do not add duplicate entry with the same arity, // as that would cause an ambiguous lookup. if (foundNameArities.count(getArity(foundInBase))) continue; - // Do not importBaseMemberDecl() if this is a recursive lookup into - // some class's superclass. importBaseMemberDecl() caches synthesized - // members, which does not work if we call it on its result, e.g.: - // - // importBaseMemberDecl(importBaseMemberDecl(foundInBase, - // recorDecl), recordDecl) - // - // Instead, we simply pass on the imported decl (foundInBase) as is, - // so that only the top-most request calls importBaseMemberDecl(). - if (inheritingDecl != recordDecl) { - result.push_back(foundInBase); - continue; - } - - if (auto newDecl = clangModuleLoader->importBaseMemberDecl( - foundInBase, recordDecl)) { - result.push_back(newDecl); - } + result.push_back(foundInBase); } } } From 171fbaf59dd0bc3697067de12d974b909231a2ef Mon Sep 17 00:00:00 2001 From: John Hui Date: Thu, 16 Jan 2025 16:31:33 -0800 Subject: [PATCH 5/6] Remove redundant check --- lib/ClangImporter/ClangImporter.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index 47dae121f448f..926aab1221e23 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -6161,17 +6161,14 @@ TinyPtrVector ClangRecordMemberLookup::evaluate( // Find the results that are actually a member of "recordDecl". TinyPtrVector result; ClangModuleLoader *clangModuleLoader = ctx.getClangModuleLoader(); - for (auto found : directResults) { - auto named = found.get(); - if (dyn_cast(named->getDeclContext()) != - recordDecl->getClangDecl()) - continue; + for (auto foundEntry : directResults) { + auto found = foundEntry.get(); // Don't import constructors on foreign reference types. - if (isa(named) && isa(recordDecl)) + if (isa(found) && isa(recordDecl)) continue; - auto imported = clangModuleLoader->importDeclDirectly(named); + auto imported = clangModuleLoader->importDeclDirectly(found); if (!imported) continue; From dd292001e90fdf373513d7461fad649bcca09380 Mon Sep 17 00:00:00 2001 From: John Hui Date: Thu, 16 Jan 2025 19:30:42 -0800 Subject: [PATCH 6/6] Revert removed check --- lib/ClangImporter/ClangImporter.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index 926aab1221e23..0be0903587fd6 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -6163,6 +6163,9 @@ TinyPtrVector ClangRecordMemberLookup::evaluate( ClangModuleLoader *clangModuleLoader = ctx.getClangModuleLoader(); for (auto foundEntry : directResults) { auto found = foundEntry.get(); + if (dyn_cast(found->getDeclContext()) != + recordDecl->getClangDecl()) + continue; // Don't import constructors on foreign reference types. if (isa(found) && isa(recordDecl))