Add more hooks for overriding name lookup in the DebuggerClient

[bgogul]

Currently, lldb's REPL mode does not work well with extensions. Specifically, we cannot redefine an extension. Consider the following LLDB repl session:

Welcome to Swift version 5.0-dev (LLVM dcb9eb74a7, Clang 95cdf7c9af, Swift 56a798a3ea).
Type :help for assistance.
  1> struct Foo {}
  2> extension Foo { func f() -> Int { return 1 } }
  3> Foo().f()
$R0: Int = 1
  4> extension Foo { func f() -> Int { return 2 } }
  5> Foo().f()
error: repl.swift:5:1: error: ambiguous use of 'f()'
Foo().f()
^

repl.swift:2:22: note: found this candidate
extension Foo { func f() -> Int { return 1 } }
                     ^

repl.swift:4:22: note: found this candidate
extension Foo { func f() -> Int { return 2 } }
                     ^

As you can see redefining a function in an extension causes an ambiguous use diagnostic. This PR adds additional hooks into the DebuggerClient based on the discussion on the swift forum.

See apple/swift-lldb#1340 for how it is used in lldb.

[davidungar]

This is a great goal! And I think I see that you are trying to pass the distinction about which lookupQualifed function was called into the debug client. But, I think there is likely a more readable way to do it. And I'm involved in refactoring lookup these days, so I have strong feelings about the readability here.

Here's why I think the code could be clearer: When I read the finishLookupIn* functions this PR adds, I had trouble figuring out 1. Why they were file-static, rather than members of the DeclContext or the debug client, and 2. What their purpose was. (A comment in front of each would have helped, but...)

Also, finishLookupIn* are names at the wrong level of abstraction for best readability because they are about the mechanics of the computation rather than what a user might think about.

It seems to me that each of these routines exists only to forward to an appropriate DebugClient routine. So, having criticized this factoring, what do I suggest? (Wait a moment while I scratch my head:)

First, I would rename the existing finishLookup to pruneResultSet, since that is what the comment says it does. But you don't have to do that if you don't want to.

When I look at, for instance, finishLookupInModule, I see that is first, pruning the results (by calling finishLookup), and then allowing the debug client to do something mysterious to the results--adding?, pruning??, replacing??--the name finishLookupInModule offers no clue. So, while I usually love factoring, in this case I cannot find a sensible abstraction for such a routine. Without more knowledge, I would suggest just open-coding the three functions at their call sites. On the other hand, if the debug client functions are pruning, then I would handle that by: renaming the existing finishLookup to pruneResults and adding a function parameter to pruneResults that would be the call into the debugger.

Does this make sense to you? I hope that together, we can figure out how to make this code as clear as possible. Thanks!

[bgogul]

Thanks for the review, @davidungar!

This is a great goal! And I think I see that you are trying to pass the distinction about which lookupQualifed function was called into the debug client.

Yes, I wanted the debugger client to have the context about what kind of qualified lookup is being processed. I considered adding a single interface hook with an appropriate set of arguments, but it seemed cleaner to have three distinct hooks.

But, I think there is likely a more readable way to do it. And I'm involved in refactoring lookup these days, so I have strong feelings about the readability here.

I will be happy to make the changes needed to make it more readable. :)

It seems to me that each of these routines exists only to forward to an appropriate DebugClient routine. So, having criticized this factoring, what do I suggest? (Wait a moment while I scratch my head:)

Yes, that is its sole purpose.

First, I would rename the existing finishLookup to pruneResultSet, since that is what the comment says it does. But you don't have to do that if you don't want to.

Good idea. Done.

When I look at, for instance, finishLookupInModule, I see that is first, pruning the results (by calling finishLookup), and then allowing the debug client to do something mysterious to the results--adding?, pruning??, replacing??--the name finishLookupInModule offers no clue. So, while I usually love factoring, in this case I cannot find a sensible abstraction for such a routine. Without more knowledge, I would suggest just open-coding the three functions at their call sites. On the other hand, if the debug client functions are pruning, then I would handle that by: renaming the existing finishLookup to pruneResults and adding a function parameter to pruneResults that would be the call into the debugger.

I have moved the body of these functions into the corresponding lookups. Given that the signature allows the debug client to do anything with decls, I prefer not to rename it to pruneResults. I have updated the comments in DebuggerClient.h appropriately. That said, are you happy with finishLookupIn... or do you prefer something else (say finalizeLookupIn...)?

Does this make sense to you? I hope that together, we can figure out how to make this code as clear as possible. Thanks!

Yes. Thanks.

[bgogul]

@davidungar I have addressed your comments and suggestions. Can you take a look when you get a chance, please?

[davidungar]

Is there a typo? "Only" should be "Once"?

[bgogul]

You are right! It should be "Once". Fixed it now.

[davidungar]

Sorry I didn't see your ping earlier. (Feel free to email me directly next time.) Looks fine to me now, thanks! I did spot one typo in a comment--nothing you wrote, though. Not critical, but would be nice to fix it if you agree.

[bgogul]

Thanks, @davidungar !

I fixed the typo and this PR is all set to be merged in, but I don't have permissions. Can you trigger tests and merge it in, please?

[bgogul]

@swift-ci please smoke test

[bgogul]

@davidungar, can I go ahead and merge this PR?