[CS] Use fixes to diagnose instance member on type (or vice versa) access

[theblixguy]

This PR migrates instance member on type and type member on instance diagnostics handling to use the new diagnostics framework (fixes) and create more reliable and accurate diagnostics in such scenarios.

[theblixguy]

cc @xedin @slavapestov

[xedin]

Great! I will take a closer look later today but one thing I see right away - please remove relevant code from CSDiag.cpp which is now obsolete.

[theblixguy]

Great! I will take a closer look later today but one thing I see right away - please remove relevant code from CSDiag.cpp which is now obsolete.

Oh yeah, done :) I am not sure about this block of code: https://github.com/apple/swift/blob/master/lib/Sema/CSDiag.cpp#L1006L1022

I've left it as it's a special case, but perhaps I can move it to diagnoseAsError() if needed, along with some other code related to UR_InstanceMemberOnType & UR_TypeMemberOnInstance handling.

[xedin]

Overall I think this is very good start, but definitely needs more work...

[theblixguy]

Thanks a lot for the feedback @xedin! Can you take a look now?

[xedin]

I think this is looking much better, almost there!

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 3759f90

[theblixguy]

@xedin Seems like the filter approach isn't going to work as there's 7 test failures :(

[xedin]

@theblixguy Looks like most of the diagnostics are improvements like:

test/decl/protocol/protocol_with_superclass_where_clause.swift
test/decl/protocol/protocol_with_superclass.swift
and
test/Generics/function_defs.swift

The rest are related to init use, used to be diagnosed by expression diagnostics before and now are diagnosed via new fixes, so diagnostic has to be updated to support that e.g. https://github.com/apple/swift/pull/21830/files#diff-5da71108df45946c4eba89a9cc3a323bR865
doesn't belong in CSDiag.cpp anymore.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 3759f90

[theblixguy]

@xedin Oh! I have updated those diagnostic messages to the improved one. I'll fix the init ones now. The reason I put it there in CSDiag.cpp is because were would end up in diagnoseUnviableLookupResults for that test case as well (i.e. it had the same issue as the example we were discussing earlier). I guess the filtering approach may have fixed it, so I'll take a look now.

[theblixguy]

@xedin I have removed the init code from CSDiag diagnoseUnviableLookupResults, however there's still one issue where the following code emits the wrong diagnostic:

class B {
  init() {}
  init(x: Int) {}
}

class D : B {
  override init() {
    super.init()
  }

  func super_calls() {
    // expected: 'super.init' cannot be called outside of an initializer
    // actual: missing argument label 'x:' in call
    super.init(0)
  }
}

So, dumping the CS makes it clear that the fix is created (actually twice):

Score: 1 0 0 0 0 0 0 0 0 0 0
Type Variables:
  $T0 [lvalue allowed] subtype_of_existential bindings={} @ locator@0x10a043618 [UnresolvedDot@/Users/suyashsrijan/Desktop/test.swift:14:11 -> member]

Active Constraints:

Inactive Constraints:

Fixes:
  [fix: allow access to instance member on type or a type member on instance] @ locator@0x10a043618 [UnresolvedDot@/Users/suyashsrijan/Desktop/test.swift:14:11 -> member]

-----

Score: 1 0 0 0 0 0 0 0 0 0 0
Type Variables:
  $T0 [lvalue allowed] subtype_of_existential bindings={} @ locator@0x10a04dc00 [UnresolvedDot@/Users/suyashsrijan/Desktop/test.swift:14:11 -> member]

Active Constraints:

Inactive Constraints:

Fixes:
  [fix: allow access to instance member on type or a type member on instance] @ locator@0x10a04dc00 [UnresolvedDot@/Users/suyashsrijan/Desktop/test.swift:14:11 -> member]

but again, it's not getting diagnosed (i.e. nothing is called diagnoseAsError() on the fix) and we end up calling diagnoseParameterErrors(). Removing the argument produces the correct diagnostic.

[xedin]

It looks like it might be producing ambiguous solution judging by two solutions you mentioned but it seemingly happening during expression diagnostics, because there is only one type variable which for regular call is too few. I'll checkout your code and try to run it to see what is going on.

[xedin]

Ah I see, this label problem does not get diagnosed via fix, that's why it complains about it first, once that's fixed there is going to be 'super.init' cannot be called outside of an initializer produced.

Let me see if I can fix that to get label problem diagnosed via fix so we can get both diagnostics at the same time.

[xedin]

@theblixguy I've opened #22797 to address this problem. Found one more simplication last moment, re-running tests locally to see if it works, and then I'm going to run tests on that PR.

[theblixguy]

@xedin Great! Once your PR gets merged, I'll add the label diagnostic to that test case, run tests & and push my changes. Hopefully this will be the last one🤞

[xedin]

Sounds like a plan! :)

[theblixguy]

@xedin I have updated the test diagnostic - ran tests locally and everything passes 🎉

[xedin]

@swift-ci please test

[xedin]

@swift-ci please test source compatibility

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - fdfe640

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - fdfe640

[theblixguy]

All tests have passed @xedin 🎉 🎉 🎉

[xedin]

Great! @theblixguy I'm going to squash everything together before merging if you don't mind? A lot of commit in the branch are merge commits and indentation changes.

[theblixguy]

No problem @xedin!

[xedin]

Thanks again for sticking with this @theblixguy!

[theblixguy]

No problem, thanks a lot for your guidance and help as well @xedin :)