Prefix LSP/BSP extension methods with sourcekit/#48
Conversation
rintaro
force-pushed
the
sourcekit-extensions
branch
9 times, most recently
from
a8f2f64 to
e63179f
Compare
|
|
||
| /// Set to `true` once the peer successfully responds to a legacy method name, | ||
| /// after which requests that have a legacy name are sent using that name directly. | ||
| private let prefersLegacyMethodNames: AtomicBool = .init(initialValue: false) |
There was a problem hiding this comment.
Should we have a per-request name tracking here in case the client has migrated one request name but not another? I don’t think that we should make any cross request name assumptions.
There was a problem hiding this comment.
I thought clients should migrate all at once. But yeah, per request name might be safer.
There was a problem hiding this comment.
Now this is methodNamesPreferringLegacyName: ThreadSafeBox<Set<String>>
| /// | ||
| /// Calls are recorded in `calls`. `responses` maps method names to explicit | ||
| /// results; any method not in the dictionary defaults to `.methodNotFound`. | ||
| private final class MockConnection: Connection, @unchecked Sendable { |
There was a problem hiding this comment.
The @unchecked Sendable seems not correct to me. If we need it, we should have a very strong argument for why it’s needed.
There was a problem hiding this comment.
It's just for tests. It's safe because we never use an instance concurrently. I don't think it's worth to make it real Sendable.
There was a problem hiding this comment.
I don’t think we should set a bad example / bad precedence here with @unchecked Sendable. I think it would be fairly simple to store all the mutable state in a struct and then just put it in a ThreadSafeBox to get proper concurrency safety.
| @Suite struct LegacyNameFallbackConnectionTests { | ||
|
|
||
| // No legacyName: pass through regardless of the reply – no retries, flag never set. | ||
| @Test func testNoLegacyMethods_passesThroughWithoutRetry() async { |
There was a problem hiding this comment.
The mixed snake case and a snake case seems like an odd naming choice to me.
There was a problem hiding this comment.
The convention here is test<WhatToTest>_<expectedOutcome>. They are originally generated names, but I thought it's kinda nice. But I agree it's not a convention we use, and I'm happy to fix it.
There was a problem hiding this comment.
I would prefer to follow the conventions that other repos in the swiftlang org have (eg. swift-build) for consistency and I don’t think they use underscores. Correct me if I’m wrong.
rintaro
force-pushed
the
sourcekit-extensions
branch
from
e63179f to
4b8d2b2
Compare
- Rename all SourceKit-LSP–specific LSP/BSP requests and notifications to use the `sourcekit/` prefix (e.g. `workspace/tests` → `sourcekit/workspace/tests`) - Add `method: String` parameter to `Connection.send<R>(_:method:id:reply:)` defaulted to `R.method`, but overridable with legacy names. - Add `legacyNames: [String: String]` parameter to `MessageRegistry.init` so the registry dispatches incoming messages from existing clients using either current or legacy method names. - Add `LegacyNameFallbackConnection`: a `Connection` wrapper that retries requests with the legacy name on `methodNotFound`, with a global flag so subsequent requests skip the primary name once a legacy peer is detected.
rintaro
force-pushed
the
sourcekit-extensions
branch
from
4b8d2b2 to
857a57f
Compare
2 participants
sourcekit/prefix (e.g.workspace/tests→sourcekit/workspace/tests)method: Stringparameter toConnection.send<R>(_:method:id:reply:)defaulted toR.method, but overridable with legacy names.legacyNames: [String: String]parameter toMessageRegistry.initso the registry dispatches incoming messages from existing clients using either current or legacy method names.LegacyNameFallbackConnection: aConnectionwrapper that retries requests with the legacy name onmethodNotFound, with a flag so subsequent requests skip the primary name once a legacy peer is detected.see swiftlang/sourcekit-lsp#2632