[Diag] Improve diagnostics for optional unwrapping

[theblixguy]

This PR resolves an issue where incorrect diagnostics would be generated when diagnosing optional unwrapping.

struct Foo {
  func bar() -> String??? { return "bar" }
}

let foo: Foo? = Foo()

// incorrect error: Value of optional type 'String??' must be unwrapped to a value of type 'String?'
let _: String?? = foo?.bar()

Resolves SR-9201.

[theblixguy]

cc @xedin

[xedin]

Thanks, @theblixguy! I'll take a look tonight.

[xedin]

LGTM! I have left a couple of minor comments, and regarding diagnostic you mentioned - I think it makes more sense to report new optional unwrap diagnostic there, so let's update all other test and see if there are any true regressions.

[theblixguy]

@xedin I have updated the diagnostic messages in the test files, can you take a look?

Also, the one in optional_context_member.swift throws an "unexpected error produced" on line 16 but the error matches what the // expected-error {{ ... }} says. Not sure what's going on there, but it could just be an issue with my build of the compiler.

[xedin]

New errors are great! Regarding the problem in optional_context_member.swift - that means that the same error is produced twice now, I think that's worth investigating.

[theblixguy]

@xedin Ok, so after some debugging, it's happening in matchTypes(). In this example:

struct Foo {
  static var someOptVar: Foo? = Foo()
}

func nonOptContext() -> Foo {
  switch () {
  case ():
    return .someOptVar 
  }
}

matchTypes() is called several times, which I think is expected.

if (objectType1->getOptionalObjectType()) { .. } is called 3 times and the check succeeds 2 times, hence why we see 2 fixes.

It's a bit strange because I don't think I have touched any code that would cause this to happen. So it must've been happening from the start, but then that doesn't explain why the diagnostic wasn't being emitted twice previously.

[theblixguy]

Dumping the fixes shows:

[fix: force optional] @ locator@0x7fe865818a20 [UnresolvedMember@/Users/suyashsrijan/Desktop/test.swift:8:13 -> unresolved member]

[fix: force optional] @ locator@0x7fe865818b10 [UnresolvedMember@/Users/suyashsrijan/Desktop/test.swift:8:13 -> rvalue adjustment]

[xedin]

@theblixguy That's interesting, in recordFix we have logic which check if we already have a fix for a particular locator. As you can see in your output - it's anchor but different path - one is unresolved member and another is rvalue adjustment. Maybe for ForceOptional fixes we'd want to record only anchor for locator instead of the whole path to avoid this kind of a situation?

[xedin]

@theblixguy Sorry it wasn't clear - I mean use anchor from locator when we construct ForceOptional fix, I think logic in recordFix should stay the same.

[theblixguy]

@xedin ah okay, that works 🎉

[xedin]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - f5264b9

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - f5264b9

[theblixguy]

@xedin I was running a validation test on my machine and it found 2 more failures. I have adjusted the diagnostics in them, but could you take a look at 18d4da1?

[xedin]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 8845fcb

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 8845fcb

[theblixguy]

@xedin all tests have passed, but seems like there's an unrelated failure: Assertion failed: (e == 0), function ~recursive_mutex, file /BuildRoot/Library/Caches/com.apple.xbs/Sources/libcxx/libcxx-400.9/src/mutex.cpp, line 64.

[xedin]

@swift-ci please test macOS platform

[xedin]

@theblixguy Thanks!