-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Print proper error message when the swiftmodule for architecture not found #17092
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
Changes from 4 commits
ea3a9f4
eebc8de
00b8f82
f15cae9
4b32103
f850c53
4b9262e
b6eb1d5
3e938a9
6f96bc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -562,6 +562,9 @@ ERROR(cannot_return_value_from_void_func,none, | |
|
||
ERROR(sema_no_import,Fatal, | ||
"no such module '%0'", (StringRef)) | ||
ERROR(sema_no_import_arch,Fatal, | ||
"could not find module '%0' for architecture '%1' " | ||
"found: %2", (StringRef, StringRef, StringRef)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this touch of including the architectures that are present! Good thinking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @brentdax's suggestion 😄 |
||
ERROR(sema_no_import_repl,none, | ||
"no such module '%0'", (StringRef)) | ||
NOTE(sema_no_import_no_sdk,none, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,12 +81,48 @@ openModuleFiles(StringRef DirName, StringRef ModuleFilename, | |
return std::error_code(); | ||
} | ||
|
||
static void addDiagnosticInfoForArchitectureMismatch(ASTContext &ctx, | ||
SourceLoc sourceLocation, | ||
StringRef moduleName, | ||
StringRef archName, | ||
StringRef directoryPath) { | ||
|
||
std::error_code errorCode; | ||
llvm::sys::fs::directory_iterator directoryIterator(directoryPath, errorCode, | ||
true); | ||
llvm::sys::fs::directory_iterator endIterator; | ||
|
||
if (errorCode) { | ||
return; | ||
} | ||
|
||
std::string foundArchs; | ||
for (; directoryIterator != endIterator; | ||
directoryIterator.increment(errorCode)) { | ||
if (errorCode) { | ||
return; | ||
} | ||
auto entry = *directoryIterator; | ||
StringRef filePath(entry.path()); | ||
StringRef extension = llvm::sys::path::extension(filePath); | ||
if (extension.startswith(".") && | ||
extension.drop_front() == SERIALIZED_MODULE_EXTENSION) { | ||
foundArchs = foundArchs + (foundArchs.length() > 0 ? ", " : "") + | ||
llvm::sys::path::stem(filePath).str(); | ||
} | ||
} | ||
|
||
ctx.Diags.diagnose(sourceLocation, diag::sema_no_import_arch, moduleName, | ||
archName, foundArchs); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @brentdax @CodaFi : I see 2 problems with the current implementation: foundArchs = foundArchs + (foundArchs.length() > 0 ? ", " : "")
+ llvm::sys::path::stem(filePath).str(); might not be the most optimal was of appending strings
Pretty new to the codebase, so any suggestions will be helpful. |
||
|
||
static bool | ||
findModule(ASTContext &ctx, AccessPathElem moduleID, | ||
std::unique_ptr<llvm::MemoryBuffer> *moduleBuffer, | ||
std::unique_ptr<llvm::MemoryBuffer> *moduleDocBuffer, | ||
bool &isFramework) { | ||
llvm::SmallString<64> moduleFilename(moduleID.first.str()); | ||
llvm::SmallString<64> moduleName(moduleID.first.str()); | ||
llvm::SmallString<64> moduleFilename(moduleName); | ||
moduleFilename += '.'; | ||
moduleFilename += SERIALIZED_MODULE_EXTENSION; | ||
|
||
|
@@ -96,9 +132,10 @@ findModule(ASTContext &ctx, AccessPathElem moduleID, | |
|
||
// FIXME: Which name should we be using here? Do we care about CPU subtypes? | ||
// FIXME: At the very least, don't hardcode "arch". | ||
llvm::SmallString<16> archFile{ | ||
llvm::SmallString<16> archName{ | ||
ctx.LangOpts.getPlatformConditionValue(PlatformConditionKind::Arch)}; | ||
llvm::SmallString<16> archDocFile{archFile}; | ||
llvm::SmallString<16> archFile{archName}; | ||
llvm::SmallString<16> archDocFile{archName}; | ||
if (!archFile.empty()) { | ||
archFile += '.'; | ||
archFile += SERIALIZED_MODULE_EXTENSION; | ||
|
@@ -122,6 +159,11 @@ findModule(ASTContext &ctx, AccessPathElem moduleID, | |
archFile.str(), archDocFile.str(), | ||
moduleBuffer, moduleDocBuffer, | ||
scratch); | ||
|
||
if(err == std::errc::no_such_file_or_directory) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (still missing a space here) |
||
addDiagnosticInfoForArchitectureMismatch( | ||
ctx, moduleID.second, moduleName, archName, currPath); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm. I think we still do need to exit early, though (here and below). Otherwise we could go on to a different search path and find a different module with the same name. (Yes, it's still possible for a different module loader to find another module with the same name right now, but I think it's okay not to worry about that at the moment.) |
||
} | ||
} | ||
if (!err) | ||
return true; | ||
|
@@ -134,12 +176,24 @@ findModule(ASTContext &ctx, AccessPathElem moduleID, | |
|
||
auto tryFrameworkImport = [&](StringRef frameworkPath) -> bool { | ||
currPath = frameworkPath; | ||
llvm::sys::path::append(currPath, moduleFramework.str(), | ||
llvm::sys::path::append(currPath, moduleFramework.str()); | ||
// Check if the framework directory exists | ||
if (!llvm::sys::fs::is_directory(currPath)) { | ||
return false; | ||
} | ||
|
||
llvm::sys::path::append(currPath, | ||
"Modules", moduleFilename.str()); | ||
auto err = openModuleFiles(currPath, | ||
archFile.str(), archDocFile.str(), | ||
moduleBuffer, moduleDocBuffer, | ||
scratch); | ||
|
||
if (err == std::errc::no_such_file_or_directory) { | ||
addDiagnosticInfoForArchitectureMismatch( | ||
ctx, moduleID.second, moduleName, archName, currPath); | ||
} | ||
|
||
return !err; | ||
}; | ||
|
||
|
@@ -642,4 +696,4 @@ SerializedASTFile::getDiscriminatorForPrivateValue(const ValueDecl *D) const { | |
Identifier discriminator = File.getDiscriminatorForPrivateValue(D); | ||
assert(!discriminator.empty() && "no discriminator found for value"); | ||
return discriminator; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
|
||
// RUN: %empty-directory(%t) | ||
// RUN: mkdir %t/new_module.swiftmodule | ||
// RUN: touch %t/new_module.swiftmodule/arm64.swiftmodule | ||
// RUN: touch %t/new_module.swiftmodule/powerpc64.swiftmodule | ||
// RUN: touch %t/new_module.swiftmodule/arm64.swiftdoc | ||
// RUN: touch %t/new_module.swiftmodule/powerpc64.swiftdoc | ||
// RUN: %target-swift-frontend %s -typecheck -I %t -verify -show-diagnostics-after-fatal | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
|
||
// RUN: %empty-directory(%t) | ||
// RUN: mkdir -p %t/new_module.framework/Modules/new_module.swiftmodule/ | ||
// RUN: touch %t/new_module.framework/Modules/new_module.swiftmodule/powerpc64.swiftmodule | ||
// RUN: touch %t/new_module.framework/Modules/new_module.swiftmodule/arm64.swiftmodule | ||
// RUN: %target-swift-frontend %s -F %t -typecheck -I %t -verify -show-diagnostics-after-fatal | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can drop the |
||
|
||
import new_module // expected-error {{no such module 'new_module'}} expected-error {{could not find module 'new_module' for architecture 'x86_64' found: powerpc64, arm64}} | ||
|
||
new_module.foo() // expected-error {{use of unresolved identifier 'new_module'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: can you add a semicolon after the
'%1'
here?