Skip to content

Implement SE-0075: CanImport #11613

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
Aug 31, 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
3 changes: 3 additions & 0 deletions include/swift/AST/ASTContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,9 @@ class ASTContext {
llvm::DenseMap<const Pattern *, DeclContext *>
DelayedPatternContexts;

/// Cache of module names that fail the 'canImport' test in this context.
llvm::SmallPtrSet<Identifier, 8> FailedModuleImportNames;

public:
/// \brief Retrieve the allocator for the given arena.
llvm::BumpPtrAllocator &
Expand Down
6 changes: 3 additions & 3 deletions include/swift/Basic/LangOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ namespace swift {
Endianness,
/// Runtime support (_ObjC or _Native)
Runtime,
/// Conditional import of module
CanImport,
};
enum { NumPlatformConditionKind = 4 };

/// Describes which Swift 3 Objective-C inference warnings should be
/// emitted.
Expand Down Expand Up @@ -341,8 +342,7 @@ namespace swift {
}

private:
llvm::SmallVector<std::pair<PlatformConditionKind, std::string>,
NumPlatformConditionKind>
llvm::SmallVector<std::pair<PlatformConditionKind, std::string>, 4>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit unfortunate to lose the named constant here...

Copy link
Contributor Author

@CodaFi CodaFi Aug 25, 2017

Choose a reason for hiding this comment

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

Maybe, but I'm not really sure what it buys us outside of this one place.

PlatformConditionValues;
llvm::SmallVector<std::string, 2> CustomConditionalCompilationFlags;
};
Expand Down
4 changes: 3 additions & 1 deletion include/swift/Frontend/Frontend.h
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,9 @@ class CompilerInstance {

/// Parses the input file but does no type-checking or module imports.
/// Note that this only supports parsing an invocation with a single file.
void performParseOnly();
///
///
void performParseOnly(bool EvaluateConditionals = false);

/// Frees up the ASTContext and SILModule objects that this instance is
/// holding on.
Expand Down
4 changes: 3 additions & 1 deletion include/swift/Parse/PersistentParserState.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,9 @@ class PersistentParserState {
};

bool InPoundLineEnvironment = false;

// FIXME: When condition evaluation moves to a later phase, remove this bit
// and adjust the client call 'performParseOnly'.
bool PerformConditionEvaluation = true;
private:
ScopeInfo ScopeInfo;
typedef llvm::DenseMap<AbstractFunctionDecl *,
Expand Down
6 changes: 6 additions & 0 deletions lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1458,12 +1458,18 @@ bool ASTContext::canImportModule(std::pair<Identifier, SourceLoc> ModulePath) {
if (getLoadedModule(ModulePath) != nullptr)
return true;

// If we've failed loading this module before, don't look for it again.
if (FailedModuleImportNames.count(ModulePath.first))
return false;

// Otherwise, ask the module loaders.
for (auto &importer : Impl.ModuleLoaders) {
if (importer->canImportModule(ModulePath)) {
return true;
}
}

FailedModuleImportNames.insert(ModulePath.first);
return false;
}

Expand Down
4 changes: 4 additions & 0 deletions lib/Basic/LangOptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ checkPlatformConditionSupported(PlatformConditionKind Kind, StringRef Value,
case PlatformConditionKind::Runtime:
return contains(SupportedConditionalCompilationRuntimes, Value,
suggestions);
case PlatformConditionKind::CanImport:
// All importable names are valid.
// FIXME: Perform some kind of validation of the string?
return true;
}
llvm_unreachable("Unhandled enum value");
}
Expand Down
3 changes: 2 additions & 1 deletion lib/Frontend/Frontend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ void CompilerInstance::performSema() {
finishTypeChecking(*SF);
}

void CompilerInstance::performParseOnly() {
void CompilerInstance::performParseOnly(bool EvaluateConditionals) {
const InputFileKind Kind = Invocation.getInputKind();
ModuleDecl *MainModule = getMainModule();
Context->LoadedModules[MainModule->getName()] = MainModule;
Expand All @@ -608,6 +608,7 @@ void CompilerInstance::performParseOnly() {
}

PersistentParserState PersistentState;
PersistentState.PerformConditionEvaluation = EvaluateConditionals;
// Parse all the library files.
for (auto BufferID : BufferIDs) {
if (BufferID == MainBufferID)
Expand Down
18 changes: 13 additions & 5 deletions lib/Parse/ParseIfConfig.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ Optional<PlatformConditionKind> getPlatformConditionKind(StringRef Name) {
.Case("arch", PlatformConditionKind::Arch)
.Case("_endian", PlatformConditionKind::Endianness)
.Case("_runtime", PlatformConditionKind::Runtime)
.Case("canImport", PlatformConditionKind::CanImport)
.Default(None);
}

Expand Down Expand Up @@ -289,7 +290,7 @@ class ValidateIfConfigCondition :
return E;
}

// ( 'os' | 'arch' | '_endian' | '_runtime' ) '(' identifier ')''
// ( 'os' | 'arch' | '_endian' | '_runtime' | 'canImport') '(' identifier ')''
auto Kind = getPlatformConditionKind(*KindName);
if (!Kind.hasValue()) {
D.diagnose(E->getLoc(), diag::unsupported_platform_condition_expression);
Expand Down Expand Up @@ -331,6 +332,8 @@ class ValidateIfConfigCondition :
DiagName = "architecture"; break;
case PlatformConditionKind::Endianness:
DiagName = "endianness"; break;
case PlatformConditionKind::CanImport:
DiagName = "import conditional"; break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this testable?

Copy link
Contributor Author

@CodaFi CodaFi Aug 29, 2017

Choose a reason for hiding this comment

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

As of now, no. There's no validation of the module name being performed, so there's no way this will get hit (see the FIXME).

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, just checking.

case PlatformConditionKind::Runtime:
llvm_unreachable("handled above");
}
Expand Down Expand Up @@ -450,6 +453,9 @@ class EvaluateIfConfigCondition :
Str, SourceLoc(), nullptr).getValue();
auto thisVersion = Ctx.LangOpts.EffectiveLanguageVersion;
return thisVersion >= Val;
} else if (KindName == "canImport") {
auto Str = extractExprSource(Ctx.SourceMgr, Arg);
return Ctx.canImportModule({ Ctx.getIdentifier(Str) , E->getLoc() });
}

