Implement "textDocument/implementation" request #126
Conversation
|
It looks like this needs an update now that I also recently added the missing test support code that will allow you to write tests for this, so let's do that. I'd love any feedback you have about using the new APIs. You can find an example test in |
# Conflicts: # Sources/LanguageServerProtocol/ServerCapabilities.swift # Sources/SourceKit/SourceKitServer.swift # Tests/LanguageServerProtocolJSONRPCTests/CodingTests.swift
| occurs = index.occurrences(relatedToUSR: usr, roles: .overrideOf) | ||
| } | ||
|
|
||
| // FIXME: overrided method logic |
There was a problem hiding this comment.
What is this FIXME about?
There was a problem hiding this comment.
Just got copy-pasted with the rest of definition(_:workspace:)
There was a problem hiding this comment.
I don't think it's relevant here.
| try ws.openDocument(ws.testLoc("b.swift").url, language: .swift) | ||
|
|
||
| func impls(at testLoc: TestLocation) throws -> [Location] { | ||
| let textDocument = TextDocumentIdentifier(testLoc.url) |
There was a problem hiding this comment.
I added a shortcut testLoc.docIdentifier
| try XCTAssertEqual(impls(at: testLoc("ClassVariable")), [loc("SubclassVariable")]) | ||
| try XCTAssertEqual(impls(at: testLoc("ClassFunction")), [loc("SubclassFunction")]) | ||
|
|
||
| try XCTAssertEqual(impls(at: testLoc("Sepulcidae")), [loc("ParapamphiliinaeConformance"), loc("XyelulinaeConformance"), loc("TrematothoracinaeConformance")]) |
There was a problem hiding this comment.
The order of these is not guaranteed in the index. Maybe we should change impls to return a Set for testing purposes?
| try XCTAssertEqual(impls(at: testLoc("Trematothoracinae")), []) | ||
|
|
||
| try XCTAssertEqual(impls(at: testLoc("Prozaiczne")), [loc("MurkwiaConformance2"), loc("SepulkaConformance1")]) | ||
| // FIXME: non-ascii characters break Tibs `TestLocation`s |
There was a problem hiding this comment.
What is the issue here?
There was a problem hiding this comment.
I got confused by late night debugging. There is a tibs bug, but I accidentally dodged it here, because "ć" and "Ł" fit into a single utf-16 code unit, even though they aren't ascii. The bug that the test caught is real though and needs investigating.
About the tibs bug - if I put this in a test file
class 😃 {}/*a*/
class B {}/*b*/
class Ć {}/*c*/
class 👩👩👧👧 {}/*d*/
and then print(loc("a").range.lowerBound.utf16index), I get 15 for all of them, which suggests that tibs counts Characters instead of utf-16 code units. I think I should get 16, 15, 15, 25 for the column numbers
There was a problem hiding this comment.
Ah, thanks! I'll fix it.
There was a problem hiding this comment.
Okay, I think I fixed this in
swiftlang/indexstore-db#38
#136
Unfortunately, this will probably expose the latent problem that we are using a utf8 index from the SymbolOccurrence, so your comparisons may start failing. If that's the case, I suggest adding new initializers to Position(badUTF16: TestLocation) and similarly for Location that do the same incorrect behaviour as we do in the implementation. It also makes it easier to find the places that should change when we someday fix the underlying utf8 vs. utf16 issue from our index data.
There was a problem hiding this comment.
Actually, assuming the tests pass we could just land your changes first, and I can fix up any discrepancies in my PR.
| try XCTAssertEqual(impls(at: testLoc("Prozaiczne")), [loc("MurkwiaConformance2"), loc("SepulkaConformance1")]) | ||
| // FIXME: non-ascii characters break Tibs `TestLocation`s | ||
| // try XCTAssertEqual(impls(at: testLoc("Sepulkowate")), [loc("MurkwiaConformance1"), loc("SepulkaConformance2"), loc("PćmaŁagodnaConformance"), loc("PćmaZwyczajnaConformance")]) | ||
| // FIXME: sourcekit returns wrong locations for the function (subclasses that don't override it, and extensions that don't implement it) |
There was a problem hiding this comment.
Could you file a bug for this one? It seems worth understanding and fixing either in the index or in sourcekit-lsp.
There was a problem hiding this comment.
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test macOS |
https://bugs.swift.org/browse/SR-10808
Won't build right now, because it relies on
occurrences(relatedToUSR:roles:)that will be introduced in swiftlang/indexstore-db#32There's a bug where if you satisfy a protocol requirement in the main body of a type, but conform to the protocol in an extension, indexstore-db will give you the location when looking for the protocol requirement implementation.