[Sema] More unwrap fixits #18324

gregomni · 2018-07-28T06:41:24Z

When we diagnose an unwrap, if the expression being unwrapped is a sole reference to a local let/var, and the local's type is inferred, offer fixits on the initializer as well. In this position we can also reasonably offer 'guard let'.

gregomni · 2018-07-28T06:49:11Z

Do you think it would be reasonable to omit the fixits at the place of use entirely in favor of the ones at the place of initialization? I think for the vast majority of users that are going to be using these fixits, that that's going to be a better fix, and potentially providing 5 fixit choices could be more overwhelming than helpful...

rudkx · 2018-07-28T19:34:21Z

include/swift/AST/DiagnosticsSema.def

@@ -925,6 +925,12 @@ NOTE(unwrap_with_default_value,none,
 NOTE(unwrap_with_force_value,none,
     "force-unwrap using '!' to abort execution if the optional value contains "
     "'nil'", ())
+NOTE(unwrap_iuo_initializer,none,
+     "initializing from an implicitly unwrapped optional is inferred to be "
+     "optional in Swift 4", ())


This was true in Swift 3 in general.

It's been longer than I remembered! Will fix.

gregomni · 2018-08-01T03:08:33Z

Went ahead and decided I think declaration site fixits are always better than use site fixits, where possible. And with that, I think this is ready, and does most everything that I think is reasonably possible to do via fixits from this thread https://forums.swift.org/t/resolved-insert-is-a-bad-fixit/10764.

@swift-ci Please smoke test.

gregomni · 2018-08-01T21:57:45Z

@swift-ci Please smoke test.

gregomni · 2018-08-06T15:33:28Z

@DougGregor @rudkx No available time to comment? Should I ask someone else for review, or do you think these diagnostic changes are reasonable to merge as is? Thanks!

rudkx · 2018-08-06T18:01:54Z

@swift-ci build toolchain

rudkx · 2018-08-06T18:02:14Z

@swift-ci Please test source compatibility

swift-ci · 2018-08-06T18:32:04Z

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 1eed767ad7b610d7385c8b141dd990ff42c91d71

Install command
tar zxf swift-PR-18324-46-ubuntu16.04.tar.gz
More info

swift-ci · 2018-08-06T19:05:03Z

macOS Toolchain
Download Toolchain
Git Sha - 1eed767ad7b610d7385c8b141dd990ff42c91d71

Install command
tar -zxf swift-PR-18324-57-osx.tar.gz --directory ~/

rudkx · 2018-08-06T19:10:55Z

I really like the idea of pointing at the declaration site in the cases where we can unambiguously identify that as the likely problem.

Looking at the behavior here (without reviewing the code), I think there are a couple areas that could be improved.

For the following:

func f(x: Int!) -> Int {
  var y = x
  let z: Int = y + 1
}

with your changes we emit:

error: initializing from an implicitly unwrapped optional is inferred to be optional, and does not remain implicitly unwrapped
  var y = x
          ^
note: unwrapped optional used here
  let z: Int = y + 1
               ^
note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
  var y = x
          ^
           !

whereas we previously would emit:

error: value of optional type 'Int?' must be unwrapped to a value of type 'Int'
  let z: Int = y + 1
               ^
note: coalesce using '??' to provide a default when the optional value contains 'nil'
  let z: Int = y + 1
               ^
                 ?? <#default value#>
note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
  let z: Int = y + 1
               ^
                !

I think the best output here would be to emit the error at the use site as the code on master does, and then point to the declaration as a note with a fix-its for the declaration site that suggest both ?? and !, and additionally continue to have the notes with fixits that we emit with the code on master for the use sites as well, e.g.:

error: value of optional type 'Int?' must be unwrapped to a value of type 'Int'
  let z: Int = y + 1
               ^
note: initializing from an implicitly unwrapped optional is inferred to be optional, and does not remain implicitly unwrapped
  var y = x
          ^
note: coalesce using '??' to provide a default when the optional value contains 'nil'
  var y = x
          ^
            ?? <#default value#>
note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
  var y = x
          ^
           !
note: coalesce using '??' to provide a default when the optional value contains 'nil'
  let z: Int = y + 1
               ^
                 ?? <#default value#>
note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
  let z: Int = y + 1
               ^
                !

rudkx · 2018-08-06T19:18:47Z

A couple other thoughts:

I don't think we need to limit this to cases where there is only a single reference. At first glance seems perfectly reasonable (and in fact even more important) to emit this if there are multiple references, and it's also perfectly reasonable if there are reassignments to the initialized value since the type is inferred at the declaration site
The message "initializing from an implicitly unwrapped optional is inferred to be optional, and does not remain implicitly unwrapped" could be improved. Perhaps something like "value inferred to be type %0 when initialized with an implicitly unwrapped value" and print the inferred type?

gregomni · 2018-08-06T20:02:34Z

@rudkx Thanks for the feedback!

gregomni · 2018-08-07T05:34:36Z

@rudkx First, with regard to IUOs:

Yes, your suggested wording for that message is much better, will change that.
My thought, in restricting the fixits to just ! for an IUO, was to assume that the value just ought not be nil (implicitly.. :) ) but I can certainly also see the benefit in providing all the fixit options, will change that too.

