Skip to content

[Serialization] Ignore the exported module name for XRefs #41984

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.

Sign up for GitHub

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 1 commit into from
Mar 23, 2022
Merged

[Serialization] Ignore the exported module name for XRefs #41984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lib/Serialization/ModuleFormat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
/// describe what change you made. The content of this comment isn't important;
/// it just ensures a conflict if two people change the module format.
/// Don't worry about adhering to the 80-column limit for this line.
const uint16_t SWIFTMODULE_VERSION_MINOR = 679; // NoAsync
const uint16_t SWIFTMODULE_VERSION_MINOR = 680; // Ignore export_as in XRef

/// A standard hash seed used for all string hashes in a serialized module.
///
Expand Down
30 changes: 23 additions & 7 deletions lib/Serialization/Serialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,8 @@ namespace {
int32_t getNameDataForBase(const NominalTypeDecl *nominal,
StringRef *dataToWrite = nullptr) {
if (nominal->getDeclContext()->isModuleScopeContext())
return -Serializer.addContainingModuleRef(nominal->getDeclContext());
return -Serializer.addContainingModuleRef(nominal->getDeclContext(),
/*ignoreExport=*/false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this one use the exported name still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used by type-checking to lookup extensions from imported modules. At this point looking them up from what is visible by the client makes sense so I kept that logic as is.

This will likely require more work, it may be both on the lookup side and on what we write to the table. At least with the lookup table in the worst case the compiler won't see some extensions and their members, but it shouldn't crash.


auto &mangledName = MangledNameCache[nominal];
if (mangledName.empty())
Expand Down Expand Up @@ -727,7 +728,8 @@ IdentifierID Serializer::addFilename(StringRef filename) {
return addUniquedString(filename).second;
}

IdentifierID Serializer::addContainingModuleRef(const DeclContext *DC) {
IdentifierID Serializer::addContainingModuleRef(const DeclContext *DC,
bool ignoreExport) {
assert(!isa<ModuleDecl>(DC) &&
"References should be to things within modules");
const FileUnit *file = cast<FileUnit>(DC->getModuleScopeContext());
Expand All @@ -746,8 +748,19 @@ IdentifierID Serializer::addContainingModuleRef(const DeclContext *DC) {

auto exportedModuleName = file->getExportedModuleName();
assert(!exportedModuleName.empty());
auto exportedModuleID = M->getASTContext().getIdentifier(exportedModuleName);
return addDeclBaseNameRef(exportedModuleID);
auto moduleID = M->getASTContext().getIdentifier(exportedModuleName);
if (ignoreExport) {
auto realModuleName = M->getRealName().str();
assert(!realModuleName.empty());
if (realModuleName != exportedModuleName) {
// Still register the exported name as it can be referenced
// from the lookup tables.
addDeclBaseNameRef(moduleID);

moduleID = M->getASTContext().getIdentifier(realModuleName);
}
}
return addDeclBaseNameRef(moduleID);
}

IdentifierID Serializer::addModuleRef(const ModuleDecl *module) {
Expand Down Expand Up @@ -1632,7 +1645,8 @@ Serializer::writeASTBlockEntity(ProtocolConformance *conformance) {
abbrCode,
addDeclRef(normal->getProtocol()),
addDeclRef(normal->getType()->getAnyNominal()),
addContainingModuleRef(normal->getDeclContext()));
addContainingModuleRef(normal->getDeclContext(),
/*ignoreExport=*/true));
}
break;
}
Expand Down Expand Up @@ -1831,7 +1845,8 @@ void Serializer::writeCrossReference(const DeclContext *DC, uint32_t pathLen) {
case DeclContextKind::FileUnit:
abbrCode = DeclTypeAbbrCodes[XRefLayout::Code];
XRefLayout::emitRecord(Out, ScratchRecord, abbrCode,
addContainingModuleRef(DC), pathLen);
addContainingModuleRef(DC, /*ignoreExport=*/true),
pathLen);
break;

case DeclContextKind::GenericTypeDecl: {
Expand Down Expand Up @@ -1883,7 +1898,8 @@ void Serializer::writeCrossReference(const DeclContext *DC, uint32_t pathLen) {
genericSig = ext->getGenericSignature().getCanonicalSignature();
}
XRefExtensionPathPieceLayout::emitRecord(
Out, ScratchRecord, abbrCode, addContainingModuleRef(DC),
Out, ScratchRecord, abbrCode,
addContainingModuleRef(DC, /*ignoreExport=*/true),
addGenericSignatureRef(genericSig));
break;
}
Expand Down
5 changes: 4 additions & 1 deletion lib/Serialization/Serialization.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,10 +518,13 @@ class Serializer : public SerializerBase {
/// may not be exactly the same as the name of the module containing DC;
/// instead, it will match the containing file's "exported module name".
///
/// \param ignoreExport When true, register the real module name,
/// ignoring exported_as definitions.
/// \returns The ID for the identifier for the module's name, or one of the
/// special module codes defined above.
/// \see FileUnit::getExportedModuleName
IdentifierID addContainingModuleRef(const DeclContext *DC);
IdentifierID addContainingModuleRef(const DeclContext *DC,
bool ignoreExport);

/// Records the module \m.
IdentifierID addModuleRef(const ModuleDecl *m);
Expand Down
30 changes: 17 additions & 13 deletions test/ClangImporter/private_frameworks.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,23 @@
// FIXME: END -enable-source-import hackaround

// Build the overlay with private frameworks.
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-module -F %S/Inputs/privateframeworks/withprivate -o %t %S/Inputs/privateframeworks/overlay/SomeKit.swift
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-module -F %S/Inputs/privateframeworks/withprivate -o %t %S/Inputs/privateframeworks/overlay/SomeKit.swift -enable-library-evolution -emit-module-interface-path %t/SomeKit.swiftinterface

// Use the overlay with private frameworks.
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate -swift-version 4 %s -import-objc-header %S/Inputs/privateframeworks/bridging-somekitcore.h -verify
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate -swift-version 4 %s -import-objc-header %S/Inputs/privateframeworks/bridging-somekitcore.h
Copy link
Contributor

Choose a reason for hiding this comment

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

The switch from -verify -> not confused me for a little bit there, but IIUC it was superfluous before because no errors were generated. And now there's only one case with -verify, where you do actually check it errors.

So the only actual change here is the addition of that extra 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.

Yeah, the previous presence of -verify only report unexpected warnings I suppose. We're losing this protection by removing -verify from all other RUN lines but it's not a very likely scenario to get warnings here.

The main change is really that using the binary swiftmodules doesn't work anymore, but rebuilding from the swiftinterface still provides the same old behavior.


// Use the overlay without private frameworks.
// Using the binary overlay without private frameworks should fail.
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate -swift-version 4 -import-objc-header %S/Inputs/privateframeworks/bridging-somekit.h %s -verify

// Using an overlay rebuilt from the swiftinterface without private frameworks should work.
// RUN: rm %t/SomeKit.swiftmodule
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate -swift-version 4 -import-objc-header %S/Inputs/privateframeworks/bridging-somekit.h %s

// Build the overlay with public frameworks.
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-module -F %S/Inputs/privateframeworks/withoutprivate -o %t %S/Inputs/privateframeworks/overlay/SomeKit.swift

// Use the overlay with private frameworks.
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate -swift-version 4 %s -import-objc-header %S/Inputs/privateframeworks/bridging-somekitcore.h -verify
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate -swift-version 4 %s -import-objc-header %S/Inputs/privateframeworks/bridging-somekitcore.h

// Use the overlay without private frameworks.
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate -swift-version 4 %s -import-objc-header %S/Inputs/privateframeworks/bridging-somekit.h
Expand All @@ -27,12 +31,12 @@
// RUN: echo 'import private_frameworks; testErrorConformance()' > %t/main.swift

// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-module -o %t -F %S/Inputs/privateframeworks/withprivate -swift-version 4 %s -import-objc-header %S/Inputs/privateframeworks/bridging-somekitcore.h
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate %t/main.swift -verify
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate %t/main.swift -verify
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate %t/main.swift
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate %t/main.swift

// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-module -o %t -F %S/Inputs/privateframeworks/withoutprivate -swift-version 4 %s -import-objc-header %S/Inputs/privateframeworks/bridging-somekit.h
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate %t/main.swift -verify
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate %t/main.swift -verify
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate %t/main.swift
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate %t/main.swift

// REQUIRES: objc_interop

Expand All @@ -42,18 +46,18 @@ func testWidget(widget: SKWidget) {
widget.someObjCMethod()
widget.someObjCExtensionMethod()

let ext = widget.extensionMethod()
let ext = widget.extensionMethod() // expected-error {{value of type 'SKWidget' has no member 'extensionMethod'}}
ext.foo()

widget.doSomethingElse(widget)
inlineWidgetOperations(widget)
widget.doSomethingElse(widget) // expected-error {{value of type 'SKWidget' has no member 'doSomethingElse'; did you mean 'doSomething'?}}
inlineWidgetOperations(widget) // expected-error {{cannot find 'inlineWidgetOperations' in scope}}

let _ = widget.name
}

func testError(widget: SKWidget) {
let c: SKWidget.Error.Code = SKWidget.Error(.boom).getCode(from: widget)
if c.isBoom { }
let c: SKWidget.Error.Code = SKWidget.Error(.boom).getCode(from: widget) // expected-error {{value of type 'SKWidget.Error' has no member 'getCode'}}
if c.isBoom { } // expected-error {{value of type 'SKWidget.Error.Code' has no member 'isBoom'}}
}

func testGlobals() {
Expand Down
Empty file added test/Serialization/Inputs/exported-modules/Exported.h
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions test/Serialization/Inputs/exported-modules/ExportedCore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
struct ExportedType {};
9 changes: 9 additions & 0 deletions test/Serialization/Inputs/exported-modules/module.modulemap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module ExportedCore [system] [extern_c] {
header "ExportedCore.h"

export_as Exported
}

module Exported [system] [extern_c] {
header "Exported.h"
}
19 changes: 19 additions & 0 deletions test/Serialization/export_as_xrefs.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/// Ensure that export_as decls don't cause a deserialization failure (even one if recovered from)
/// rdar://90272035

// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend -emit-module -DLIB_A %s -module-name A -emit-module-path %t/A.swiftmodule -I %t -I %S/Inputs/exported-modules
// RUN: %target-swift-frontend -emit-module -DLIB_B %s -module-name B -emit-module-path %t/B.swiftmodule -I %t -I %S/Inputs/exported-modules -disable-deserialization-recovery

#if LIB_A
import ExportedCore

public func foo() -> ExportedType { fatalError() }

#elseif LIB_B

import A

let a = foo()

#endif