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

DougGregor · 2018-07-13T01:45:08Z

When we determine that an optional value needs to be unwrapped to make
an expression type check, use notes to provide several different
Fix-It options (with descriptions) rather than always pushing users
toward '!'. Specifically, the errors + Fix-Its now looks like this:

    error: value of optional type 'X?' must be unwrapped to a value of type 'X'
      f(x)
        ^
    note: coalesce using '??' to provide a default when the optional value contains 'nil'
      f(x)
        ^
          ?? <#default value#>
    note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
      f(x)
        ^
         !

In the case where we are referring to a member of an optional value, but wanted to refer to a member of the wrapped type, we provide a similar diagnostic with separate Fix-Its that suggestions a choice between optional chaining (with '?') and force-unwrap (with '!'):

error: value of optional type 'X?' must be unwrapped to refer to member 'f' of wrapped base type 'X'
  let _: Int = x.f()
               ^
note: chain the optional using '?' to access member 'f' only for non-'nil' base values
  let _: Int = x.f()
               ^
                ?
note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
  let _: Int = x.f()
               ^
                !

Fixes rdar://problem/42081852.

xedin · 2018-07-13T01:55:20Z

@DougGregor Maybe it would be useful to pull these changes in as well? https://github.com/apple/swift/pull/13839/files#diff-cf676abf2776cbcdb21380661ec41563R7718

Main idea of that PR was to remove re-solving with fixes enabled, that's why it didn't get merged.

DougGregor · 2018-07-13T17:39:01Z

@xedin I do think we should bring those improvements into master, but I'm trying to keep my PR isolated to very-low-risk changes to diagnostics emission... I don't want anything to touch the solver in this PR because I want to pull it into 4.2.

xedin · 2018-07-13T17:59:30Z

Sounds good! I will create a separate PR to get them in then!

…apped. When we determine that an optional value needs to be unwrapped to make an expression type check, use notes to provide several different Fix-It options (with descriptions) rather than always pushing users toward '!'. Specifically, the errors + Fix-Its now looks like this: error: value of optional type 'X?' must be unwrapped to a value of type 'X' f(x) ^ note: coalesce using '??' to provide a default when the optional value contains 'nil' f(x) ^ ?? <#default value#> note: force-unwrap using '!' to abort execution if the optional value contains 'nil' f(x) ^ ! Fixes rdar://problem/42081852.

DougGregor · 2018-07-13T18:04:14Z

Now with updated test cases, but I have a few more places to eliminate the old diagnostic and provide a better diagnostic.

…ional base. Improve diagnostics when referencing a member of an optional base, where the Optional type does not have the member but the wrapped type does. Specifically, suggest both the chaining ‘?’ and the force-unwrapping ‘!’ Fix-Its via explanatory notes, e.g.: error: value of optional type '[Int]?' must be unwrapped to refer to member 'subscript' of wrapped base type '[Int]' return foo.array[0] ^ note: chain the optional using '?' to access member 'subscript' only for non-'nil' base values return foo.array[0] ^ ? note: force-unwrap using '!' to abort execution if the optional value contains 'nil' return foo.array[0] ^ ! More of rdar://problem/42081852.

DougGregor · 2018-07-13T22:49:48Z

Second commit provides a diagnostic that suggests using the chaining '?' or a force-wrap '!' when doing member access onto an optional that needs to be unwrapped. It looks a little like this:

  error: value of optional type '[Int]?' must be unwrapped to refer to member 'subscript' of wrapped base type '[Int]'
    return foo.array[0]
               ^
  note: chain the optional using '?' to access member 'subscript' only for non-'nil' base values
    return foo.array[0]
               ^
                    ?
  note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
    return foo.array[0]
               ^
                    !

but isn't kicking in as often as I'd like due to other issues.

…mber access. Introduce a new fix kind into the constraint solver to cover unwrapping the base of a member access so we can refer to the a member of the unwrapped base. Wire this fix kind to the just-added diagnostic that suggests either the chaining ‘?’ or the force-unwrap ‘!’ via separate, descriptive Fix-Its. Example: error: value of optional type 'X?' must be unwrapped to refer to member 'f' of wrapped base type 'X' let _: Int = x.f() ^ note: chain the optional using '?' to access member 'f' only for non-'nil' base values let _: Int = x.f() ^ ? note: force-unwrap using '!' to abort execution if the optional value contains 'nil' let _: Int = x.f() ^ ! Before this, we would sometimes get a Fix-It for just ‘?’ and sometimes get a Fix-It for the coalescing ‘??’, neither of which is likely to be right. More work on rdar://problem/42081852.

