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

#if targetEnvironment(simulator) #12964

Merged
merged 4 commits into from Nov 29, 2017
File filter...
Filter file types
Jump to file or symbol
Failed to load files and symbols.
+127 −3
Diff settings

Always

Just for now

[Parse] Add fixit for targetEnvironment(simulator)

  • Loading branch information...
graydon committed Nov 16, 2017
commit d31bad45b8c8bdc866f3d9db4aae102144c6ef27
@@ -1503,6 +1503,9 @@ ERROR(empty_version_string,none,
WARNING(unknown_platform_condition_argument,none,
"unknown %0 for build configuration '%1'",
(StringRef, StringRef))
WARNING(likely_simulator_platform_condition,none,
"plaform condition appears to be testing for simulator environment",

This comment has been minimized.

@DougGregor

DougGregor Nov 17, 2017

Member

Could you tack on "; use 'targetEnvironment(simulator)' instead" ? That way, one doesn't have to look at the contents of the Fix-It to see what the fix is.

())

//------------------------------------------------------------------------------
// Availability query parsing diagnostics
Copy path View file
@@ -547,8 +547,97 @@ static bool isVersionIfConfigCondition(Expr *Condition) {
return IsVersionIfConfigCondition().visit(Condition);
}

/// Get the identifier string from an \c Expr if it's an
/// \c UnresolvedDeclRefExpr, otherwise the empty string.
static StringRef getDeclRefStr(Expr *E) {
if (auto *UDRE = cast<UnresolvedDeclRefExpr>(E)) {

This comment has been minimized.

@compnerd

compnerd Nov 29, 2017

Collaborator

Am I just mistaken, or shouldn't this be a dyn_cast? cast implies that this must succeed and will abort if the expression type is not a UnresolvedDeclRefExpr.

This comment has been minimized.

@graydon

graydon Nov 29, 2017

Contributor

Oh, yes, this is a typo, thanks! Will fix tomorrow.

return UDRE->getName().getBaseIdentifier().str();
}
return "";
}

static bool isPlatformConditionDisjunction(Expr *E, PlatformConditionKind Kind,
ArrayRef<StringRef> Vals) {
if (auto *Or = dyn_cast<BinaryExpr>(E)) {
if (getDeclRefStr(Or->getFn()) == "||") {
auto Args = Or->getArg()->getElements();
return (isPlatformConditionDisjunction(Args[0], Kind, Vals) &&
isPlatformConditionDisjunction(Args[1], Kind, Vals));
}
} else if (auto *P = dyn_cast<ParenExpr>(E)) {
return isPlatformConditionDisjunction(P->getSubExpr(), Kind, Vals);
} else if (auto *C = dyn_cast<CallExpr>(E)) {
if (getPlatformConditionKind(getDeclRefStr(C->getFn())) != Kind)
return false;
if (auto *ArgP = dyn_cast<ParenExpr>(C->getArg())) {
if (auto *Arg = ArgP->getSubExpr()) {
auto ArgStr = getDeclRefStr(Arg);
for (auto V : Vals) {
if (ArgStr == V)
return true;
}
}
}
}
return false;
}

// Search for the first occurrence of a _likely_ (but not definite) implicit
// simulator-environment platform condition, or negation thereof. This is
// defined as any logical conjunction of one or more os() platform conditions
// _strictly_ from the set {iOS, tvOS, watchOS} and one or more arch() platform
// conditions _strictly_ from the set {i386, x86_64}.
//
// These are (at the time of writing) defined as de-facto simulators in
// Platform.cpp, and if a user is testing them they're _likely_ looking for
// simulator-ness indirectly. If there is anything else in the condition aside
// from these conditions (or the negation of such a conjunction), we
// conservatively assume the user is testing something other than
// simulator-ness.
static Expr *findAnyLikelySimulatorEnvironmentTest(Expr *Condition) {

if (!Condition)
return nullptr;

if (auto *N = dyn_cast<PrefixUnaryExpr>(Condition)) {
return findAnyLikelySimulatorEnvironmentTest(N->getArg());
} else if (auto *P = dyn_cast<ParenExpr>(Condition)) {
return findAnyLikelySimulatorEnvironmentTest(P->getSubExpr());
}

// We assume the user is writing the condition in CNF -- say (os(iOS) ||
// os(tvOS)) && (arch(i386) || arch(x86_64)) -- rather than DNF, as the former
// is exponentially more terse, and these conditions are already quite
// unwieldy. If field evidence shows people using other variants, possibly add
// them here.

auto isSimulatorPlatformOSTest = [](Expr *E) -> bool {
return isPlatformConditionDisjunction(
E, PlatformConditionKind::OS, {"iOS", "tvOS", "watchOS"});
};

auto isSimulatorPlatformArchTest = [](Expr *E) -> bool {
return isPlatformConditionDisjunction(
E, PlatformConditionKind::Arch, {"i386", "x86_64"});
};

if (auto *And = dyn_cast<BinaryExpr>(Condition)) {
if (getDeclRefStr(And->getFn()) == "&&") {
auto Args = And->getArg()->getElements();
if ((isSimulatorPlatformOSTest(Args[0]) &&
isSimulatorPlatformArchTest(Args[1])) ||
(isSimulatorPlatformOSTest(Args[1]) &&
isSimulatorPlatformArchTest(Args[0]))) {
return And;
}
}
}
return nullptr;
}

} // end anonymous namespace


