LLDB REPL does not work well with extensions


(Gogul Balakrishnan) #1

Hi,

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?


(Martin R) #2

An extension can add a method, but not override an existing method, so the REPL should already complain at the second declaration of func f().


(Xiaodi Wu) #3

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.


(Gogul Balakrishnan) #4

Thanks for the reply, @xwu and @Martin.

Yes, the compiler complains about the second definition, but the REPL should allow redefinitions.


(Gogul Balakrishnan) #5

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 ugly hack 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?


(Jordan Rose) #6

If anything works in Swift's built-in REPL, it's as likely to be by accident as by design. Please don't take hints from it.

(I do support finding a way to make this work.)


(Jim Ingham) #7

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".


(Gogul Balakrishnan) #8

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?


(Jim Ingham) #9

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.


(Jordan Rose) #10

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.

  1> func foo() { print("0") }
  2> func foo(_ x: Int) { print("1") }
  3> foo()
0
  4> foo(1)
1
  5> func foo(_ x: String) { print("2") }
  6> foo(1)
1
  7> foo("2")
2
  8> func foo(_ x: String) { print("3") }
  9> foo("2") 
3
 10> foo(2)
1

Tagging in @Douglas_Gregor and @xedin instead.


(Gogul Balakrishnan) #11

@Douglas_Gregor and @xedin, any suggestions on how to proceed on this?


(Pavel Yaskevich) #12

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?


(Gogul Balakrishnan) #13

Thanks for taking a look, @xedin. My understanding is that each REPL line is compiled as a different module as well.

I dug around a bit after your comment and see two options that might work to avoid ambiguous names returned on a lookup:

Do either of these approaches sound reasonable?


(Pavel Yaskevich) #14

Yes, that sounds reasonable to me!


(Pavel Yaskevich) #15

I'm not an expert in that part of code but I think #1 might be the most viable approach.


(Gogul Balakrishnan) #16

@xedin, I created a PR to deal with this problem based on our discussions:

I have assigned the swift PR for you to review. Please re-route it to an appropriate person if needed.

Thanks a lot for the help!


(Pavel Yaskevich) #17

Thank you, @bgogul! I've requested @Douglas_Gregor and @David_Ungar2 to take a look at your PR.