The major difficulty comes with how to provide the at-declaration and at-use fixits for Xcode. I agree with you that from the command line providing all 5 fixit notes is best. Unfortunately, it looks like what Xcode does (10b5, at least) is to highlight the line where the first note appears, and only offer fixits for that line. E.g. image

... And actually as I write that, it occurs to me that that's fine. I do what you suggest, and we see both at-decl and at-use on the command line, and only at-decl in Xcode, which is what I fell back on providing in any case...

Ok. So I'll do that. So in short, I'll just do all your suggestions... It would be nice to make Xcode deal with this sort of thing better though - should I file a radar?

Finally, yes, this doesn't need to be limited to single references, but I'd like to avoid duplicating the at-decl notes if there are, say, 3 references, and all of them have the same unwrap error. So that maybe requires scope level marking of which decls we've already complained about or something like that. Would like to leave expanding in that direction for a future PR.

gregomni · 2018-08-07T06:46:00Z

Did the things.

@swift-ci Please smoke test.

rudkx · 2018-08-07T15:23:30Z

@swift-ci build toolchain macOS platform

gregomni · 2018-08-10T14:56:04Z

Squashed this down to a single commit in prep for merging with #18620

xedin

One interesting thing I noticed while working on porting of the diagnostics over to new abstraction is that if I remove condition which presents unwrap fixes from being issues from here we only get failures in test/Constraints/bridging.swift e.g.

test/Constraints/bridging.swift:279:42: error: incorrect message found
  let _: String? = ns // expected-error{{cannot convert value of type 'NSString?' to specified type 'String?'}}
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                         'NSString?' is not convertible to 'String'; did you mean to use 'as!' to force downcast?
test/Constraints/bridging.swift:280:42: error: incorrect message found
  var _: String? = ns // expected-error{{cannot convert value of type 'NSString?' to specified type 'String?'}}
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                         'NSString?' is not convertible to 'String'; did you mean to use 'as!' to force downcast?
test/Constraints/bridging.swift:286:43: error: incorrect message found
  let _: String? = vns // expected-error{{cannot convert value of type 'NSString?' to specified type 'String?'}}
                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                          'NSString?' is not convertible to 'String'; did you mean to use 'as!' to force downcast?
test/Constraints/bridging.swift:287:43: error: incorrect message found
  var _: String? = vns // expected-error{{cannot convert value of type 'NSString?' to specified type 'String?'}}
                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                          'NSString?' is not convertible to 'String'; did you mean to use 'as!' to force downcast?
test/Constraints/bridging.swift:294:70: error: incorrect message found
  var s: String = ns ?? "str" as String as String // expected-error{{cannot convert value of type 'NSString?' to expected argument type 'String?'}}
                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                     'NSString?' is not convertible to 'String'; did you mean to use 'as!' to force downcast?
test/Constraints/bridging.swift:295:64: error: incorrect message found
  var s2 = ns ?? "str" as String as String // expected-error {{cannot convert value of type 'String' to expected argument type 'NSString'}}
                                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                               'NSString?' is not convertible to 'String'; did you mean to use 'as!' to force downcast?
test/Constraints/bridging.swift:299:51: error: incorrect message found
  var s4: String = ns ?? "str" // expected-error{{cannot convert value of type 'NSString' to specified type 'String'}}
                                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                  'NSString?' is not convertible to 'String'; did you mean to use 'as!' to force downcast?

It does seem like it's suggesting a right thing anyways via fix?...

So maybe it's time to revisit the condition, WDYT?

slavapestov · 2018-08-11T00:17:07Z

lib/Sema/CSDiag.cpp