/// Parse and populate a #if ... #endif directive.
/// Delegate callback function to parse elements in the blocks.
ParserResult<IfConfigDecl> Parser::parseIfConfig(
@@ -597,6 +686,13 @@ ParserResult<IfConfigDecl> Parser::parseIfConfig(
diag::extra_tokens_conditional_compilation_directive);
}

if (Expr *Test = findAnyLikelySimulatorEnvironmentTest(Condition)) {
diagnose(Test->getLoc(),
diag::likely_simulator_platform_condition)
.fixItReplace(Test->getSourceRange(),
"targetEnvironment(simulator)");
}

// Parse elements
SmallVector<ASTNode, 16> Elements;
if (isActive || !isVersionCondition) {
@@ -1,6 +1,6 @@
// RUN: %swift -typecheck %s -verify -target x86_64-apple-ios7.0 -parse-stdlib
// RUN: %swift -typecheck %s -verify -target x86_64-unknown-linux-simulator -parse-stdlib
// RUN: %swift-ide-test -test-input-complete -source-filename=%s -target x86_64-apple-ios7.0
// RUN: %swift -swift-version 4 -typecheck %s -verify -target x86_64-apple-ios7.0 -parse-stdlib
// RUN: %swift -swift-version 4 -typecheck %s -verify -target x86_64-unknown-linux-simulator -parse-stdlib
// RUN: %swift-ide-test -swift-version 4 -test-input-complete -source-filename=%s -target x86_64-apple-ios7.0
#if !targetEnvironment(simulator)
// This block should not parse.
@@ -12,3 +12,28 @@ class C {}
var x = C()
#endif
var y = x

#if os(iOS) && arch(i386)
// expected-warning @-1 {{plaform condition appears to be testing for simulator environment}} {{5-26=targetEnvironment(simulator)}}
class C1 {}
#endif

#if arch(i386) && os(iOS)
// expected-warning @-1 {{plaform condition appears to be testing for simulator environment}} {{5-26=targetEnvironment(simulator)}}
class C2 {}
#endif

#if arch(i386) && (os(iOS) || os(watchOS))
// expected-warning @-1 {{plaform condition appears to be testing for simulator environment}} {{5-43=targetEnvironment(simulator)}}
class C3 {}
#endif

#if (arch(x86_64) || arch(i386)) && (os(iOS) || os(watchOS) || os(tvOS))
// expected-warning @-1 {{plaform condition appears to be testing for simulator environment}} {{5-73=targetEnvironment(simulator)}}
class C4 {}
#endif

#if !(arch(x86_64) && os(tvOS))
// expected-warning @-1 {{plaform condition appears to be testing for simulator environment}} {{7-31=targetEnvironment(simulator)}}
class C5 {}
#endif
ProTip! Use n and p to navigate between commits in a pull request.