Skip to content

[ClangImporter] Add direct access for import-as-member types. #10956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 17, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions include/swift/AST/Module.h
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,18 @@ class FileUnit : public DeclContext {
return nullptr;
}

/// Directly look for a nested type declared within this module inside the
/// given nominal type (including any extensions).
///
/// This is a fast-path hack to avoid circular dependencies in deserialization
/// and the Clang importer.
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used as a failable fast path? That is, if this succeeds we're fast and if it returns nullptr we go through slower lookups?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. It could probably be non-failable but since the eventual goal is to remove it altogether…no reason not to be cautious.

/// Private and fileprivate types should not be returned by this lookup.
virtual TypeDecl *lookupNestedType(Identifier name,
const NominalTypeDecl *parent) const {
return nullptr;
}

/// Find ValueDecls in the module and pass them to the given consumer object.
///
/// This does a simple local lookup, not recursively looking through imports.
Expand Down
4 changes: 4 additions & 0 deletions include/swift/ClangImporter/ClangModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ class ClangModuleUnit final : public LoadedFile {
DeclName name, NLKind lookupKind,
SmallVectorImpl<ValueDecl*> &results) const override;

virtual TypeDecl *
lookupNestedType(Identifier name,
const NominalTypeDecl *baseType) const override;

virtual void lookupVisibleDecls(ModuleDecl::AccessPathTy accessPath,
VisibleDeclConsumer &consumer,
NLKind lookupKind) const override;
Expand Down
2 changes: 1 addition & 1 deletion include/swift/Serialization/ModuleFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@ class ModuleFile : public LazyMemberLoader {

/// Searches the module's nested type decls table for the given member of
/// the given type.
TypeDecl *lookupNestedType(Identifier name, const ValueDecl *parent);
TypeDecl *lookupNestedType(Identifier name, const NominalTypeDecl *parent);

/// Searches the module's operators for one with the given name and fixity.
///
Expand Down
4 changes: 4 additions & 0 deletions include/swift/Serialization/SerializedModuleLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,10 @@ class SerializedASTFile final : public LoadedFile {

virtual TypeDecl *lookupLocalType(StringRef MangledName) const override;

virtual TypeDecl *
lookupNestedType(Identifier name,
const NominalTypeDecl *parent) const override;

virtual OperatorDecl *lookupOperator(Identifier name,
DeclKind fixity) const override;

Expand Down
89 changes: 87 additions & 2 deletions lib/ClangImporter/ClangImporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2432,6 +2432,91 @@ static bool isVisibleClangEntry(clang::ASTContext &ctx,
return true;
}

TypeDecl *
ClangModuleUnit::lookupNestedType(Identifier name,
const NominalTypeDecl *baseType) const {
// Special case for error code enums: try looking directly into the struct
// first. But only if it looks like a synthesized error wrapped struct.
if (name == getASTContext().Id_Code && !baseType->hasClangNode() &&
isa<StructDecl>(baseType) && !baseType->hasLazyMembers() &&
baseType->isChildContextOf(this)) {
auto *mutableBase = const_cast<NominalTypeDecl *>(baseType);
auto codeEnum = mutableBase->lookupDirect(name,/*ignoreNewExtensions*/true);
// Double-check that we actually have a good result. It's possible what we
// found is /not/ a synthesized error struct, but just something that looks
// like it. But if we still found a good result we should return that.
if (codeEnum.size() == 1 && isa<TypeDecl>(codeEnum.front()))
return cast<TypeDecl>(codeEnum.front());
if (codeEnum.size() > 1)
return nullptr;
// Otherwise, fall back and try via lookup table.
}

auto lookupTable = owner.findLookupTable(clangModule);
if (!lookupTable)
return nullptr;

auto baseTypeContext = owner.getEffectiveClangContext(baseType);
if (!baseTypeContext)
return nullptr;

auto &clangCtx = owner.getClangASTContext();

// FIXME: This is very similar to what's in Implementation::lookupValue and
// Implementation::loadAllMembers.
SmallVector<TypeDecl *, 2> results;
for (auto entry : lookupTable->lookup(SerializedSwiftName(name.str()),
baseTypeContext)) {
// If the entry is not visible, skip it.
if (!isVisibleClangEntry(clangCtx, entry)) continue;

auto clangDecl = entry.dyn_cast<clang::NamedDecl *>();
auto clangTypeDecl = dyn_cast_or_null<clang::TypeDecl>(clangDecl);
if (!clangTypeDecl)
continue;

clangTypeDecl = cast<clang::TypeDecl>(clangTypeDecl->getMostRecentDecl());

bool anyMatching = false;
TypeDecl *originalDecl = nullptr;
owner.forEachDistinctName(clangTypeDecl, [&](ImportedName newName,
ImportNameVersion nameVersion){
if (anyMatching)
return;
if (!newName.getDeclName().isSimpleName(name))
return;

auto decl = dyn_cast_or_null<TypeDecl>(
owner.importDeclReal(clangTypeDecl, nameVersion));
if (!decl)
return;

if (!originalDecl)
originalDecl = decl;
else if (originalDecl == decl)
return;

auto *importedContext = decl->getDeclContext()->
getAsNominalTypeOrNominalTypeExtensionContext();
if (importedContext != baseType)
return;

assert(decl->getFullName().matchesRef(name) &&
"importFullName behaved differently from importDecl");
results.push_back(decl);
anyMatching = true;
});
}

if (results.size() != 1) {
// It's possible that two types were import-as-member'd onto the same base
// type with the same name. In this case, fall back to regular lookup.
return nullptr;
}

return results.front();
}

void ClangImporter::loadExtensions(NominalTypeDecl *nominal,
unsigned previousGeneration) {
// Determine the effective Clang context for this Swift nominal type.
Expand Down Expand Up @@ -3127,7 +3212,7 @@ void ClangImporter::Implementation::lookupAllObjCMembers(
}

EffectiveClangContext ClangImporter::Implementation::getEffectiveClangContext(
NominalTypeDecl *nominal) {
const NominalTypeDecl *nominal) {
// If we have a Clang declaration, look at it to determine the
// effective Clang context.
if (auto constClangDecl = nominal->getClangDecl()) {
Expand All @@ -3142,7 +3227,7 @@ EffectiveClangContext ClangImporter::Implementation::getEffectiveClangContext(

// Resolve the type.
if (auto typeResolver = getTypeResolver())
typeResolver->resolveDeclSignature(nominal);
typeResolver->resolveDeclSignature(const_cast<NominalTypeDecl *>(nominal));

// If it's an @objc entity, go look for it.
if (nominal->isObjC()) {
Expand Down
3 changes: 2 additions & 1 deletion lib/ClangImporter/ImporterImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1180,7 +1180,8 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
VisibleDeclConsumer &consumer);

/// Determine the effective Clang context for the given Swift nominal type.
EffectiveClangContext getEffectiveClangContext(NominalTypeDecl *nominal);
EffectiveClangContext
getEffectiveClangContext(const NominalTypeDecl *nominal);

/// Attempts to import the name of \p decl with each possible
/// ImportNameVersion. \p action will be called with each unique name.
Expand Down
26 changes: 9 additions & 17 deletions lib/Serialization/Deserialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1319,9 +1319,7 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
&blobData);
switch (recordID) {
case XREF_TYPE_PATH_PIECE: {
if (values.size() == 1) {
ModuleDecl *module = values.front()->getModuleContext();

if (values.size() == 1 && isa<NominalTypeDecl>(values.front())) {
// Fast path for nested types that avoids deserializing all
// members of the parent type.
IdentifierID IID;
Expand All @@ -1334,29 +1332,23 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
"If you're seeing a crash here, try passing "
"-Xfrontend -disable-serialization-nested-type-lookup-table"};

auto *baseType = cast<NominalTypeDecl>(values.front());
TypeDecl *nestedType = nullptr;
if (onlyInNominal) {
// Only look in the file containing the type itself.
const DeclContext *dc = values.front()->getDeclContext();
auto *serializedFile =
dyn_cast<SerializedASTFile>(dc->getModuleScopeContext());
if (serializedFile) {
nestedType =
serializedFile->File.lookupNestedType(memberName,
values.front());
auto *containingFile =
dyn_cast<FileUnit>(dc->getModuleScopeContext());
if (containingFile) {
nestedType = containingFile->lookupNestedType(memberName, baseType);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can any of this logic be factored out into a separate function with early returns? Perhaps return when the nested type is actually found? I'm having a hard time understanding the logic here. I realized that refactoring this method is probably outside the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would certainly be reasonable; resolveCrossReference is getting huge. But yeah, this part is supposed to be a small tweak to what's already there.

} else {
// Fault in extensions, then ask every serialized AST in the module.
(void)cast<NominalTypeDecl>(values.front())->getExtensions();
for (FileUnit *file : module->getFiles()) {
(void)baseType->getExtensions();
for (FileUnit *file : baseType->getModuleContext()->getFiles()) {
if (file == getFile())
continue;
auto *serializedFile = dyn_cast<SerializedASTFile>(file);
if (!serializedFile)
continue;
nestedType =
serializedFile->File.lookupNestedType(memberName,
values.front());
nestedType = file->lookupNestedType(memberName, baseType);
if (nestedType)
break;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/Serialization/ModuleFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1363,7 +1363,7 @@ TypeDecl *ModuleFile::lookupLocalType(StringRef MangledName) {
}

TypeDecl *ModuleFile::lookupNestedType(Identifier name,
const ValueDecl *parent) {
const NominalTypeDecl *parent) {
PrettyStackTraceModuleFile stackEntry(*this);

if (!NestedTypeDecls)
Expand Down
6 changes: 6 additions & 0 deletions lib/Serialization/SerializedModuleLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,12 @@ TypeDecl *SerializedASTFile::lookupLocalType(llvm::StringRef MangledName) const{
return File.lookupLocalType(MangledName);
}

TypeDecl *
SerializedASTFile::lookupNestedType(Identifier name,
const NominalTypeDecl *parent) const {
return File.lookupNestedType(name, parent);
}

OperatorDecl *SerializedASTFile::lookupOperator(Identifier name,
DeclKind fixity) const {
return File.lookupOperator(name, fixity);
Expand Down
8 changes: 8 additions & 0 deletions test/ClangImporter/Inputs/custom-modules/ImportAsMember.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,12 @@ extern void IAMStruct1SelfComesLast(double x, struct IAMStruct1 s)
extern void IAMStruct1SelfComesThird(int a, float b, struct IAMStruct1 s, double x)
__attribute__((swift_name("Struct1.selfComesThird(a:b:self:x:)")));


struct IAMMultipleNested {
int value;
};

typedef int MNInnerInt __attribute__((swift_name("IAMMultipleNested.Inner")));
typedef float MNInnerFloat __attribute__((swift_name("IAMMultipleNested.Inner")));

#endif // IMPORT_AS_MEMBER_H
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
struct IAMOuter { int x; };
struct IAMSOuter { int x; };

struct IAMInner { int y; };
struct IAMSInner { int y; };
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// The order of these forward-declarations affects whether there was a bug.
struct IAMOuter;
struct IAMInner;
struct IAMSOuter;
struct IAMSInner;
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Name: ImportAsMemberSubmodules
Tags:
- Name: IAMInner
SwiftName: IAMOuter.Inner
- Name: IAMSInner
SwiftName: IAMSOuter.Inner
6 changes: 4 additions & 2 deletions test/ClangImporter/import-as-member.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -F %S/Inputs/frameworks %s -verify
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -F %S/Inputs/frameworks -I %S/Inputs/custom-modules %s -verify

import ImportAsMember
import ImportAsMemberSubmodules

let _: IAMOuter.Inner?
let _: IAMSOuter.Inner?
let _: IAMMultipleNested.Inner? // expected-error {{ambiguous type name 'Inner' in 'IAMMultipleNested'}}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
struct Outer {
int value;
};

struct __attribute__((swift_name("Outer.InterestingValue"))) Inner {
int value;
};

#if __OBJC__

@import Foundation;

extern NSString * const ErrorCodeDomain;
enum __attribute__((ns_error_domain(ErrorCodeDomain))) ErrorCodeEnum {
ErrorCodeEnumA
};

#endif // __OBJC__
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module NestedClangTypes {
header "NestedClangTypes.h"
}
48 changes: 48 additions & 0 deletions test/Serialization/xref-nested-clang-type.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-module -o %t -I %S/Inputs/xref-nested-clang-type/ %s -module-name Lib
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -I %t -I %S/Inputs/xref-nested-clang-type/ %s -DCLIENT -verify

// RUN: %empty-directory(%t)
// RUN: cp %S/Inputs/xref-nested-clang-type/NestedClangTypes.h %t
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-module -o %t -import-objc-header %t/NestedClangTypes.h %s -module-name Lib -DNO_MODULE
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -I %t -import-objc-header %t/NestedClangTypes.h %s -DCLIENT -verify

// RUN: %empty-directory(%t)
// RUN: cp %S/Inputs/xref-nested-clang-type/NestedClangTypes.h %t
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-module -o %t -import-objc-header %t/NestedClangTypes.h -pch-output-dir %t/PCHCache %s -module-name Lib -DNO_MODULE
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -I %t -pch-output-dir %t/PCHCache -import-objc-header %t/NestedClangTypes.h %s -DCLIENT -verify

#if CLIENT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this PR is about fast paths, then should you be testing the value of the stats like NumNestedTypeShortcuts? Or is this a correctness test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a correctness test in this case; master will crash with circular dependencies without the "fast path". Maybe I should call it a "direct path".


import Lib

func test(x: MyInner) {}

#if _runtime(_ObjC)
import Foundation
func test(x: MyCode) {}
#endif // ObjC

#else // CLIENT

#if !NO_MODULE
import NestedClangTypes
#endif

public typealias MyOuter = Outer
public typealias MyInner = Outer.InterestingValue

extension MyOuter {
public func use(inner: MyInner) {}
}

#if _runtime(_ObjC)
import Foundation

public typealias MyCode = ErrorCodeEnum.Code
extension ErrorCodeEnum {
public func whatever(code: MyCode) {}
}
#endif // ObjC

#endif // CLIENT