It seems that lldb -r does not work well with extensions. Consider the following 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. The diagnostic is issued in the ConstraintSystem::salvage(...) function when type checking the expression Foo().f()
Is interfacing with a DebuggerClient to patch up the results a reasonable solution?
If not, what is a better way to take care of this issue?
The REPL is designed to allow redefinitions. Since, as you say, the compiler should otherwise complain at the redefinition, in the REPL it should work as @bgogul describes.
Just for some context, I tried the sequence in swift's in-built REPL and it works as expected. Therefore, this is only an issue for lldb -r.
I added a really uglyhack to the tensorflow branch that seems to work. Obviously, this is not the correct long-term solution and is extremely brittle. However, is doing something in ConstraintSystem::salvage the right thing to do?
@Jim_Ingham, is there a better way to deal with this issue?
All this is handled through lldb's SwiftPersistentExpressionState. That maintains the list of variables you have defined, but it also maintains a list of the non-variable Decls that were made in the REPL (in the m_swift_persistent_decls). Then we hand out decls from this list when Swift asks LLDB's DebuggerClient for lookup by name. The debugger client for the REPL is the LLDBREPLNameLookup class, and of we're providing names through its lookupAdditions method).
So @bgogul is mostly right, except we are using a DebuggerClient to sort out redefinitions already.
The way it is supposed to work is that SwiftPersistentExpressionState is asked for the persistent Decl's after parse (by calling lookupAdditions). lookupAdditions calls SwiftPersistentExpressionState::GetSwiftPersistentDecls which returns two lists, the Decls in the persistent store, and the ones we've found in the current compilation. Then when the LLDBREPLNameLookup is handing out Decls for Name back to Swift, it should prefer equivalent Decls from the current compilation. Then once we're done we put the Decl's back into the SwiftPersistentExpressionState's list of persistent Decls, overwriting the old equivalent Decls.
My guess is that the determination that two extension functions for the same class are equivalent isn't correct? Or maybe we don't notice them as a thing of interest in the current compilation and don't put it on that list. Or there's some other breakage along that line.
But anyway, it shouldn't be hard to watch Decls come in and out of the SwiftPersistentExpressionState, and into lookupAdditions and see where we're mishandling "method in extension".
This might be one of the reasons. The current implementation only registers VarDecls and ValueDecls present at the top level of the module into SwiftPersistentExpressionState. (ExtensionDecl is neither a VarDecl nor a ValueDecl.) It is also not clear how we can deal with ExtensionDecl here as we cannot simply replace the previous extension.
Also, note that the ambiguity diagnostic is issued when type checking (see SwiftExpressionParser:1502) the expression Foo().f() after parsing is done. Given this context, my question was if it makes sense to with the DebuggerClient from the ConstraintSystem::salvage method as well?
My guess was that we passed both types back to swift when it asked for Decls for name, and if we had instead passed the right one, then the type checker wouldn't have balked. I assumed that once a method gets added in an extension, it just becomes another method of the class so you could always hand them out and check them one by one. Is the containing extension important?
Anyway, if we have to pass whole extensions rather than decl's for each thing in the extension, we're going to have to merging the contents of new and old ones. After all, you could add an extension with two methods, and then another that redefines only one of them...
@jrose or someone who doesn't just play a swift parser engineer on TV would be a better source for the right strategy to handle this situation.
The containing extension might have constraints on it, so it's not always possible to just treat members as if they were defined in the original declaration.
We actually are talking about overload resolution at this point, but in theory that's no different from what has to work for top level code. I don't know how LLDB's handling that, but I'm pretty sure it's not as late as salvaging a constraint system.
From what I can see in :debug constraints on mode, each of the extensions appears in the different module e.g. when I try from example from first comment in REPL it would be REPL_1.(file).Foo extension.f() and REPL_2.(file).Foo extension.f() that's why name lookup treats these as different declarations and constraint system just operates on what ever results name lookup returned for the given name.
It seems like the way to solve this would be to add some special logic to declaration checker to handle scenarios like that, WDYT @Douglas_Gregor?