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

gregomni · 2018-03-17T04:17:02Z

Be smarter about optional chaining fixits. Previously if we didn't already know a contextual result type to check for optionality we'd always optimistically offer an optional chaining fixit, which results in many new programmers hitting the fix button to add an "?" to an expression that needs a non-optional final value, only to hit the fix button again to add an "!" the next compile now that context is clearer.

See https://forums.swift.org/t/resolved-insert-is-a-bad-fixit/10764/34 for more.

Now we make a disjunction to try both ways and get a correct result the first time. The code turns out to be a bit baroque for long dotted member chains because we need to stepwise make optional each member along the path because we don't have the advantage of rearranging the AST to group the whole chain here inside the constraint system, but we manage it while only making a single
fixit and disjunction and referring back to that original fixit at each step.

…ready know a contextual result type to check for optionality we'd always optimistically offer an optional chaining fixit, which results in many new coders hitting the fix button to add an "?" to an expression that needs a non-optional final value, only to hit the fix button again to add an "!" the next compile now that context is clearer. Now we make a disjunction to try both ways and get a correct result the first time. The code turns out to be a bit baroque for long dotted member chains because we need to stepwise make optional each member along the path because we don't have the advantage of rearranging the AST to group the whole chain here inside the constraint system, but we manage it while only making a single disjunction.

gregomni · 2018-03-17T04:17:17Z

@swift-ci Please smoke test.

CodaFi · 2018-03-17T04:54:02Z

lib/Sema/CSApply.cpp

+    if (!solution.DisjunctionChoices.count(locator) ||
+        !solution.getDisjunctionChoice(fix.second)) {
+      auto type = solution.simplifyType(getType(affected))
+      ->getRValueObjectType();


Nit: Formatting changed when you moved this case

CodaFi · 2018-03-17T05:00:00Z

lib/Sema/CSApply.cpp

@@ -7785,6 +7785,21 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
  }

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


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

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.

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.

CodaFi · 2018-03-17T05:04:46Z

lib/Sema/CSSimplify.cpp

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


Can this really bind to inout?

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.

“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 :)

CodaFi · 2018-03-17T05:08:08Z

lib/Sema/CSSimplify.cpp

+    bool continuingChain = false;
+    if (auto dotExpr = dyn_cast<UnresolvedDotExpr>(locator->getAnchor())) {
+      for (auto pair : Fixes) {
+        if (pair.first.getKind() == FixKind::OptionalChaining) {


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

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.

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™).

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?

CodaFi · 2018-03-17T05:13:24Z

test/Constraints/fixes.swift

+  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=?}}


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

Will add some!

CodaFi · 2018-03-17T05:29:00Z

test/Constraints/iuo_objc.swift

@@ -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 '?'?}}


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

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

- Fix for a silly bug that could cause infinite loop while looking for pre-existing chaining fix if the AST wasn't expected. - Added check so we only try the new complications for FunctionRefKind::Unapplied. If we're doing calls or tuple indexes or something, the optional-resulting disjunction wouldn't work (would require a bunch more complication to get right), so this now reproduces the old behavior for those.

…/checking disjunctions. This also fixes the extra diagnostics that were coming from iuo_objc.

gregomni · 2018-03-18T21:26:34Z

Previously I hadn't realized that a constraint could have an associated fix applied with it. Changed to use this feature for the ? vs ! disjunctions, which lets me not remember or check disjunction results any longer, which is definitely cleaner and hopefully makes @CodaFi a bit happier. :)

CodaFi · 2018-03-19T01:02:43Z

I am happier, thank you. @xedin or @rudkx should give you the final sign-off.

xedin · 2018-03-19T18:53:15Z

lib/Sema/CSSimplify.cpp

@@ -3449,24 +3448,104 @@ ConstraintSystem::simplifyMemberConstraint(ConstraintKind kind,
    instanceTy = MTT->getInstanceType();

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


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.

rudkx

I tried pulling down your changes, and it seems like we end up with a worse result in cases like this:

class C {
  var i: Int = 0
}

class D {
  var c: C? = nil
}

func test(d: D?) -> Int? {
  return d.c.i
}

We previously would have suggested fix-its for both ., and now only suggest a fix-it for the first. After you fix the first and then attempt to compile again, you'll get a fixit for the second.

rudkx · 2018-04-16T20:18:31Z

lib/Sema/CSApply.cpp

+                            diag::missing_unwrap_optional, type);
+    diag.fixItInsertAfter(affected->getEndLoc(), "?");
+    return true;
+  }


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

