Skip to content

[CSDiagnostics] Diagnose invalid optional unwrap via fixes #21043

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 2 commits into from
Dec 6, 2018
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
14 changes: 14 additions & 0 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1226,3 +1226,17 @@ bool AutoClosureForwardingFailure::diagnoseAsError() {
.fixItInsertAfter(argExpr->getEndLoc(), "()");
return true;
}

bool NonOptionalUnwrapFailure::diagnoseAsError() {
auto *anchor = getAnchor();

auto diagnostic = diag::invalid_optional_chain;
if (isa<ForceValueExpr>(anchor))
diagnostic = diag::invalid_force_unwrap;

emitDiagnostic(anchor->getLoc(), diagnostic, BaseType)
.highlight(anchor->getSourceRange())
.fixItRemove(anchor->getEndLoc());

return true;
}
23 changes: 23 additions & 0 deletions lib/Sema/CSDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,29 @@ class AutoClosureForwardingFailure final : public FailureDiagnostic {
bool diagnoseAsError() override;
};

/// Diagnose situations when there was an attempt to unwrap entity
/// of non-optional type e.g.
///
/// ```swift
/// let i: Int = 0
/// _ = i!
///
/// struct A { func foo() {} }
/// func foo(_ a: A) {
/// a?.foo()
/// }
/// ```
class NonOptionalUnwrapFailure final : public FailureDiagnostic {
Type BaseType;

public:
NonOptionalUnwrapFailure(Expr *root, ConstraintSystem &cs, Type baseType,
ConstraintLocator *locator)
: FailureDiagnostic(root, cs, locator), BaseType(baseType) {}

bool diagnoseAsError() override;
};

} // end namespace constraints
} // end namespace swift

Expand Down
11 changes: 11 additions & 0 deletions lib/Sema/CSFix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -213,3 +213,14 @@ AutoClosureForwarding *AutoClosureForwarding::create(ConstraintSystem &cs,
ConstraintLocator *locator) {
return new (cs.getAllocator()) AutoClosureForwarding(cs, locator);
}

bool RemoveUnwrap::diagnose(Expr *root, bool asNote) const {
auto failure = NonOptionalUnwrapFailure(root, getConstraintSystem(), BaseType,
getLocator());
return failure.diagnose(asNote);
}

RemoveUnwrap *RemoveUnwrap::create(ConstraintSystem &cs, Type baseType,
ConstraintLocator *locator) {
return new (cs.getAllocator()) RemoveUnwrap(cs, baseType, locator);
}
20 changes: 20 additions & 0 deletions lib/Sema/CSFix.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ enum class FixKind : uint8_t {
/// @autoclosure conversions are unsupported starting from
/// Swift version 5.
AutoClosureForwarding,

/// Remove `!` or `?` because base is not an optional type.
RemoveUnwrap,
};

class ConstraintFix {
Expand Down Expand Up @@ -408,6 +411,23 @@ class AutoClosureForwarding final : public ConstraintFix {
ConstraintLocator *locator);
};

class RemoveUnwrap final : public ConstraintFix {
Type BaseType;

public:
RemoveUnwrap(ConstraintSystem &cs, Type baseType, ConstraintLocator *locator)
: ConstraintFix(cs, FixKind::RemoveUnwrap, locator), BaseType(baseType) {}

std::string getName() const override {
return "remove unwrap operator `!` or `?`";
}

bool diagnose(Expr *root, bool asNote = false) const override;

static RemoveUnwrap *create(ConstraintSystem &cs, Type baseType,
ConstraintLocator *locator);
};

} // end namespace constraints
} // end namespace swift

Expand Down
24 changes: 21 additions & 3 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2979,10 +2979,27 @@ ConstraintSystem::simplifyOptionalObjectConstraint(
return SolutionKind::Unsolved;
}

// If the base type is not optional, the constraint fails.

Type objectTy = optTy->getOptionalObjectType();
if (!objectTy)
return SolutionKind::Error;
// If the base type is not optional, let's attempt a fix (if possible)
// and assume that `!` is just not there.
if (!objectTy) {
// Let's see if we can apply a specific fix here.
if (shouldAttemptFixes()) {
auto *fix =
RemoveUnwrap::create(*this, optTy, getConstraintLocator(locator));

if (recordFix(fix))
return SolutionKind::Error;

// If the fix was successful let's record
// "fixed" object type and continue.
objectTy = optTy;
} else {
// If fixes are not allowed, no choice but to fail.
return SolutionKind::Error;
}
}

