Skip to content

[ScanDependency] Do not use public/private swiftinterface in the same package #71857

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 1 commit into from
Feb 29, 2024
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
3 changes: 3 additions & 0 deletions include/swift/Basic/LangOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,9 @@ namespace swift {
/// The name of the package this module belongs to.
std::string PackageName;

/// Allow importing a non-package interface from the same package.
bool AllowNonPackageInterfaceImportFromSamePackage = false;

/// Diagnose implicit 'override'.
bool WarnImplicitOverrides = false;

Expand Down
1 change: 1 addition & 0 deletions include/swift/Frontend/FrontendInputsAndOutputs.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ class FrontendInputsAndOutputs {
bool shouldTreatAsLLVM() const;
bool shouldTreatAsSIL() const;
bool shouldTreatAsModuleInterface() const;
bool shouldTreatAsNonPackageModuleInterface() const;
bool shouldTreatAsObjCHeader() const;

bool areAllNonPrimariesSIB() const;
Expand Down
5 changes: 5 additions & 0 deletions include/swift/Serialization/SerializedModuleLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ struct SerializedModuleBaseName {
std::optional<std::string> findInterfacePath(llvm::vfs::FileSystem &fs,
ASTContext &ctx) const;

/// Returns the package-name of the interface file.
std::optional<std::string>
getPackageNameFromInterface(StringRef interfacePath,
llvm::vfs::FileSystem &fs) const;

/// Returns the .package.swiftinterface path if its package-name also applies to
/// the the importing module. Returns an empty optional otherwise.
std::optional<std::string>
Expand Down
9 changes: 8 additions & 1 deletion lib/Frontend/CompilerInvocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -974,8 +974,15 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
auto pkgName = A->getValue();
if (StringRef(pkgName).empty())
Diags.diagnose(SourceLoc(), diag::error_empty_package_name);
else
else {
Opts.PackageName = pkgName;
// Unless the input type is public or private swift interface, do not
// allow non package interface imports for dependencies in the same
// package.
Opts.AllowNonPackageInterfaceImportFromSamePackage =
FrontendOpts.InputsAndOutputs
.shouldTreatAsNonPackageModuleInterface();
}
}

if (const Arg *A = Args.getLastArg(OPT_require_explicit_availability_EQ)) {
Expand Down
10 changes: 10 additions & 0 deletions lib/Frontend/FrontendInputsAndOutputs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,16 @@ bool FrontendInputsAndOutputs::shouldTreatAsModuleInterface() const {
return InputType == file_types::TY_SwiftModuleInterfaceFile;
}

bool FrontendInputsAndOutputs::shouldTreatAsNonPackageModuleInterface() const {
if (!hasSingleInput())
return false;

file_types::ID InputType =
file_types::lookupTypeFromFilename(getFilenameOfFirstInput());
return InputType == file_types::TY_SwiftModuleInterfaceFile ||
Copy link
Contributor

@elsh elsh Feb 28, 2024

Choose a reason for hiding this comment

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

Where are these input types specified? In deps.json as in the test below?

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 the type of the input on the command-line, determined by file extension. The only time something can import public/private interface when in the same package is building a public/private interface from the same package, hence checking the input type.

InputType == file_types::TY_PrivateSwiftModuleInterfaceFile;
}

bool FrontendInputsAndOutputs::shouldTreatAsSIL() const {
if (hasSingleInput()) {
// If we have exactly one input filename, and its extension is "sil",
Expand Down
12 changes: 6 additions & 6 deletions lib/Frontend/ModuleInterfaceLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1244,8 +1244,8 @@ std::error_code ModuleInterfaceLoader::findModuleFilesInDirectory(

// First check to see if the .swiftinterface exists at all. Bail if not.
auto &fs = *Ctx.SourceMgr.getFileSystem();
std::string InPath = BaseName.findInterfacePath(fs, Ctx).value_or("");
if (InPath.empty()) {
auto InPath = BaseName.findInterfacePath(fs, Ctx);
if (!InPath) {
if (fs.exists(ModPath)) {
LLVM_DEBUG(llvm::dbgs()
<< "No .swiftinterface file found adjacent to module file "
Expand All @@ -1256,22 +1256,22 @@ std::error_code ModuleInterfaceLoader::findModuleFilesInDirectory(
}

if (ModuleInterfaceSourcePath)
ModuleInterfaceSourcePath->assign(InPath.begin(), InPath.end());
ModuleInterfaceSourcePath->assign(InPath->begin(), InPath->end());

// If we've been told to skip building interfaces, we are done here and do
// not need to have the module actually built. For example, if we are
// currently answering a `canImport` query, it is enough to have found
// the interface.
if (skipBuildingInterface) {
if (ModuleInterfacePath)
ModuleInterfacePath->assign(InPath.begin(), InPath.end());
ModuleInterfacePath->assign(InPath->begin(), InPath->end());
return std::error_code();
}

// Create an instance of the Impl to do the heavy lifting.
auto ModuleName = ModuleID.Item.str();
ModuleInterfaceLoaderImpl Impl(
Ctx, ModPath, InPath, ModuleName, InterfaceChecker.CacheDir,
Ctx, ModPath, *InPath, ModuleName, InterfaceChecker.CacheDir,
InterfaceChecker.PrebuiltCacheDir, InterfaceChecker.BackupInterfaceDir,
ModuleID.Loc, InterfaceChecker.Opts,
InterfaceChecker.RequiresOSSAModules,
Expand All @@ -1290,7 +1290,7 @@ std::error_code ModuleInterfaceLoader::findModuleFilesInDirectory(
if (ModuleBuffer) {
*ModuleBuffer = std::move(*ModuleBufferOrErr);
if (ModuleInterfacePath)
ModuleInterfacePath->assign(InPath.begin(), InPath.end());
ModuleInterfacePath->assign(InPath->begin(), InPath->end());
}

// Open .swiftsourceinfo file if it's present.
Expand Down
24 changes: 5 additions & 19 deletions lib/Serialization/ScanningLoaders.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ std::error_code SwiftModuleScanner::findModuleFilesInDirectory(
auto &fs = *Ctx.SourceMgr.getFileSystem();

auto ModPath = BaseName.getName(file_types::TY_SwiftModuleFile);
auto InPath = BaseName.getName(file_types::TY_SwiftModuleInterfaceFile);
auto InPath = BaseName.findInterfacePath(fs, Ctx);

if (LoadMode == ModuleLoadingMode::OnlySerialized || !fs.exists(InPath)) {
if (LoadMode == ModuleLoadingMode::OnlySerialized || !InPath) {
if (fs.exists(ModPath)) {
// The module file will be loaded directly.
auto dependencies = scanModuleFile(ModPath, IsFramework);
Expand All @@ -61,26 +61,12 @@ std::error_code SwiftModuleScanner::findModuleFilesInDirectory(
return std::error_code();
}
return dependencies.getError();
} else {
return std::make_error_code(std::errc::no_such_file_or_directory);
}
return std::make_error_code(std::errc::no_such_file_or_directory);
}
assert(fs.exists(InPath));
assert(InPath);

// Use package.swiftinterface if it exists and its package-name applies to
// the importer module.
auto PkgInPath = BaseName.getPackageInterfacePathIfInSamePackage(fs, Ctx).value_or("");
if (!PkgInPath.empty() && fs.exists(PkgInPath)) {
InPath = PkgInPath;
} else {
// If not in package, use the private interface file if exits.
auto PrivateInPath =
BaseName.getName(file_types::TY_PrivateSwiftModuleInterfaceFile);
if (fs.exists(PrivateInPath)) {
InPath = PrivateInPath;
}
}
auto dependencies = scanInterfaceFile(InPath, IsFramework);
auto dependencies = scanInterfaceFile(*InPath, IsFramework);
if (dependencies) {
this->dependencies = std::move(dependencies.get());
return std::error_code();
Expand Down
104 changes: 55 additions & 49 deletions lib/Serialization/SerializedModuleLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -604,35 +604,43 @@ std::optional<std::string>
SerializedModuleBaseName::getPackageInterfacePathIfInSamePackage(
llvm::vfs::FileSystem &fs, ASTContext &ctx) const {
if (!ctx.LangOpts.EnablePackageInterfaceLoad)
return {};
return std::nullopt;

std::string packagePath{
getName(file_types::TY_PackageSwiftModuleInterfaceFile)};
getName(file_types::TY_PackageSwiftModuleInterfaceFile)};

if (fs.exists(packagePath)) {
// Read the interface file and extract its package-name argument value
std::string result;
if (auto packageFile = llvm::MemoryBuffer::getFile(packagePath)) {
llvm::BumpPtrAllocator alloc;
llvm::StringSaver argSaver(alloc);
SmallVector<const char*, 8> args;
(void)extractCompilerFlagsFromInterface(packagePath,
(*packageFile)->getBuffer(), argSaver, args);
for (unsigned I = 0, N = args.size(); I + 1 < N; I++) {
StringRef current(args[I]), next(args[I + 1]);
if (current == "-package-name") {
// Instead of `break` here, continue to get the last value in case of dupes,
// to be consistent with the default parsing logic.
result = next;
}
if (auto packageName = getPackageNameFromInterface(packagePath, fs)) {
// Return the .package.swiftinterface path if the package name applies to
// the importer module.
if (*packageName == ctx.LangOpts.PackageName)
return packagePath;
}
}
return std::nullopt;
}

std::optional<std::string>
SerializedModuleBaseName::getPackageNameFromInterface(
StringRef interfacePath, llvm::vfs::FileSystem &fs) const {
std::optional<std::string> result;
if (auto interfaceFile = fs.getBufferForFile(interfacePath)) {
llvm::BumpPtrAllocator alloc;
llvm::StringSaver argSaver(alloc);
SmallVector<const char *, 8> args;
(void)extractCompilerFlagsFromInterface(
interfacePath, (*interfaceFile)->getBuffer(), argSaver, args);
for (unsigned I = 0, N = args.size(); I + 1 < N; I++) {
StringRef current(args[I]), next(args[I + 1]);
if (current == "-package-name") {
// Instead of `break` here, continue to get the last value in case of
// dupes, to be consistent with the default parsing logic.
result = next;
}
}
// Return the .package.swiftinterface path if the package name applies to
// the importer module.
if (!result.empty() && result == ctx.LangOpts.PackageName)
return packagePath;
}
return {};
return result;
}

std::optional<std::string>
Expand All @@ -642,16 +650,18 @@ SerializedModuleBaseName::findInterfacePath(llvm::vfs::FileSystem &fs,
// Ensure the public swiftinterface already exists, otherwise bail early
// as it's considered the module doesn't exist.
if (!fs.exists(interfacePath))
return {};

// Check if a package interface exists and if the package name applies to
// the importer module.
auto pkgPath = getPackageInterfacePathIfInSamePackage(fs, ctx).value_or("");
if (!pkgPath.empty() && fs.exists(pkgPath))
return pkgPath;
return std::nullopt;

// If in the same package, try return the package interface path if not
// preferring the interface file.
if (!ctx.LangOpts.AllowNonPackageInterfaceImportFromSamePackage) {
if (auto packageName = getPackageNameFromInterface(interfacePath, fs)) {
if (*packageName == ctx.LangOpts.PackageName)
return getPackageInterfacePathIfInSamePackage(fs, ctx);
}
}

// If above fails, use the existing logic as fallback.
// If present, use the private interface instead of the public one.
// Otherwise, use the private interface instead of the public one.
std::string privatePath{
getName(file_types::TY_PrivateSwiftModuleInterfaceFile)};
if (fs.exists(privatePath))
Expand Down Expand Up @@ -704,35 +714,33 @@ bool SerializedModuleLoaderBase::findModule(
std::optional<SerializedModuleBaseName> firstAbsoluteBaseName;

for (const auto &targetSpecificBaseName : targetSpecificBaseNames) {
SerializedModuleBaseName
absoluteBaseName{currPath, targetSpecificBaseName};
SerializedModuleBaseName absoluteBaseName{currPath,
targetSpecificBaseName};

if (!firstAbsoluteBaseName.has_value())
firstAbsoluteBaseName.emplace(absoluteBaseName);

auto result = findModuleFilesInDirectory(
moduleID, absoluteBaseName, moduleInterfacePath,
moduleInterfaceSourcePath, moduleBuffer, moduleDocBuffer,
moduleSourceInfoBuffer, skipBuildingInterface,
IsFramework, isTestableDependencyLookup);
if (!result) {
moduleSourceInfoBuffer, skipBuildingInterface, IsFramework,
isTestableDependencyLookup);
if (!result)
return SearchResult::Found;
} else if (result == std::errc::not_supported) {
if (result == std::errc::not_supported)
return SearchResult::Error;
} else if (result != std::errc::no_such_file_or_directory) {
if (result != std::errc::no_such_file_or_directory)
return SearchResult::NotFound;
}
}

// We can only get here if all targetFileNamePairs failed with
// 'std::errc::no_such_file_or_directory'.
if (firstAbsoluteBaseName
&& maybeDiagnoseTargetMismatch(moduleID.Loc, moduleName,
*firstAbsoluteBaseName)) {
if (firstAbsoluteBaseName &&
maybeDiagnoseTargetMismatch(moduleID.Loc, moduleName,
*firstAbsoluteBaseName))
return SearchResult::Error;
} else {
return SearchResult::NotFound;
}

return SearchResult::NotFound;
};

SmallVector<std::string, 4> InterestingFilenames = {
Expand Down Expand Up @@ -788,13 +796,11 @@ bool SerializedModuleLoaderBase::findModule(
moduleID, absoluteBaseName, moduleInterfacePath,
moduleInterfaceSourcePath, moduleBuffer, moduleDocBuffer,
moduleSourceInfoBuffer, skipBuildingInterface, isFramework);
if (!result) {
if (!result)
return true;
} else if (result == std::errc::not_supported) {
if (result == std::errc::not_supported)
return false;
} else {
continue;
}
continue;
}
case ModuleSearchPathKind::Framework:
case ModuleSearchPathKind::DarwinImplicitFramework: {
Expand Down
7 changes: 7 additions & 0 deletions test/ScanDependencies/package_interface.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// RUN: %target-swift-frontend -emit-module %t/Bar.swift -I %t \
// RUN: -module-name Bar -package-name barpkg \
// RUN: -enable-library-evolution -swift-version 5 \
// RUN: -o %t/Bar.swiftmodule \
// RUN: -emit-module-interface-path %t/Bar.swiftinterface \
// RUN: -emit-private-module-interface-path %t/Bar.private.swiftinterface \
// RUN: -emit-package-module-interface-path %t/Bar.package.swiftinterface
Expand All @@ -21,10 +22,16 @@
// RUN: %t/Client.swift -module-name Client -swift-version 5
// RUN: %FileCheck %s --input-file=%t/deps3.json --check-prefix CHECK --check-prefix CHECK-PRIVATE

/// If -experimental-package-interface-load is not used but in the same package, it should find the binary module
// RUN: %target-swift-frontend -scan-dependencies -I %t \
// RUN: %t/Client.swift -module-name Client -package-name barpkg -swift-version 5 | \
// RUN: %FileCheck %s --check-prefix CHECK-BINARY

// CHECK: "swift": "Bar"
// CHECK: "modulePath": "{{.*}}{{/|\\}}Bar-{{.*}}.swiftmodule"
// CHECK-PACKAGE: "moduleInterfacePath": "{{.*}}{{/|\\}}Bar.package.swiftinterface"
// CHECK-PRIVATE: "moduleInterfacePath": "{{.*}}{{/|\\}}Bar.private.swiftinterface"
// CHECK-BINARY: "swiftPrebuiltExternal": "Bar"

//--- Bar.swift
public enum PubEnum {
Expand Down
2 changes: 1 addition & 1 deletion test/Serialization/load_package_module.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

// RUN: not %target-swift-frontend -typecheck %t/ClientLoadInterface.swift -package-name libPkg -I %t 2> %t/resultY.output
// RUN: %FileCheck %s -check-prefix CHECK-Y < %t/resultY.output
// CHECK-Y: error: module 'LibInterface' is in package 'libPkg' but was built from a non-package interface; modules of the same package can only be loaded if built from source or package interface: {{.*}}LibInterface.private.swiftinterface
// CHECK-Y: error: no such module 'LibInterface'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the behavior we want. The original diagnostics should still happen here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So... this is tricky. The reason for the diagnostics do not happen is after the change, the private swiftinterface is not considered a valid dependency for the source, thus it is not doing an implicit module build to create a module that cannot be imported with the error.

You can still construct a case that trigger the warning but you have to produce the wrong module first before running the implicit build. That behavior is verified in test/Sema/accessibility_package_import_interface.swift



//--- Lib.swift
Expand Down