Skip to content

[Sema] More correct optional chaining vs. force unwrap fixits #15321

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 8 commits into from
Closed

[Sema] More correct optional chaining vs. force unwrap fixits #15321

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d5ed794
Be smarter about optional chaining fixits. Previously if we didn't al…
gregomni Mar 17, 2018
e4da93d
- Formatting fixes.
gregomni Mar 18, 2018
4160528
Changed to use constraints with fixes in them rather than remembering…
gregomni Mar 18, 2018
df9bd22
comment change
gregomni Mar 23, 2018
606f009
Clean up the strategy for dealing with nested optional chain fixes by…
gregomni May 21, 2018
e2802e8
Merge remote-tracking branch 'apple/master' into opty
gregomni May 21, 2018
f0cee60
Review feedback
gregomni May 24, 2018
029526a
Typo correction
gregomni May 24, 2018
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
25 changes: 12 additions & 13 deletions lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7989,18 +7989,16 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
if (auto subscript = dyn_cast<SubscriptExpr>(affected))
affected = subscript->getBase();
}

switch (fix.first.getKind()) {
case FixKind::ForceOptional: {
const Expr *unwrapped = affected->getValueProvidingExpr();
auto type = solution.simplifyType(getType(affected))
->getRValueObjectType();
auto type = solution.simplifyType(getType(affected))->getRValueObjectType();

if (auto tryExpr = dyn_cast<OptionalTryExpr>(unwrapped)) {
TC.diagnose(tryExpr->getTryLoc(), diag::missing_unwrap_optional_try,
type)
.fixItReplace({tryExpr->getTryLoc(), tryExpr->getQuestionLoc()},
"try!");
TC.diagnose(tryExpr->getTryLoc(), diag::missing_unwrap_optional_try, type)
.fixItReplace({tryExpr->getTryLoc(), tryExpr->getQuestionLoc()},
"try!");

} else {
auto diag = TC.diagnose(affected->getLoc(),
Expand All @@ -8014,12 +8012,13 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
}
return true;
}

case FixKind::OptionalChaining: {
auto type = solution.simplifyType(getType(affected))
->getRValueObjectType();
auto diag = TC.diagnose(affected->getLoc(),
diag::missing_unwrap_optional, type);

case FixKind::OptionalChaining:
LLVM_FALLTHROUGH;
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need the LLVM_FALLTHROUGH here.

case FixKind::OptionalMember: {
auto type = solution.simplifyType(getType(affected))->getRValueObjectType();
auto diag =
TC.diagnose(affected->getLoc(), diag::missing_unwrap_optional, type);
diag.fixItInsertAfter(affected->getEndLoc(), "?");
return true;
}
Expand Down
137 changes: 110 additions & 27 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3455,6 +3455,73 @@ performMemberLookup(ConstraintKind constraintKind, DeclName memberName,
return result;
}

static bool fixOptionalBaseMemberConstraint(ConstraintSystem &cs,
Type unwrappedBaseObjTy,
DeclName member, Type memberTy,
DeclContext *useDC,
FunctionRefKind functionRefKind,
ArrayRef<OverloadChoice> outerAlternatives,
ConstraintLocator *locator) {
// Don't try to fixup multiple levels of optionality, if we unwrapped and
// the result is still optional, fail.
if (unwrappedBaseObjTy->getOptionalObjectType())
return false;

if (!(cs.Options & ConstraintSystemFlags::AllowFixes))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use cs.shouldAttemptFixes().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was intentional, but maybe there's a better way to fix it? Very open to suggestions.

The trouble is, that in order to get all the fixits at once in a multi-step dotted path that should be optional-chained, we have to be able to continue this code even while inside of a disjunction choice (the disjunction created for the first step in the dotted path). But trying a disjunction choice creates a new SolverScope that defaults recordFixes to false.

Stack frames:

0 fixOptionalBaseMemberConstraint()
...
8 DisjunctionChoice::solve()
9 ConstraintSystem::solveSimplified() <--- here is where the SolverScope is set
...
20 ConstraintSystem::diagnoseFailureForExpr()
21 ConstraintSystem::salvage()

How to get the information that recording fixes should be ok from salvage() all the way down to solveSimplified() is the trouble. There is also a FIXME in salvage() that says "can this be removed? We need to arrange for recordFixes to be eliminated". I'm not sure what this is referring to exactly, but if recordFixes could be eliminated entirely, what would be left would be just looking at ConstraintSystemFlags::AllowFixes, as here. Does it seem safe to do that? (Just eliminate recordFixes entirely?) Everything certainly appears to work correctly if I do that, but I may be unaware of all the effects.

So this code is hedging by ignoring recordFixes here, but not touching its use elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow. recordFixes is part of SolverState, not SolverScope.

return false;

auto contextTy = cs.getContextualType();
bool forceUnwrap;
if (contextTy)
forceUnwrap = contextTy->getOptionalObjectType().isNull();
else
forceUnwrap =
cs.Options.contains(ConstraintSystemFlags::PreferForceUnwrapToOptional);

if (forceUnwrap || functionRefKind != FunctionRefKind::Unapplied) {
// Note the fix.
auto fixKind =
forceUnwrap ? FixKind::ForceOptional : FixKind::OptionalChaining;
if (cs.recordFix(fixKind, locator))
return false;

// The simple case - just look through one level of optional.
cs.addValueMemberConstraint(unwrappedBaseObjTy, member, memberTy, useDC,
functionRefKind, outerAlternatives, locator);
return true;
}

auto innerTV =
cs.createTypeVariable(locator, TVO_CanBindToLValue | TVO_CanBindToInOut);
Type optTy = cs.getTypeChecker().getOptionalType(SourceLoc(), innerTV);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should pass a location, e.g. the one from locator->getAnchor().

if (!optTy)
return false;

// Add a disjunction with the three possibilities:
// 1) member is non-optional, result of expression adds optional to it ("?")
// 2) member is non-optional, result of expression is non-optional ("!")
// 3) member is optional, result of expression matches it ("?")
SmallVector<Constraint *, 3> optionalities;
auto optionalResult = Constraint::createFixed(cs, ConstraintKind::Conversion,
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 not clear to me why you are using ConstraintKind::Conversion in these three constraints rather than ConstraintKind::Equal.

Wouldn't the expectation be that the types are equal?

FixKind::OptionalChaining,
optTy, memberTy, locator);
auto nonoptionalResult =
Constraint::createFixed(cs, ConstraintKind::Conversion,
FixKind::ForceOptional, optTy, memberTy, locator);
auto optionalMember = Constraint::createFixed(cs, ConstraintKind::Conversion,
FixKind::OptionalMember,
innerTV, memberTy, locator);
optionalMember->setFavored();
optionalities.push_back(optionalResult);
optionalities.push_back(nonoptionalResult);
optionalities.push_back(optionalMember);
cs.addDisjunctionConstraint(optionalities, locator);

cs.addValueMemberConstraint(unwrappedBaseObjTy, member, innerTV, useDC,
functionRefKind, outerAlternatives, locator);
return true;
}

ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
ConstraintKind kind, Type baseTy, DeclName member, Type memberTy,
DeclContext *useDC, FunctionRefKind functionRefKind,
Expand Down Expand Up @@ -3504,38 +3571,23 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
if (!result.UnviableCandidates.empty())
return SolutionKind::Error;


// If the lookup found no hits at all (either viable or unviable), diagnose it
// as such and try to recover in various ways.

auto instanceTy = baseObjTy;
if (auto MTT = instanceTy->getAs<MetatypeType>())
instanceTy = MTT->getInstanceType();

// Value member lookup has some hacks too.
if (shouldAttemptFixes() && baseObjTy->getOptionalObjectType()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd really appreciate if the whole original block of code was removed from the simplifyMemberConstraint and new logic would be somehow integrated into CSDiag, I think it's better place to do this kind of work since we'd be able to evaluate optional chaining per overload choice without risking blowing out the stack and we'd know exactly where in the expression chain it is. Currently we have some logic to handle optional unwraps here and in CSDiag it would be great to unify them.

I'd be great if you could overload takeNon and takeOpt since that makes things more interesting.

// If the base type was an optional, look through it.

// Determine whether or not we want to provide an optional chaining fixit or
// a force unwrap fixit.
bool optionalChain;
if (!getContextualType())
optionalChain = !(Options & ConstraintSystemFlags::PreferForceUnwrapToOptional);
else
optionalChain = !getContextualType()->getOptionalObjectType().isNull();
auto fixKind = optionalChain ? FixKind::OptionalChaining : FixKind::ForceOptional;

// Note the fix.
if (recordFix(fixKind, locator))
return SolutionKind::Error;

// Look through one level of optional.
addValueMemberConstraint(baseObjTy->getOptionalObjectType(),
member, memberTy, useDC, functionRefKind,
outerAlternatives, locator);
// If the base type was an optional, look through it and try to generate
// fixes to make it valid.
auto unwrappedBaseObjTy = baseObjTy->getOptionalObjectType();
if (unwrappedBaseObjTy && fixOptionalBaseMemberConstraint(
*this, unwrappedBaseObjTy, member, memberTy,
useDC, functionRefKind, outerAlternatives,
locator))
return SolutionKind::Solved;
}
return SolutionKind::Error;
else
return SolutionKind::Error;
}

ConstraintSystem::SolutionKind
Expand Down Expand Up @@ -4814,7 +4866,7 @@ bool ConstraintSystem::recordFix(Fix fix, ConstraintLocatorBuilder locator) {

// Increase the score. If this would make the current solution worse than
// the best solution we've seen already, stop now.
increaseScore(SK_Fix);
increaseScore(SK_Fix, fix.scoreWeight());
if (worseThanBestSolution())
return true;

Expand Down Expand Up @@ -4842,8 +4894,7 @@ ConstraintSystem::simplifyFixConstraint(Fix fix, Type type1, Type type2,
TypeMatchOptions subflags =
getDefaultDecompositionOptions(flags) | TMF_ApplyingFix;
switch (fix.getKind()) {
case FixKind::ForceOptional:
case FixKind::OptionalChaining: {
case FixKind::ForceOptional: {
// Assume that '!' was applied to the first type.
auto result =
matchTypes(type1->getRValueObjectType()->getOptionalObjectType(), type2,
Expand All @@ -4854,6 +4905,38 @@ ConstraintSystem::simplifyFixConstraint(Fix fix, Type type1, Type type2,

return result;
}

case FixKind::OptionalChaining: {
// Types have already been munged, we just want to apply the fix or not
// based on them being solved.
auto result =
matchTypes(type1, type2, matchKind, subflags, locator);
if (result == SolutionKind::Solved)
if (recordFix(fix, locator))
return SolutionKind::Error;

return result;
}

case FixKind::OptionalMember: {
// The second type must result in an optional, and then can be matched
// to the first.
auto fixedType =
getFixedTypeRecursive(type2, flags, /* wantRValue= */ false,
/* retainParens= */ false);
if (fixedType->hasTypeVariable())
return SolutionKind::Unsolved;
if (!fixedType->getOptionalObjectType())
return SolutionKind::Error;

auto result = matchTypes(type1, type2, matchKind, subflags, locator);
if (result == SolutionKind::Solved)
if (recordFix(fix, locator))
return SolutionKind::Error;

return result;
}

case FixKind::ForceDowncast:
// These work whenever they are suggested.
if (recordFix(fix, locator))
Expand Down
2 changes: 2 additions & 0 deletions lib/Sema/Constraint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,8 @@ StringRef Fix::getName(FixKind kind) {
return "fix: force optional";
case FixKind::OptionalChaining:
return "fix: optional chaining";
case FixKind::OptionalMember:
return "fix: optional member";
case FixKind::ForceDowncast:
return "fix: force downcast";
case FixKind::AddressOf:
Expand Down
12 changes: 12 additions & 0 deletions lib/Sema/Constraint.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,11 @@ enum class FixKind : uint8_t {
/// Introduce a '?.' to begin optional chaining.
OptionalChaining,

/// Implicitly part of an optional chain where the member returns
/// an optional, so we don't want to wrap it in an additional level
/// of optionality.
OptionalMember,

/// Append 'as! T' to force a downcast to the specified type.
ForceDowncast,

Expand Down Expand Up @@ -276,6 +281,13 @@ class Fix {
/// If this fix has a type argument, retrieve it.
Type getTypeArgument(ConstraintSystem &cs) const;

/// Weight to put on scoring this fix in a potential solution.
unsigned scoreWeight() const {
// Prefer lots of other (optional chain) fixes to a single
// force optional, if solutions for both exist.
return getKind() == FixKind::ForceOptional ? 10 : 1;
}

/// Return a string representation of a fix.
static llvm::StringRef getName(FixKind kind);

Expand Down
83 changes: 83 additions & 0 deletions test/Constraints/fixes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,86 @@ struct S1116 {

let a1116: [S1116] = []
var s1116 = Set(1...10).subtracting(a1116.map({ $0.s })) // expected-error {{cannot convert value of type '[Int?]' to expected argument type 'Set<Int>'}}

func moreComplexUnwrapFixes() {
struct S { let value: Int }
struct T {
let s: S
let optS: S?
}
struct U {
let t: T
}
func takeNon(_ x: Int) -> Void {}
func takeOpt(_ x: Int?) -> Void {}

let s = S(value: 0)
let t: T? = T(s: s, optS: nil)
let u: U? = U(t: t!)
let os: S? = s

takeOpt(os.value) // expected-error{{value of optional type 'S?' not unwrapped; did you mean to use '!' or '?'?}} {{13-13=?}}
Copy link
Contributor

Choose a reason for hiding this comment

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

None of these tests are going to do nasty things like introduce disjuncts or throw an unbound-type-variable-wrench into your logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add some!

takeNon(os.value) // expected-error{{value of optional type 'S?' not unwrapped; did you mean to use '!' or '?'?}} {{13-13=!}}

takeOpt(t.s.value) // expected-error{{value of optional type 'T?' not unwrapped; did you mean to use '!' or '?'?}} {{12-12=?}}
// expected-error@-1 {{value of optional type 'S?' not unwrapped; did you mean to use '!' or '?'?}} {{14-14=?}}
takeNon(t.s.value) // expected-error{{value of optional type 'T?' not unwrapped; did you mean to use '!' or '?'?}} {{12-12=!}}
takeOpt(t.optS.value) // expected-error{{value of optional type 'T?' not unwrapped; did you mean to use '!' or '?'?}} {{12-12=?}}
// expected-error@-1 {{value of optional type 'S?' not unwrapped; did you mean to use '!' or '?'?}} {{17-17=?}}
takeNon(t.optS.value) // expected-error{{value of optional type 'T?' not unwrapped; did you mean to use '!' or '?'?}} {{12-12=?}}
// expected-error@-1 {{value of optional type 'S?' not unwrapped; did you mean to use '!' or '?'?}}{{17-17=!}}

takeOpt(u.t.s.value) // expected-error{{value of optional type 'U?' not unwrapped; did you mean to use '!' or '?'?}} {{12-12=?}}
// expected-error@-1 {{value of optional type 'T?' not unwrapped; did you mean to use '!' or '?'?}}{{14-14=?}}
// expected-error@-2 {{value of optional type 'S?' not unwrapped; did you mean to use '!' or '?'?}}{{16-16=?}}

takeNon(u.t.s.value) // expected-error{{value of optional type 'U?' not unwrapped; did you mean to use '!' or '?'?}} {{12-12=!}}

t.s.value = 2 // expected-error{{value of optional type 'T?' not unwrapped; did you mean to use '!' or '?'?}} {{4-4=?}}
// expected-error@-1 {{value of optional type 'S?' not unwrapped; did you mean to use '!' or '?'?}}{{6-6=?}}

struct A {
var n: Int = 0
var ns: [Int] = []
}
struct B {
var a: A? = nil
var r: [A] = []
var n: Int? = nil
}
struct C {
var b: B
func thing() -> A? { return nil } // expected-note{{found this candidate}}
func thing() -> B? { return nil } // expected-note{{found this candidate}}
}

func moreTest(c: C?) {
takeOpt(c.thing().n) // expected-error{{ambiguous reference to member 'thing()'}}
// Not ambigious because we can prefer A.n over B.n
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: ambiguous

takeNon(c.thing().n) // expected-error{{value of optional type 'C?' not unwrapped; did you mean to use '!' or '?'?}}{{14-14=?}}
// expected-error@-1 {{value of optional type 'A?' not unwrapped; did you mean to use '!' or '?'?}}{{22-22=!}}

takeOpt(c.b.r[0].n) // expected-error{{value of optional type 'C?' not unwrapped; did you mean to use '!' or '?'?}}{{14-14=!}}
takeNon(c.b.r[0].n) // expected-error{{value of optional type 'C?' not unwrapped; did you mean to use '!' or '?'?}}{{14-14=!}}
takeOpt(c?.b.a.ns.first) // expected-error{{value of optional type 'A?' not unwrapped; did you mean to use '!' or '?'?}}{{19-19=?}}
// expected-error@-1 {{value of optional type '[Int]?' not unwrapped; did you mean to use '!' or '?'?}}{{22-22=?}}
takeNon(c!.b.a.ns.first) // expected-error{{value of optional type 'A?' not unwrapped; did you mean to use '!' or '?'?}}{{19-19=?}}
// expected-error@-1 {{value of optional type '[Int]?' not unwrapped; did you mean to use '!' or '?'?}}{{22-22=?}}
// expected-error@-2 {{value of optional type 'Int?' not unwrapped; did you mean to use '!' or '?'?}}{{28-28=!}}
}
}

func moreComplexUnwrapFixes2() {
class C {
var i: Int = 0
}

class D {
var c: C? = nil
}

func test(d: D?) -> Int? {
return d.c.i // expected-error{{value of optional type 'D?' not unwrapped; did you mean to use '!' or '?'?}}{{13-13=?}}
// expected-error@-1 {{value of optional type 'C?' not unwrapped; did you mean to use '!' or '?'?}}{{15-15=?}}
}
}