rudkx · 2018-04-16T20:31:11Z

lib/Sema/CSSimplify.cpp

@@ -3449,24 +3448,104 @@ ConstraintSystem::simplifyMemberConstraint(ConstraintKind kind,
    instanceTy = MTT->getInstanceType();

  // Value member lookup has some hacks too.
-  if (shouldAttemptFixes() && baseObjTy->getOptionalObjectType()) {
+  auto unwrappedBaseObjTy = baseObjTy->getOptionalObjectType();
+  if (unwrappedBaseObjTy) {


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.

… treating '?.' on a member of non-optional type differently from a member of optional type, with separate constraints in the disjunction, although the same fixit spelling. Also split optional chain fixes handling into a seperate function, added more tests.

gregomni · 2018-05-21T02:58:14Z

Sorry for the delay in getting back to this! I believe that this last commit takes into account all the feedback and generates all the necessary fixits for optional chaining in a single compile now.

Resolving conflict

rudkx

I'm still reviewing your changes, but wanted to send some initial feedback.

rudkx · 2018-05-21T19:49:30Z

lib/Sema/CSApply.cpp

-                            diag::missing_unwrap_optional, type);
+
+  case FixKind::OptionalChaining:
+    LLVM_FALLTHROUGH;


You shouldn't need the LLVM_FALLTHROUGH here.

rudkx · 2018-05-21T19:53:38Z

lib/Sema/CSSimplify.cpp

+  if (unwrappedBaseObjTy->getOptionalObjectType())
+    return false;
+
+  if (!(cs.Options & ConstraintSystemFlags::AllowFixes))


This should use cs.shouldAttemptFixes().

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.

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

rudkx · 2018-05-21T20:05:06Z

lib/Sema/CSSimplify.cpp

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


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

rudkx · 2018-05-21T20:12:13Z

test/Constraints/fixes.swift

+
+  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


typo: ambiguous

rudkx · 2018-05-21T21:46:34Z

lib/Sema/CSSimplify.cpp

+  // 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,


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?

gregomni · 2018-05-25T00:08:55Z

I've been fighting CSDiag for the last couple nights, and there turns out to not really be any cleaner way to accomplish my goal here (multiple fixes chosen via disjuncts) with the current state of things. The commits here fix the rest of the feedback and add a FIXME comment about not using shouldAttemptFixes().

The wider right-er way of handling this, I think, is a much bigger rearrangement where none of the constraint system fixes are enabled at all until after the system fails and then only turning on the AllowFixes option in salvaging, but I don't have the bandwidth to attempt that larger scope of changes right now.

gregomni · 2018-07-28T20:22:01Z

Most of the useful bits of this are now in later PRs

CodaFi reviewed Mar 17, 2018

View reviewed changes

rudkx requested review from xedin and rudkx March 17, 2018 05:07

CodaFi reviewed Mar 17, 2018

View reviewed changes

gregomni added 2 commits March 17, 2018 18:15

Changed to use constraints with fixes in them rather than remembering…

4160528

…/checking disjunctions. This also fixes the extra diagnostics that were coming from iuo_objc.

xedin reviewed Mar 19, 2018

View reviewed changes

comment change

df9bd22

rudkx requested changes Apr 17, 2018

View reviewed changes

Merge remote-tracking branch 'apple/master' into opty

e2802e8

Resolving conflict

rudkx reviewed May 21, 2018

View reviewed changes

gregomni added 2 commits May 24, 2018 15:35

Review feedback

f0cee60

Typo correction

029526a

DougGregor self-assigned this Jul 13, 2018

DougGregor mentioned this pull request Jul 13, 2018

[Type checker] Improve diagnostics when an optional value is not unwrapped #17921

Merged

gregomni mentioned this pull request Jul 19, 2018

[Sema] More diagnostic improvement when an optional value is not unwrapped. #18094

Merged

gregomni closed this Jul 28, 2018

gregomni deleted the opty branch August 18, 2020 23:13

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

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

Uh oh!

Conversation

gregomni commented Mar 17, 2018

Uh oh!

gregomni commented Mar 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregomni Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregomni commented Mar 18, 2018

Uh oh!

CodaFi commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregomni commented May 21, 2018

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregomni commented May 25, 2018

Uh oh!

gregomni commented Jul 28, 2018

Uh oh!

CodaFi Mar 17, 2018 •

edited

Loading

CodaFi Mar 17, 2018 •

edited

Loading

gregomni Mar 17, 2018 •

edited

Loading