[Sema] Always look through optionals for unresolved member lookup #34715

Jumhyn · 2020-11-12T22:36:07Z

Previously, we only looked through optionals on UnresolvedMemberExprs when there were no results from Optional itself. However, in cases where Optional has a member with the same name, this would prevent programs from compiling even if the member on Optional was obviously invalid in the context of the UnresolvedMemberExpr. Consider this example:

import SwiftUI

extension UIColor {
  static let background: UIColor? = UIColor.black
}

let backgroundColor: UIColor? = .background

In SwiftUI, Optional conditionally conforms to View whenever Wrapped conforms to View. Additionally, View has an extension which provides an instance method named background(_:alignment:). This means that unresolved member lookup of background will find the method on Optional, and won't continue to look in UIColor for the user-defined background member.

This PR allows the above code to compile by unconditionally looking into Wrapped for unresolved member lookup, even if we already found results in Optional<Wrapped>. Because we already prefer overloads found on Optional to those found on Wrapped in CSRanking, situations where there are valid members on both should continue to choose the member on Optional (though this definitely needs a source-compat run).

Resolves SR-13815.

Jumhyn · 2020-11-12T22:37:53Z

cc @xedin @hborla interested in both of your thoughts on this! It seems like this should be source compatible given our ranking logic but I'd like more eyes to make sure I'm not missing something...

xedin · 2020-11-13T00:13:50Z