+  int referencesCount() { return count; }
+};
+
+bool swift::diagnoseUnwrap(ConstraintSystem &CS, Expr *expr, Type type) {


Maybe move these functions to the new CSDiagnostics.cpp, and CSDiag.cpp can just be the graveyard for the old style of diagnostics that will eventually be ripped out?

slavapestov · 2018-08-11T00:17:25Z

lib/Sema/ConstraintSystem.h

@@ -3571,7 +3571,7 @@ class InputMatcher {
 /// that needs to be unwrapped.
 ///
 /// \returns true if a diagnostic was produced.
-bool diagnoseUnwrap(TypeChecker &TC, DeclContext *DC, Expr *expr, Type type);
+bool diagnoseUnwrap(constraints::ConstraintSystem &CS, Expr *expr, Type type);


Put this in CSDiagnostics.h

gregomni · 2018-08-12T03:40:02Z

@xedin Which condition are you removing for that result, I'm not sure what you are referring to?

I don't think we should do it as-is right now because that would lead to fixiting as! String for those tests, when the preferred fixit should really be as? String?. The result will compile, but you'll be forcing and then re-injecting the optional. I think it's slightly better to error without a fixit than to error with a non-ideal fixit like this, although I could see arguing either way.

I do think it ought to be explored as a possible place to improve diagnostics further and I'd be happy to do that.

gregomni · 2018-08-12T03:42:53Z

@slavapestov There is one other caller (optional near-miss callee-candidate) in CSDiag. I was hoping it might be dead code, but removing it causes a couple test failures, so the functions can't be trivially moved at the moment. Another good direction for further work.

xedin · 2018-08-12T03:57:04Z

@gregomni I has just talking conditions in the beginning of applySolutionFix which checks if locator could be simplified all the way down to expression or not...

PS. I think the best would be suggest as String? right?

gregomni · 2018-08-12T04:00:38Z

@xedin Yes, sorry, that's right.

gregomni · 2018-08-12T04:03:39Z

@swift-ci Please smoke test.

…le reference to a local let/var, and the local's type is inferred, offer fixits on the initializer as well. In this position we can also reasonably offer 'guard let'. Since we're identifying the implicitly typed initializer now, we can give a specific explanation of how IUOs get turned into optional types with the new IUO implementation. Pass constraint system down into offering force unwrap fixits so that we can identify the type of the last member chosen for an optional chain. If there's a chain and the last member's return type isn't optional, then it's cleaner to offer to change that last '?' into a '!' to perform the force, rather than parenthesize the whole expression and force the result.

gregomni · 2018-08-12T20:05:51Z

Found the restriction in tryTypeVariableBindings() that was keeping us from always finding MissingOptionalUnwrapFailures (and then had to fix conflict with @slavapestov's tuple type cleanup).

Which then allows all the unwrap fixit code to be moved into CSDiagnostics and made static since it now has only a single caller.

@swift-ci Please smoke test.

gregomni · 2018-08-12T21:41:26Z

Ok to merge?

xedin · 2018-08-12T21:42:43Z

@swift-ci please test source compatibility

xedin · 2018-08-12T21:44:04Z

I haven’t had a chance to look at the updated patch yet, could you wait for a bit? Also let’s run source compatibility suite since this changed solver logic...

xedin · 2018-08-12T21:44:43Z

@swift-ci please test source compatibility

xedin · 2018-08-12T22:16:54Z

lib/Sema/CSSolver.cpp

-              }
-            } else {
+      // If we were unsuccessful solving for T?, try solving for T.
+      if (auto objTy = type->getOptionalObjectType()) {


I guess the reason why it was only allowed for subtypes is related to value-to-optional promotion, which makes T a subtype of T? effectively? I think @rudkx should have more input regarding if this is for all types of bindings...

Right, it's valid to go T? to T for subtype bindings. It's not valid to go T? to T for any others, but binding the type variable to T (generally the type variable for a param) after T? has already failed gives us the opportunity in attempting to solve the system to match the T? value to the T type variable by adding a MissingOptionalUnwrapFailure.

So the result is a solution with a fix, instead of unsolved.

I think that might be reasonable to extend a check to subtype || shouldAttemptFixes() and leave comment about why it’s that way for posterity, WDYT?

Yep, you're right, the change definitely should be gated off of shouldAttemptFixes()

gregomni · 2018-08-13T00:39:22Z

@swift-ci please test source compatibility

gregomni · 2018-08-13T00:46:09Z

@swift-ci Please smoke test

rudkx · 2018-08-13T17:40:05Z

lib/Sema/CSSolver.cpp

+      // if fixes are allowed, because that gives us the opportunity to
+      // match T? values to the T binding by adding an unwrap fix.
+      if (binding.Kind == AllowedBindingKind::Subtypes ||
+          shouldAttemptFixes()) {


At first glance I'm not sure this has to be guarded by shouldAttemptFixes(), and it seems like it might even be misleading to do so because we're not recording a fix here.

It also seems like if this is legal (and again at first glance I think it is), this should be part of a separate PR.

I'd be fine with pulling out the last two commits here into a separate PR and merging the rest to get it in, if you are ok with that.

There is no functional change between Subtypes || shouldAttemptFixes() and no condition here because this attempted binding will just be discarded if solveRec() fails due to lack of fixes. But I don't think that it can succeed in the non-subtypes case without fixes so it is saving time.

I think this is kind of chicken-and-egg problem here, we have to attempt to produce a fix and we can't record it upfront either...

gregomni · 2018-08-14T15:13:21Z

@rudkx Went ahead and pulled out the CSSolver binding change and will add that as a separate PR in a moment.

@swift-ci Please smoke test

rudkx · 2018-08-14T17:36:57Z

Great, thanks!

gregomni requested a review from DougGregor July 28, 2018 06:45

gregomni force-pushed the opty2 branch from 68133af to cd01acc Compare July 28, 2018 14:51

rudkx reviewed Jul 28, 2018

View reviewed changes

gregomni requested a review from rudkx August 1, 2018 14:56

gregomni force-pushed the opty2 branch from be151e4 to 00d7cc6 Compare August 10, 2018 14:54

xedin reviewed Aug 10, 2018

View reviewed changes

slavapestov reviewed Aug 11, 2018

View reviewed changes

gregomni force-pushed the opty2 branch from 00d7cc6 to f1a2d97 Compare August 12, 2018 03:08

Conflict resolution.

ba96db6

gregomni force-pushed the opty2 branch from d3eb74f to 5067338 Compare August 12, 2018 20:00

xedin reviewed Aug 12, 2018

View reviewed changes

rudkx reviewed Aug 13, 2018

View reviewed changes

gregomni force-pushed the opty2 branch from bd0fecb to ba96db6 Compare August 14, 2018 15:12

gregomni mentioned this pull request Aug 14, 2018

[ConstraintSystem] Further unwrap cleanup #18699

Merged

gregomni merged commit 8f41ee7 into swiftlang:master Aug 14, 2018

gregomni mentioned this pull request Aug 16, 2018

[Sema] Improve type coercion fixits for optional-to-optional conversions. #18757

Merged

[Sema] More unwrap fixits #18324

[Sema] More unwrap fixits #18324

Uh oh!

Conversation

gregomni commented Jul 28, 2018

Uh oh!

gregomni commented Jul 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregomni commented Aug 1, 2018

Uh oh!

gregomni commented Aug 1, 2018

Uh oh!

gregomni commented Aug 6, 2018

Uh oh!

rudkx commented Aug 6, 2018

Uh oh!

rudkx commented Aug 6, 2018

Uh oh!

swift-ci commented Aug 6, 2018

Uh oh!

swift-ci commented Aug 6, 2018

Uh oh!

rudkx commented Aug 6, 2018

Uh oh!

rudkx commented Aug 6, 2018

Uh oh!

gregomni commented Aug 6, 2018

Uh oh!

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

Uh oh!

Uh oh!

gregomni commented Aug 7, 2018

Uh oh!

rudkx commented Aug 7, 2018

Uh oh!

gregomni commented Aug 10, 2018

Uh oh!

xedin 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 Aug 12, 2018

Uh oh!

gregomni commented Aug 12, 2018

Uh oh!

xedin commented Aug 12, 2018

Uh oh!

gregomni commented Aug 12, 2018

Uh oh!

gregomni commented Aug 12, 2018

Uh oh!

gregomni commented Aug 12, 2018

Uh oh!

gregomni commented Aug 12, 2018

Uh oh!

xedin commented Aug 12, 2018

Uh oh!

xedin commented Aug 12, 2018

Uh oh!

xedin commented Aug 12, 2018

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 Aug 13, 2018

Uh oh!

gregomni commented Aug 13, 2018

Uh oh!

gregomni commented Aug 7, 2018 •

edited

Loading

gregomni Aug 13, 2018 •

edited

Loading