…wraps more consistently. Replace the last (and most obscure) use of the poor “use ‘?’ or ‘!’” diagnostic with the new, more explanatory version that provides separate notes with Fix-Its for coalescing or force-unwrapping the value. Finishes rdar://problem/42081852.

DougGregor · 2018-07-13T23:51:00Z

@gregomni My change here to improve diagnostics around optional values that need to be unwrapped is going to conflict with #15321. I think this change makes it somewhat easier to accomplish what you want, though, because we have a better sense of when we're doing member access that could be done via a force or an optional chain, and it's eliminating some state we don't need in the constraint solver.

DougGregor · 2018-07-13T23:51:08Z

@swift-ci please smoke test

DougGregor · 2018-07-13T23:51:41Z

@swift-ci please smoke test

rudkx

Looks awesome, thanks!

xedin · 2018-07-14T00:28:27Z

test/Constraints/fixes.swift

+// expected-note@-1{{chain the optional using '?'}}{{18-18=?}}
+// expected-note@-2{{force-unwrap using '!'}}{{18-18=!}}
+let b: Int = q.s.utf8 // expected-error{{value of optional type 'String?' must be unwrapped to refer to member 'utf8' of wrapped base type 'String'}}
+// expected-note@-1{{chain the optional using '?'}}{{17-17=?}}


Just as an observation - it might not be best to suggest ? in case when contextual is not optional because we'd end up suggesting wrapping right-hand side in (q.s?.utf8)! anyway after that fix-it is applied...

That requires the solver try it as ? and see what happens. It’s good follow-up work that fits well with what @gregomni is doing, but I don’t want to make the solver do more in a 4.2-targeted change.

Sure, makes sense! I just wanted to point that out...

DougGregor · 2018-07-14T03:25:31Z

An LLDB test was depending on this Fix-It; apple/swift-lldb#768 switches the test over to a different Fix-It.

DougGregor · 2018-07-14T05:53:48Z

@swift-ci please smoke test macOS

CodaFi · 2018-07-14T10:18:51Z

lib/Sema/CSSimplify.cpp

-    if (recordFix(fixKind, locator))
+
+    // We're unwrapping the base to perform a member access.
+    if (recordFix(Fix::getUnwrapOptionalBase(*this, member), locator))


I’m confident the rest of this can be pulled out of the typing rule. Removing it altogether affects like 4 diagnostics in the test suite.

The only reason it’s here is because it is, by definition, operating on chains.

DougGregor force-pushed the optional-not-unwrapped-diags branch from 3aade99 to 945c09b Compare July 13, 2018 18:02

DougGregor added 2 commits July 13, 2018 16:26

DougGregor changed the title ~~[WIP] [Type checker] Improve diagnostics when an optional value is not unwrapped~~ [Type checker] Improve diagnostics when an optional value is not unwrapped Jul 13, 2018

rudkx approved these changes Jul 14, 2018

View reviewed changes

xedin reviewed Jul 14, 2018

View reviewed changes

DougGregor merged commit 44088fa into swiftlang:master Jul 14, 2018

DougGregor deleted the optional-not-unwrapped-diags branch July 14, 2018 06:43

CodaFi reviewed Jul 14, 2018

View reviewed changes

gregomni mentioned this pull request Jul 19, 2018

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

DougGregor commented Jul 13, 2018 •

edited

Loading

Uh oh!

xedin commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

xedin commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

rudkx left a comment

Uh oh!

xedin Jul 14, 2018

Uh oh!

DougGregor Jul 14, 2018

Uh oh!

xedin Jul 14, 2018

Uh oh!

DougGregor commented Jul 14, 2018

Uh oh!

DougGregor commented Jul 14, 2018

Uh oh!

CodaFi Jul 14, 2018

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

DougGregor commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

xedin commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

DougGregor commented Jul 13, 2018

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

xedin Jul 14, 2018

Choose a reason for hiding this comment

Uh oh!

DougGregor Jul 14, 2018

Choose a reason for hiding this comment

Uh oh!

xedin Jul 14, 2018

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Jul 14, 2018

Uh oh!

DougGregor commented Jul 14, 2018

Uh oh!

CodaFi Jul 14, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DougGregor commented Jul 13, 2018 •

edited

Loading