// The object type is an lvalue if the optional was.
if (optLValueTy->is<LValueType>())
Expand Down Expand Up @@ -5427,6 +5444,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
case FixKind::RelabelArguments:
case FixKind::AddConformance:
case FixKind::AutoClosureForwarding:
case FixKind::RemoveUnwrap:
llvm_unreachable("handled elsewhere");
}

Expand Down
8 changes: 8 additions & 0 deletions test/Constraints/diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1183,6 +1183,14 @@ func rdar17170728() {

let _ = [i, j, k].reduce(0 as Int?) {
$0 && $1 ? $0! + $1! : ($0 ? $0! : ($1 ? $1! : nil))
// expected-error@-1 {{cannot force unwrap value of non-optional type 'Bool'}} {{18-19=}}
// expected-error@-2 {{cannot force unwrap value of non-optional type 'Bool'}} {{24-25=}}
// expected-error@-3 {{cannot force unwrap value of non-optional type 'Bool'}} {{36-37=}}
// expected-error@-4 {{cannot force unwrap value of non-optional type 'Bool'}} {{48-49=}}
}

let _ = [i, j, k].reduce(0 as Int?) {
$0 && $1 ? $0 + $1 : ($0 ? $0 : ($1 ? $1 : nil))
// expected-error@-1 {{ambiguous use of operator '+'}}
}
}
Expand Down
1 change: 0 additions & 1 deletion test/Constraints/dynamic_lookup.swift
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ type(of: obj).foo!(obj)(5) // expected-error{{instance member 'foo' cannot be us

// Checked casts to AnyObject
var p: P = Y()
// expected-warning @+1 {{forced cast from 'P' to 'AnyObject' always succeeds; did you mean to use 'as'?}}
var obj3 : AnyObject = (p as! AnyObject)! // expected-error{{cannot force unwrap value of non-optional type 'AnyObject'}} {{41-42=}}

// Implicit force of an implicitly unwrapped optional
Expand Down
10 changes: 10 additions & 0 deletions test/Constraints/optional.swift
Original file line number Diff line number Diff line change
Expand Up @@ -292,3 +292,13 @@ func se0213() {
_ = Q("who")!.foo // Ok
_ = Q?("how") // Ok
}

func rdar45218255(_ i: Int) {
struct S<T> {
init(_:[T]) {}
}

_ = i! // expected-error {{cannot force unwrap value of non-optional type 'Int'}} {{8-9=}}
_ = [i!] // expected-error {{cannot force unwrap value of non-optional type 'Int'}} {{9-10=}}
_ = S<Int>([i!]) // expected-error {{cannot force unwrap value of non-optional type 'Int'}} {{16-17=}}
}
7 changes: 5 additions & 2 deletions test/Parse/operators.swift
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,11 @@ infix operator !!
func !!(x: Man, y: Man) {}
let foo = Man()
let bar = TheDevil()
foo!!foo // expected-error{{cannot force unwrap value of non-optional type 'Man'}} {{4-5=}} expected-error{{consecutive statements}} {{6-6=;}}
// expected-warning @-1 {{expression of type 'Man' is unused}}
foo!!foo
// expected-error@-1 {{cannot force unwrap value of non-optional type 'Man'}} {{4-5=}}
// expected-error@-2 {{cannot force unwrap value of non-optional type 'Man'}} {{5-6=}}
// expected-error@-3 {{consecutive statements}} {{6-6=;}}
// expected-warning@-4 {{expression of type 'Man' is unused}}

foo??bar // expected-error{{broken standard library}} expected-error{{consecutive statements}} {{6-6=;}}
// expected-warning @-1 {{expression of type 'TheDevil' is unused}}
2 changes: 1 addition & 1 deletion test/decl/init/failable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class Sub : Super {
}

convenience init(forceNonfail: Int) {
self.init(nonfail: forceNonfail)! // expected-error{{cannot force unwrap value of non-optional type '()'}} {{37-38=}}
self.init(nonfail: forceNonfail)! // expected-error{{cannot force unwrap value of non-optional type 'Sub'}} {{37-38=}}
}

init(nonfail2: Int) { // okay, traps on nil
Expand Down