auto Val = getDeclRefStr(Arg);
Expand Down Expand Up @@ -567,9 +573,10 @@ ParserResult<IfConfigDecl> Parser::parseIfConfig(
Expr *Condition = nullptr;
bool isActive = false;

// Parse and evaluate the directive.
// Parse the condition. Evaluate it to determine the active
// clause unless we're doing a parse-only pass.
if (isElse) {
isActive = !foundActive;
isActive = !foundActive && State->PerformConditionEvaluation;
} else {
llvm::SaveAndRestore<bool> S(InPoundIfEnvironment, true);
ParserResult<Expr> Result = parseExprSequence(diag::expected_expr,
Expand All @@ -582,8 +589,9 @@ ParserResult<IfConfigDecl> Parser::parseIfConfig(
// Error in the condition;
isActive = false;
isVersionCondition = false;
} else if (!foundActive) {
// Evaluate the condition only if we haven't found any active one.
} else if (!foundActive && State->PerformConditionEvaluation) {
// Evaluate the condition only if we haven't found any active one and
// we're not in parse-only mode.
isActive = evaluateIfConfigCondition(Condition, Context);
isVersionCondition = isVersionIfConfigCondition(Condition);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -I %S/../Inputs/custom-modules -typecheck %s -verify

// REQUIRES: objc_interop
// REQUIRES: can_import

// Test that 'canImport(Foo)' directives do not open symbols from 'Foo' into the
// current module. Only an 'import Foo' statement should do this.
Expand Down
1 change: 0 additions & 1 deletion test/ClangImporter/can_import_missing_requirement.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -I %S/Inputs/missing-requirement %s -verify

// REQUIRES: objc_interop
// REQUIRES: can_import

class Unique {}

Expand Down
2 changes: 0 additions & 2 deletions test/IDE/print_ast_non_typechecked.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,3 @@ func qux() {}
// CHECK1: {{^}} func qux() {
// CHECK1: {{^}} }
// CHECK1: {{^}}#endif
// CHECK1: {{^}}func qux() {
// CHECK1: {{^}}}
2 changes: 0 additions & 2 deletions test/Parse/ConditionalCompilation/can_import.swift
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// RUN: %target-typecheck-verify-swift -parse-as-library
// RUN: %target-typecheck-verify-swift -D WITH_PERFORM -primary-file %s %S/Inputs/can_import_nonprimary_file.swift

// REQUIRES: can_import

public struct LibraryDependentBool : ExpressibleByBooleanLiteral {
#if canImport(Swift)
var _description: String
Expand Down
2 changes: 0 additions & 2 deletions test/Parse/ConditionalCompilation/can_import_idempotent.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
// RUN: echo "public var dummyVar = Int()" | %target-swift-frontend -module-name DummyModule -emit-module -o %t -
// RUN: %target-swift-frontend -typecheck -verify -I %t %s

// REQUIRES: can_import

#if canImport(DummyModule)
print(DummyModule.dummyVar) // expected-error {{use of unresolved identifier 'DummyModule'}}
print(dummyVar) // expected-error {{use of unresolved identifier 'dummyVar'}}
Expand Down
1 change: 0 additions & 1 deletion test/Parse/ConditionalCompilation/can_import_sdk.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -verify %s

// REQUIRES: objc_interop
// REQUIRES: can_import

class Unique {}

Expand Down
2 changes: 1 addition & 1 deletion tools/SourceKit/lib/SwiftLang/SwiftDocSupport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1366,7 +1366,7 @@ SourceFile *SwiftLangSupport::getSyntacticSourceFile(
Error = "Compiler invocation set up failed";
return nullptr;
}
ParseCI.performParseOnly();
ParseCI.performParseOnly(/*EvaluateConditionals*/true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benlangmuir Is this correct? Should this be a parameter to this function? Its callers are the 'syntactic rename' and the 'find rename ranges' actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a fine stopgap to me.


SourceFile *SF = nullptr;
unsigned BufferID = ParseCI.getInputBufferIDs().back();
Expand Down
2 changes: 1 addition & 1 deletion tools/swift-refactor/swift-refactor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ int main(int argc, char *argv[]) {
switch (options::Action) {
case RefactoringKind::GlobalRename:
case RefactoringKind::FindGlobalRenameRanges:
CI.performParseOnly();
CI.performParseOnly(/*EvaluateConditionals*/true);
break;
default:
CI.performSema();
Expand Down