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 1 commit
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
24 changes: 15 additions & 9 deletions lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7785,6 +7785,21 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
}

switch (fix.first.getKind()) {
case FixKind::OptionalChaining: {
if (!solution.DisjunctionChoices.count(locator) ||
Copy link
Contributor

@CodaFi CodaFi Mar 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Examining disjuncts is a brittle way to propagate this through the constraint system. I wonder if you couldn't implement this with another FixKind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the way that implicit unwrapping or not with IUOs gets propagated. I know IUOs isn't exactly the best part of the implementation to emulate, but it is existing practice... If you have another suggestion I'd be glad to try it out. Another FixKind wouldn't work by itself because there is no way to know at the time the fix is added which of the two possibilities will type check.

Copy link
Contributor

@CodaFi CodaFi Mar 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

it is existing practice

It is existing practice in the context of the implementation of IUOs because there we put up a front to maintain the illusion that IUOs aren't a modality of T?. This uses disjunctions as pieces of state and I suspect we have better ways of handling that.

!solution.getDisjunctionChoice(fix.second)) {
auto type = solution.simplifyType(getType(affected))
->getRValueObjectType();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Formatting changed when you moved this case

auto diag = TC.diagnose(affected->getLoc(),
diag::missing_unwrap_optional, type);
diag.fixItInsertAfter(affected->getEndLoc(), "?");
return true;
}
// Making the result optional failed, but non-optional succeeded, so
// we can't optional chain after all: fallthrough into forcing.
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.

Is there an actual change here, or were these just reordered? If the latter, is there a reason to do so?


case FixKind::ForceOptional: {
const Expr *unwrapped = affected->getValueProvidingExpr();
auto type = solution.simplifyType(getType(affected))
Expand All @@ -7808,15 +7823,6 @@ 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);
diag.fixItInsertAfter(affected->getEndLoc(), "?");
return true;
}

case FixKind::ForceDowncast: {
if (auto *paren = dyn_cast<ParenExpr>(affected))
Expand Down
104 changes: 87 additions & 17 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3440,7 +3440,6 @@ ConstraintSystem::simplifyMemberConstraint(ConstraintKind kind,
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.

Expand All @@ -3449,24 +3448,95 @@ ConstraintSystem::simplifyMemberConstraint(ConstraintKind kind,
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.

auto unwrappedBaseObjTy = baseObjTy->getOptionalObjectType();
if (unwrappedBaseObjTy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you split all of this out into a separate function? It's gone from ~20 lines to ~100 lines.

Also, please run clang-format or otherwise change the formatting so that the code fits within 80 columns of text.

// 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;


// Figure out if we have an existing optional chaining fix done earlier in
// this dotted expression chain.
bool continuingChain = false;
if (auto dotExpr = dyn_cast<UnresolvedDotExpr>(locator->getAnchor())) {
for (auto pair : Fixes) {
if (pair.first.getKind() == FixKind::OptionalChaining) {
Copy link
Contributor

Choose a reason for hiding this comment

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

CSSimplify is too early to be examining the Fixes generated by a particular constraint system. This should be done in CSApply.

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 is to avoid creating disjunctions at every step in the chain, which seems worthwhile to avoid 2^n attempted solves. It's true that if we wanted to let that happen we could add a fix at each dot expr and then check fixes in CSApply to make all but the first one just produce no fixit.

Copy link
Contributor

Choose a reason for hiding this comment

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

My thinking is that potentially doing n^2~ish work on every simplification isn't that great either. If the constraint system doesn't look right, perhaps we should look into changing its morphology or emitting a "compound fix" like you mention here.

Besides, I suspect n would be quite low in practice and would be helped by deforestation of the solution space in other parts of this file (This Is Not An Endorsement™).

Copy link
Contributor Author

@gregomni gregomni Mar 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Now that I've slept, let me try to talk about this better:

Given an example like takeOpt(u.t.s.value) the parameter takes an optional so we want the result to be a fixit to optional chain this series of dots. Stepwise for each dot expr we see an optional base object type, fix it, and tell the constraint system the result should then be optional.

If this was done in a symmetrical, consistent way instead of the admittedly hacky look-to-see-if-a-chain-fix-already-exists way, then it'd have two major impacts. First, we'd need to allow fixes during evaluation of disjunction candidates instead of just at the top level which is an algorithmic change I don't know enough to judge. This may or may not be a good idea in general?

Second, it always type checks in this sort of expression to force the optional at any of the steps and then rewrap at the end, and with multiple independent chaining fixes, a solution of a single force fix at the beginning will get the best score. So taking a symmetrical approach would require additional state on the Disjunction or perhaps on each Constraint inside that would be examined by the scorer to make it so that a forcing fix was scored as some multiple N-times worse than a step of chaining fix. If this ability to make lopsided disjunction scoring seems like it'd be useful elsewhere, maybe that's the way to go?

auto innerExpr = dotExpr->getBase();
while (innerExpr) {
if (pair.second->getAnchor() == innerExpr) {
continuingChain = true;
break;
}
if (auto innerDot = dyn_cast<UnresolvedDotExpr>(innerExpr))
innerExpr = innerDot->getBase();
}
}
}
}

// If we aren't continuing an existing chain, do we want to add a new fixit?
if (!continuingChain) {
if (!shouldAttemptFixes())
return SolutionKind::Error;

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

// Note the fix.
auto fixKind = forceUnwrap ? FixKind::ForceOptional : FixKind::OptionalChaining;
if (recordFix(fixKind, locator))
return SolutionKind::Error;

if (forceUnwrap) {
// The simple case - just look through one level of optional.
addValueMemberConstraint(unwrappedBaseObjTy,
member, memberTy, useDC, functionRefKind, locator);
return SolutionKind::Solved;
}
}

// See if there is a matching member with an optional type.
// If so, then inserting an optional chaining fix won't produce a MORE optional result,
// and it is safe to just fix with chaining.
MemberLookupResult result =
performMemberLookup(kind, member, unwrappedBaseObjTy, functionRefKind, locator,
/*includeInaccessibleMembers*/false);
bool memberIsOptional = false;
for (auto candidate : result.ViableCandidates) {
if (candidate.isDecl())
memberIsOptional |= !candidate.getDecl()->getInterfaceType()->
getOptionalObjectType().isNull();
}

// If the member isn't already optional, we need to make the result optional.
if (!memberIsOptional) {
auto innerTV = createTypeVariable(locator, TVO_CanBindToLValue | TVO_CanBindToInOut);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this really bind to inout?

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 is cribbed from how the type variables for non-implicit optional chaining are created in CSGen. I can look at whether it's necessary or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

“Can bind to inout” used to be the default. When I made an explicit flag for it, I went and added it in every place we were creating a type variable. Most of them can lose the flag - that was the original motivation for adding it - but nobody got around to cleaning that up yet. Maybe @CodaFi can do it :)

Type optTy = getTypeChecker().getOptionalType(SourceLoc(), innerTV);
if (!optTy)
return SolutionKind::Error;

if (continuingChain) {
addConstraint(ConstraintKind::Conversion, optTy, memberTy, locator);
} else {
// If we are starting a new chain, it may or may not be valid. Add a disjunction
// with the optional and non-optional result. Later when we generate the fixit
// we'll check the disjunction and fall back on force unwrapping if the
// non-optional result is the solution.
SmallVector<Constraint *, 4> optionalities;
auto optionalResult = Constraint::create(*this, ConstraintKind::Conversion,
optTy, memberTy, locator);
auto nonoptionalResult = Constraint::create(*this, ConstraintKind::Conversion,
innerTV, memberTy, locator);
optionalities.push_back(optionalResult);
optionalities.push_back(nonoptionalResult);
addDisjunctionConstraint(optionalities, locator, RememberChoice);
}
memberTy = innerTV;
}

// Look through one level of optional.
addValueMemberConstraint(baseObjTy->getOptionalObjectType(),
addValueMemberConstraint(unwrappedBaseObjTy,
member, memberTy, useDC, functionRefKind, locator);
return SolutionKind::Solved;
}
Expand Down
32 changes: 32 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,35 @@ 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=?}}
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=?}}
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=?}}
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=?}}
}
6 changes: 4 additions & 2 deletions test/Constraints/iuo_objc.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ import Foundation

func iuo_error(prop: IUOProperty) {
let _: Coat? = prop.iuo.optional()
// expected-error@-1 {{value of optional type '(() -> Coat?)?' not unwrapped; did you mean to use '!' or '?'?}}
// expected-error@-1 {{value of optional type 'OptionalMethods?' not unwrapped; did you mean to use '!' or '?'?}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the promotion of the chain from U! to T? intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry is this comment pointed at the wrong place? Not sure what's intended.

// expected-error@-2 {{value of optional type '(() -> Coat?)?' not unwrapped; did you mean to use '!' or '?'?}}
let _: Coat? = prop.iuo.optional()!
// expected-error@-1 {{cannot invoke 'optional' with no arguments}}
// expected-error@-1 {{value of optional type 'OptionalMethods?' not unwrapped; did you mean to use '!' or '?'?}}
// expected-error@-2 {{value of optional type '(() -> Coat?)?' not unwrapped; did you mean to use '!' or '?'?}}
let _: Coat? = prop.iuo.optional!()
let _: Coat? = prop.iuo.optional!()!
let _: Coat? = prop.iuo!.optional()
Expand Down