Skip to content

Revert "Flatten nested optionals resulting from try? and optional sub-expressions" #20645

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

Closed
wants to merge 1 commit into from
Closed

Revert "Flatten nested optionals resulting from try? and optional sub-expressions" #20645

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
6 changes: 0 additions & 6 deletions include/swift/Migrator/ASTMigratorPass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,6 @@ void runAPIDiffMigratorPass(EditorAdapter &Editor,
SourceFile *SF,
const MigratorOptions &Opts);

/// Run a pass to fix up the new type of 'try?' in Swift 4
void runOptionalTryMigratorPass(EditorAdapter &Editor,
SourceFile *SF,
const MigratorOptions &Opts);


} // end namespace migrator
} // end namespace swift

Expand Down
22 changes: 8 additions & 14 deletions lib/AST/ASTVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1958,21 +1958,15 @@ class Verifier : public ASTWalker {
void verifyChecked(OptionalTryExpr *E) {
PrettyStackTraceExpr debugStack(Ctx, "verifying OptionalTryExpr", E);

if (Ctx.LangOpts.isSwiftVersionAtLeast(5)) {
checkSameType(E->getType(), E->getSubExpr()->getType(),
"OptionalTryExpr and sub-expression");
}
else {
Type unwrappedType = E->getType()->getOptionalObjectType();
if (!unwrappedType) {
Out << "OptionalTryExpr result type is not optional\n";
abort();
}

checkSameType(unwrappedType, E->getSubExpr()->getType(),
"OptionalTryExpr and sub-expression");
Type unwrappedType = E->getType()->getOptionalObjectType();
if (!unwrappedType) {
Out << "OptionalTryExpr result type is not optional\n";
abort();
}


checkSameType(unwrappedType, E->getSubExpr()->getType(),
"OptionalTryExpr and sub-expression");

verifyCheckedBase(E);
}

Expand Down
7 changes: 0 additions & 7 deletions lib/AST/Expr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1821,13 +1821,6 @@ RebindSelfInConstructorExpr::getCalledConstructor(bool &isChainToSuper) const {
candidate = covariantExpr->getSubExpr();
continue;
}

// Look through inject into optional expressions
if (auto injectIntoOptionalExpr
= dyn_cast<InjectIntoOptionalExpr>(candidate)) {
candidate = injectIntoOptionalExpr->getSubExpr();
continue;
}
break;
}

Expand Down
1 change: 0 additions & 1 deletion lib/Migrator/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ add_swift_host_library(swiftMigrator STATIC
FixitApplyDiagnosticConsumer.cpp
Migrator.cpp
MigrationState.cpp
OptionalTryMigratorPass.cpp
RewriteBufferEditsReceiver.cpp
LINK_LIBRARIES swiftSyntax swiftIDE)

Expand Down
2 changes: 0 additions & 2 deletions lib/Migrator/Migrator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,6 @@ bool Migrator::performSyntacticPasses() {

runAPIDiffMigratorPass(Editor, StartInstance->getPrimarySourceFile(),
getMigratorOptions());
runOptionalTryMigratorPass(Editor, StartInstance->getPrimarySourceFile(),
getMigratorOptions());

Edits.commit(Editor.getEdits());

Expand Down
96 changes: 0 additions & 96 deletions lib/Migrator/OptionalTryMigratorPass.cpp
View file
Open in desktop

This file was deleted.

44 changes: 13 additions & 31 deletions lib/SILGen/SILGenExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1082,9 +1082,6 @@ RValue RValueEmitter::visitForceTryExpr(ForceTryExpr *E, SGFContext C) {
RValue RValueEmitter::visitOptionalTryExpr(OptionalTryExpr *E, SGFContext C) {
// FIXME: Much of this was copied from visitOptionalEvaluationExpr.

// Prior to Swift 5, an optional try's subexpression is always wrapped in an additional optional
bool shouldWrapInOptional = !(SGF.getASTContext().LangOpts.isSwiftVersionAtLeast(5));

auto &optTL = SGF.getTypeLowering(E->getType());

Initialization *optInit = C.getEmitInto();
Expand Down Expand Up @@ -1115,27 +1112,16 @@ RValue RValueEmitter::visitOptionalTryExpr(OptionalTryExpr *E, SGFContext C) {
JumpDest(catchBB, SGF.Cleanups.getCleanupsDepth(), E)};

SILValue branchArg;
if (shouldWrapInOptional) {
if (isByAddress) {
assert(optInit);
SILValue optAddr = optInit->getAddressForInPlaceInitialization(SGF, E);
SGF.emitInjectOptionalValueInto(E, E->getSubExpr(), optAddr, optTL);
} else {
ManagedValue subExprValue = SGF.emitRValueAsSingleValue(E->getSubExpr());
ManagedValue wrapped = SGF.getOptionalSomeValue(E, subExprValue, optTL);
branchArg = wrapped.forward(SGF);
}
}
else {
if (isByAddress) {
assert(optInit);
SGF.emitExprInto(E->getSubExpr(), optInit);
} else {
ManagedValue subExprValue = SGF.emitRValueAsSingleValue(E->getSubExpr());
branchArg = subExprValue.forward(SGF);
}
if (isByAddress) {
assert(optInit);
SILValue optAddr = optInit->getAddressForInPlaceInitialization(SGF, E);
SGF.emitInjectOptionalValueInto(E, E->getSubExpr(), optAddr, optTL);
} else {
ManagedValue subExprValue = SGF.emitRValueAsSingleValue(E->getSubExpr());
ManagedValue wrapped = SGF.getOptionalSomeValue(E, subExprValue, optTL);
branchArg = wrapped.forward(SGF);
}

localCleanups.pop();

// If it turns out there are no uses of the catch block, just drop it.
Expand All @@ -1147,10 +1133,8 @@ RValue RValueEmitter::visitOptionalTryExpr(OptionalTryExpr *E, SGFContext C) {
if (!isByAddress)
return RValue(SGF, E,
SGF.emitManagedRValueWithCleanup(branchArg, optTL));

if (shouldWrapInOptional) {
optInit->finishInitialization(SGF);
}

optInit->finishInitialization(SGF);

// If we emitted into the provided context, we're done.
if (usingProvidedContext)
Expand Down Expand Up @@ -1197,10 +1181,8 @@ RValue RValueEmitter::visitOptionalTryExpr(OptionalTryExpr *E, SGFContext C) {
return RValue(SGF, E, SGF.emitManagedRValueWithCleanup(arg, optTL));
}

if (shouldWrapInOptional) {
optInit->finishInitialization(SGF);
}

optInit->finishInitialization(SGF);

// If we emitted into the provided context, we're done.
if (usingProvidedContext)
return RValue::forInContext();
Expand Down
33 changes: 1 addition & 32 deletions lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2810,38 +2810,7 @@ namespace {
}

Expr *visitOptionalTryExpr(OptionalTryExpr *expr) {
// Prior to Swift 5, 'try?' simply wraps the type of its sub-expression
// in an Optional, regardless of the sub-expression type.
//
// In Swift 5+, the type of a 'try?' expression of static type T is:
// - Equal to T if T is optional
// - Equal to T? if T is not optional
//
// The result is that in Swift 5, 'try?' avoids producing nested optionals.

if (!cs.getTypeChecker().getLangOpts().isSwiftVersionAtLeast(5)) {
// Nothing to do for Swift 4 and earlier!
return simplifyExprType(expr);
}

Type subExprType = cs.getType(expr->getSubExpr());
Type targetType = simplifyType(subExprType);

// If the subexpression is not optional, wrap it in
// an InjectIntoOptionalExpr. Then use the type of the
// subexpression as the type of the 'try?' expr
bool subExprIsOptional = (bool) subExprType->getOptionalObjectType();

if (!subExprIsOptional) {
targetType = OptionalType::get(targetType);
auto subExpr = coerceToType(expr->getSubExpr(), targetType,
cs.getConstraintLocator(expr));
if (!subExpr) return nullptr;
expr->setSubExpr(subExpr);
}

cs.setType(expr, targetType);
return expr;
return simplifyExprType(expr);
}

Expr *visitParenExpr(ParenExpr *expr) {
Expand Down
20 changes: 4 additions & 16 deletions lib/Sema/CSDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,22 +576,10 @@ bool MissingOptionalUnwrapFailure::diagnoseAsError() {
auto *tryExpr = dyn_cast<OptionalTryExpr>(unwrapped);
if (!tryExpr)
return diagnoseUnwrap(getConstraintSystem(), unwrapped, type);

bool isSwift5OrGreater = getTypeChecker().getLangOpts().isSwiftVersionAtLeast(5);
auto subExprType = getType(tryExpr->getSubExpr());
bool subExpressionIsOptional = (bool)subExprType->getOptionalObjectType();


if (isSwift5OrGreater && subExpressionIsOptional) {
// Using 'try!' won't change the type for a 'try?' with an optional sub-expr
// under Swift 5+, so just report that a missing unwrap can't be handled here.
return false;
}
else {
emitDiagnostic(tryExpr->getTryLoc(), diag::missing_unwrap_optional_try, type)
.fixItReplace({tryExpr->getTryLoc(), tryExpr->getQuestionLoc()}, "try!");
return true;
}

emitDiagnostic(tryExpr->getTryLoc(), diag::missing_unwrap_optional_try, type)
.fixItReplace({tryExpr->getTryLoc(), tryExpr->getQuestionLoc()}, "try!");
return true;
}

bool RValueTreatedAsLValueFailure::diagnoseAsError() {
Expand Down
15 changes: 3 additions & 12 deletions lib/Sema/CSGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1709,18 +1709,9 @@ namespace {
if (!optTy)
return Type();

// Prior to Swift 5, 'try?' always adds an additional layer of optionality,
// even if the sub-expression was already optional.
if (CS.getTypeChecker().getLangOpts().isSwiftVersionAtLeast(5)) {
CS.addConstraint(ConstraintKind::Conversion,
CS.getType(expr->getSubExpr()), optTy,
CS.getConstraintLocator(expr));
}
else {
CS.addConstraint(ConstraintKind::OptionalObject,
optTy, CS.getType(expr->getSubExpr()),
CS.getConstraintLocator(expr));
}
CS.addConstraint(ConstraintKind::OptionalObject,
optTy, CS.getType(expr->getSubExpr()),
CS.getConstraintLocator(expr));
return optTy;
}

Expand Down
14 changes: 0 additions & 14 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2401,20 +2401,6 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
forceUnwrapPossible = false;
}
}

if (auto optTryExpr =
dyn_cast_or_null<OptionalTryExpr>(locator.trySimplifyToExpr())) {
auto subExprType = getType(optTryExpr->getSubExpr());
bool isSwift5OrGreater = TC.getLangOpts().isSwiftVersionAtLeast(5);
if (isSwift5OrGreater && (bool)subExprType->getOptionalObjectType()) {
// For 'try?' expressions, a ForceOptional fix converts 'try?'
// to 'try!'. If the sub-expression is optional, then a force-unwrap
// won't change anything in Swift 5+ because 'try?' already avoids
// adding an additional layer of Optional there.
forceUnwrapPossible = false;
}
}


if (forceUnwrapPossible) {
conversionsOrFixes.push_back(
Expand Down
Loading