lib/Sema/CSSimplify.cpp

      constraintKind == ConstraintKind::UnresolvedValueMember) {
    if (auto objectType = instanceTy->getOptionalObjectType()) {
      // If we don't have a wrapped type yet, we can't look through the optional
      // type.
-      if (objectType->getAs<TypeVariableType>()) {
+      if (objectType->getAs<TypeVariableType>() && result.ViableCandidates.empty()) {


I think this would break source compatibility when score of something found on Wrapped be better than score on Optional itself...

Since we already score Decl better than DeclViaUnwrappedOptional, the score on Wrapped would have to outweigh the disadvantage from the OverloadChoiceKind mismatch. Do you think such situations are likely to occur in practice?

Is there any way to unconditionally prefer the non-optional choices, in the case where we've found multiple solutions?

Since we already score Decl better than DeclViaUnwrappedOptional

Solutions are first ranked on the score produced by the solver (https://github.com/apple/swift/blob/main/lib/Sema/CSRanking.cpp#L758) and then their declaration diff is compared if scores match. It's quite possible that choices could have different optionality of require other implicit conversion which would change the behavior here.

@xedin Ah, thank you for pointing that out. The situation where we have identically-named static members on both Optional<Wrapped> and Wrapped where both properly type-check seems pretty unlikely to me, but if it's above the risk level for being considered source-compatible (as opposed to just a bug) then so be it.

Is there a way to add the Wrapped overloads but not skip over them whenever any Optional overloads successfully type check?

One way to implement this without changing existing behavior would be to lift DeclViaUnwrappedOptional into the solver score to make sure regular declarations are always preferred over the ones found on wrapped type.

Jumhyn · 2020-11-13T16:44:17Z

@xedin I've taken your suggestion and lifted DeclViaUnwrappedOptional into ScoreKind for UnresolvedMemberExpr overload resolution. I've also added a couple tests to make sure that we resolve to overloads on Optional when there are matching overloads on Wrapped that would otherwise be preferred. Let me know what you think!

xedin

LGTM! I think might be worth it to add a unit test for this behavior as it might be much easier to make sure the behavior is correct without having to rely on debug output formatting of the solver.

lib/Sema/ConstraintSystem.cpp

Jumhyn · 2020-11-14T02:46:43Z

@xedin Yeah that makes sense! Is there a good example I could refer to about how to do that? I haven't written a test like that before.

xedin · 2020-11-14T08:30:47Z

I added a couple of unit tests which use ConstraintSystem and related types in unittests/Sema. You’d need to create a couple of fake declarations, generate constraints for a fake unresolved member reference and check that solution has an expected declaration picked and score doesn’t have member-via-unwrap increased.

Jumhyn · 2020-11-14T16:12:33Z

Alright, I'll give those a look—thanks! Will ping you when this PR is ready for another look.

xedin · 2020-11-14T17:47:10Z

Thank you!

Jumhyn · 2020-11-15T02:47:24Z

@xedin Finally got a chance to sit down with this and realized that relying on the solver output is overkill. All we really need is the type-checked AST, so I've switched to just checking the output of -dump-ast instead. Do you think that's sufficient, or do you still want a unit test here as well?

xedin · 2020-11-16T18:56:30Z

@Jumhyn I'd still prefer if we had that as a unit test but I think it's okay to use -dump-ast to verify that, at least it's more stable than -debug-constraints.

LucianoPAlmeida · 2020-11-18T01:45:36Z

@Jumhyn @xedin After those changes member lookup here https://github.com/apple/swift/blob/a9f5107b44c71929521ffbf3a91c5081554f272d/lib/Sema/CSSimplify.cpp#L7183 will return both candidates for Optional and Wrapped types? Just checking as I'm trying to change the CSApply warning to a fix and right now we have to do another lookup ...

Jumhyn · 2020-11-18T14:25:35Z

@LucianoPAlmeida Yes, the results from that line should include candidates from both Optional and Wrapped (but only when we’re doing lookup for an UnresolvedMemberExpr).

LucianoPAlmeida · 2020-11-19T00:44:01Z

@LucianoPAlmeida Yes, the results from that line should include candidates from both Optional and Wrapped (but only when we’re doing lookup for an UnresolvedMemberExpr).

Great thanks, I think that could simplify things in that case :)

Jumhyn · 2020-11-19T01:29:32Z

@LucianoPAlmeida Unfortunately, I probably won't have a chance for the next couple of days to write the unit test to get this mergeable, so you may have to wait for a bit for the changes here to be usable for your fix.

LucianoPAlmeida · 2020-11-19T01:36:38Z

@LucianoPAlmeida Unfortunately, I probably won't have a chance for the next couple of days to write the unit test to get this mergeable, so you may have to wait for a bit for the changes here to be usable for your fix.

No worries @Jumhyn :)
Those are very simple changes so I've already "cherry-picked" to my branch to work with them. It did break the implementation that I had already so it will take some time for me to have something also.

LucianoPAlmeida · 2020-11-21T00:00:47Z

@xedin @Jumhyn Quick question, in this case

enum Foo {
  case bar
  case none
}

let _: Foo?? = .none

Name lookup is actually returning only 2 viable candidates where the base is Foo?? and Foo?, so my question is if that is expected and intentional or it should have dig deeper and find also base Foo?

xedin · 2020-11-21T00:12:10Z

@LucianoPAlmeida It intentionally only unwrap a single layer of optionality at the moment.

LucianoPAlmeida · 2020-11-21T00:18:47Z

@LucianoPAlmeida It intentionally only unwrap a single layer of optionality at the moment.

Ah, thank you @xedin :)
I think that will require another lookup then in order to record the fix correctly then

xedin · 2020-11-21T00:28:09Z

That would be okay since it's performed in diagnostic mode anyway.

LucianoPAlmeida · 2020-11-21T02:02:08Z

That would be okay since it's performed in diagnostic mode anyway.

Humm, that may be a problem because since it is a warning we have to always record it ... but on the other hand the extra lookup will be done only in certain situations involving deep optionals. I'm finishing up the changes and pushing, so we can discuss it there :)

Jumhyn · 2020-12-03T19:37:17Z

Thank you for your patience @xedin! Finally had a chance to sit down an write up the unit test(s). Let me know if this matches what you were picturing.

cc @LucianoPAlmeida, this will hopefully be merge-ready soon! :)

xedin

Looks great! That’s exactly what I had in mind.

unittests/Sema/SemaFixture.cpp

Jumhyn · 2020-12-03T20:16:43Z

@xedin Updated per your comment! Would you mind kicking off the tests on this PR?

xedin · 2020-12-03T20:37:10Z

@swift-ci please test

xedin · 2020-12-03T20:37:30Z

@swift-ci please test source compatibility

UnresolvedMemberLookupTests.cpp

unittests/Sema/SemaFixture.cpp

UnresolvedMemberLookupTests.cpp

swift-ci · 2020-12-03T22:01:08Z

Build failed
Swift Test Linux Platform
Git Sha - cd745f3c5d250b831b3ae680ca01afbd7c4fe852

swift-ci · 2020-12-03T22:28:55Z

Build failed
Swift Test OS X Platform
Git Sha - cd745f3c5d250b831b3ae680ca01afbd7c4fe852

Jumhyn · 2020-12-04T03:52:52Z

@LucianoPAlmeida Latest push addresses your comments and fixes the test failure on Windows (in theory🤞). From what I could tell the Linux and macOS test failures were not related to the code touched here... mind kicking off another round of tests?

xedin · 2020-12-04T04:23:17Z

@swift-ci please test

xedin · 2020-12-04T04:23:27Z

@swift-ci please test source compatibility

swift-ci · 2020-12-04T06:02:23Z

Build failed
Swift Test OS X Platform
Git Sha - 7b35e577c8d1103e05018a87badac6b392daa47e

LucianoPAlmeida · 2020-12-04T12:04:10Z

@swift-ci please test MacOS Platform

LucianoPAlmeida · 2020-12-04T12:11:56Z

@swift-ci Please test OS X platform

Jumhyn · 2020-12-04T17:17:05Z

Windows failure looks like a flake. Rebased against main to see if that resolves the (seemingly unrelated) macOS failure... @xedin @LucianoPAlmeida one more round of tests, please?

LucianoPAlmeida · 2020-12-05T00:12:24Z

@swift-ci please test

xedin · 2020-12-05T04:07:06Z

@swift-ci please test source compatibility

Jumhyn · 2020-12-05T15:11:51Z

@xedin All checks passed!

xedin reviewed Nov 13, 2020

View reviewed changes

Jumhyn requested a review from xedin November 13, 2020 00:30

Jumhyn force-pushed the SR-13815 branch from 1c7bda4 to 25d6893 Compare November 13, 2020 16:34

xedin approved these changes Nov 14, 2020

View reviewed changes

LucianoPAlmeida reviewed Nov 14, 2020

View reviewed changes

lib/Sema/ConstraintSystem.cpp Show resolved Hide resolved

Jumhyn force-pushed the SR-13815 branch from 25d6893 to 0b53cba Compare November 15, 2020 02:44

Jumhyn mentioned this pull request Nov 15, 2020

[Sema][NFC] Correcting test case for SR-11535 and adding an explanation comment #34744

Merged

Jumhyn force-pushed the SR-13815 branch from 0b53cba to 817b49d Compare November 24, 2020 04:06

Jumhyn force-pushed the SR-13815 branch 2 times, most recently from ce979a0 to ef52a0a Compare December 3, 2020 19:35

xedin approved these changes Dec 3, 2020

View reviewed changes

unittests/Sema/SemaFixture.cpp Outdated Show resolved Hide resolved

Jumhyn force-pushed the SR-13815 branch from ef52a0a to cd745f3 Compare December 3, 2020 20:16

LucianoPAlmeida reviewed Dec 3, 2020

View reviewed changes

UnresolvedMemberLookupTests.cpp Outdated Show resolved Hide resolved

LucianoPAlmeida reviewed Dec 3, 2020

View reviewed changes

unittests/Sema/SemaFixture.cpp Outdated Show resolved Hide resolved

LucianoPAlmeida reviewed Dec 3, 2020

View reviewed changes

unittests/Sema/SemaFixture.cpp Outdated Show resolved Hide resolved

LucianoPAlmeida reviewed Dec 3, 2020

View reviewed changes

UnresolvedMemberLookupTests.cpp Outdated Show resolved Hide resolved

Jumhyn force-pushed the SR-13815 branch from cd745f3 to 7b35e57 Compare December 4, 2020 03:39

Jumhyn added 2 commits December 4, 2020 12:11

[Sema] Always look through optionals for unresolved member lookup

a53b1e4

[Unit tests] Add unit tests for optional lookup behavior

6b0f5da

Jumhyn force-pushed the SR-13815 branch from 7b35e57 to 6b0f5da Compare December 4, 2020 17:15

xedin merged commit 65db86b into swiftlang:main Dec 5, 2020

Jumhyn deleted the SR-13815 branch December 5, 2020 23:56

Jumhyn restored the SR-13815 branch February 10, 2021 20:48

LucianoPAlmeida mentioned this pull request Dec 11, 2020

[SR-11535] Generic ambiguous none case is not diagnosed #53936

Closed

xedin mentioned this pull request Dec 5, 2020

[SR-13815] Confusing unwrapping diagnostic when using implicit member access on Optionals. #56212

Closed

[Sema] Always look through optionals for unresolved member lookup #34715

[Sema] Always look through optionals for unresolved member lookup #34715

Uh oh!

Conversation

Jumhyn commented Nov 12, 2020

Uh oh!

Jumhyn commented Nov 12, 2020

Uh oh!

xedin Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

Jumhyn Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

xedin Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

Jumhyn Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

xedin Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

Jumhyn commented Nov 13, 2020

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jumhyn commented Nov 14, 2020

Uh oh!

xedin commented Nov 14, 2020

Uh oh!

Jumhyn commented Nov 14, 2020

Uh oh!

xedin commented Nov 14, 2020

Uh oh!

Jumhyn commented Nov 15, 2020

Uh oh!

xedin commented Nov 16, 2020

Uh oh!

LucianoPAlmeida commented Nov 18, 2020

Uh oh!

Jumhyn commented Nov 18, 2020

Uh oh!

LucianoPAlmeida commented Nov 19, 2020

Uh oh!

Jumhyn commented Nov 19, 2020

Uh oh!

LucianoPAlmeida commented Nov 19, 2020

Uh oh!

LucianoPAlmeida commented Nov 21, 2020

Uh oh!

xedin commented Nov 21, 2020

Uh oh!

LucianoPAlmeida commented Nov 21, 2020

Uh oh!

xedin commented Nov 21, 2020

Uh oh!

LucianoPAlmeida commented Nov 21, 2020

Uh oh!

Jumhyn commented Dec 3, 2020

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jumhyn commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Dec 3, 2020

Uh oh!

xedin commented Dec 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swift-ci commented Dec 3, 2020

Uh oh!

Jumhyn commented Dec 3, 2020 •

edited

Loading