[Sema] FixIt for override func that should be override var and vice-versa

[Rajveer100]

Fixes #57499

This adds override_property_not_method and override_method_not_property notes by performing
a member lookup for the respective diagnostics.

[Rajveer100]

@AnthonyLatsis I have managed to get the diagnosis to work, and getting the desire output with the desired note. While am providing the fixit to the same and adding tests, please review the changes and correct any other things required.

[Rajveer100]

How to debug a Fix-It, meaning how to see the fixit and see the new form?
(Like we would normally click it and see the code change in the IDE)

Or if there is a way to print the modified form in the debugger?

[AnthonyLatsis]

How to debug a fix-it

See https://github.com/apple/swift/blob/main/docs/Diagnostics.md#diagnostic-verifier

[Rajveer100]

Alright, I managed to add the fixit as well, is there anything else I must add to this fixit:

  override func canBecomeFirstResponder() -> Bool {}
           ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           var canBecomeFirstResponder: Bool

Maybe I should also probably also clear the body of the computed property | function with an addition of { <#code#> } after the signature, to let the user type the body for the override accordingly?

[xedin]

I have left some comments inline and please add test-cases to test/decl/class/override.swift and test/decl/inherit/override.swift that check that newly added fix-its and notes.

[Rajveer100]

Hey, thanks! @xedin
I will make the changes and add the test files.

[xedin]

Happy to help! Please press "re-request view" once you feel like that PR is ready for another round of review!

[Rajveer100]

@xedin What would be the right (or appropriate) way to add tests to the respective files you mentioned?

[xedin]

All new tests are usually appended to the end of the test file, you should mention Github issue and/or radar number and wrap in either do or func declarations to avoid polluting global namespace with names that are local to tests.

[Rajveer100]

Alright!

[Rajveer100]

@AnthonyLatsis @xedin

The -verify frontend flag which is told to be used in order to check the expected note or fixit (in this case) must be added or passed exactly where in the Xcode workspace?

[AnthonyLatsis]

The commands that are run for a given test file are usually specified at the top of the file using RUN: directives. The files Pavel mentioned should already be passing -verify in at least one of those invocations. Also, you should not be attempting to run tests in Xcode.

[Rajveer100]

I am aware of running tests, as I had done them previously in my pull requests via this command:

utils/run-test --lit ../llvm-project/llvm/utils/lit/lit.py \
  ../build/Ninja-RelWithDebInfoAssert/swift-macosx-$(uname -m)/test-macosx-$(uname -m) \
  --filter="MyTest"

But regarding the flags, I thought some extra work was needed as this is the first time I am adding test cases, my doubt was whether I should use certain -flag in that command in terminal, or is it some flag that needs to be added in my workspace and that would throw something and I don't know what exactly the RUN command does as I had seen it before too.

So what would be the appropriate command to execute, let's say, for the file test/decl/class/override.swift:

Would it be the same way as the command I used earlier (mentioned on top of this comment)?

[AnthonyLatsis]

If the -verify flag is present in one of the RUN: lines (which it ought to in these particular files), then no extra work is needed. lit does not do anything special — it simply executes the RUN: lines. Each test file is responsible for supplying its own testing commands using this directive.

[Rajveer100]

Ahh I see! Thanks!

[Rajveer100]

@xedin Could you please review this, I have mentioned few in line comments as well to describe certain issues.

[Rajveer100]

@xedin
Request you to look into this thread.

[Rajveer100]

@xedin
Been quite long since we last spoke about this issue! Any advice based on the last conversation we had, if I correctly remember I had used the overkill constraint system for the lookup, which was still working correctly, but as you suggested later to use lookup direct which is where the last problem occurred as here.

[xedin]

I don’t have any other suggestions besides running your changes under debugger and trying to figure out the root cause of the crash, the approach we have discussed should be sound, it’s the a matter of figuring out how exactly to use the APIs.

[AnthonyLatsis]

@Rajveer100 Are you still working on this?

[Rajveer100]

Been a while since I last worked on this, getting back to it :)

[Rajveer100]

@xedin
Could you review this as well :)

[AnthonyLatsis]

I think a better approach is to handle this case in OverrideMatcher (where name lookup is already done for us) during OverrideCheckingAttempt::BaseName, and to ignore any other differences between the two declarations. Just always emit a tailored diagnostic for var vs. func, and leave the fix-its for another time. Quality fix-its are a deeper rabbit hole as things stand because the current logic is both exclusive and dependent on declaration kind equality, and there are plenty of rules aside from type matching for whether one property or method is a valid override of another.

---

That said, a great improvement to your current change would be to extract the diagnosis into a function.