[LSPAny] Add automatic encoding/decoding backed by Swift.Codable #41
Conversation
rintaro
force-pushed
the
lspany-codable
branch
2 times, most recently
from
729f244 to
14e9e62
Compare
owenv
left a comment
owenv
left a comment
There was a problem hiding this comment.
Been a long time since I wrote an Encoder/Decoder but this looks correct to me
rintaro
force-pushed
the
lspany-codable
branch
5 times, most recently
from
ce8ddb7 to
b274a41
Compare
| self.textDocument = textDocument | ||
| self.color = color | ||
| self._range = CustomCodable<PositionRange>(wrappedValue: range) | ||
| self.range = range |
There was a problem hiding this comment.
These are drive-by changes. We don't need to initialize the underlying storage.
| } | ||
|
|
||
| // TODO: Remove this extension after every clients migrated to `@CustomCodable<PositionRange>`. | ||
| extension Range: LSPAnyCodable where Bound == Position { |
There was a problem hiding this comment.
This extension is only used from sourcekit-lsp repo now. They should use @CustomCodable<PositionRange> for the properties instead.
There was a problem hiding this comment.
I'd like to keep this PR independent from sourcekit-lsp changes.
I will remove this after sourcekit-lsp PR is merged
Implement `LSPAnyEncoder`/`LSPAnyDecoder` and provide default `init(fromLSPAny:)`/`encodeToLSPAny()` for any `LSPAnyCodable` type that also conforms to `Codable`. Conforming types can rely on auto-synthesized `Codable` implementations or provide custom `init(from:)`/`encode(to:)` methods.
…cked defaults Remove hand-written `init?(fromLSPDictionary:)` / `encodeToLSPAny()` from LSP and BSP types, relying instead on the default implementations introduced in the previous commit that delegate to `LSPAnyDecoder` / `LSPAnyEncoder`.
When encoding an `LSPAny` value via `LSPAnyEncoder`, pass it through directly instead of re-encoding through the generic `Encodable` machinery. Likewise, when decoding into an `LSPAny` target type via `LSPAnyDecoder`, return the stored value directly.
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Very nice, getting rid of some annoying boilerplate
| array.append(value) | ||
| storage = .unkeyed(array) | ||
| } | ||
| func count() -> Int { |
There was a problem hiding this comment.
Should this be a property instead of a function?
| func prepareKeyed() { | ||
| storage = .keyed([:]) | ||
| } | ||
| func set(key: String, value: LSPAnyReference) { | ||
| guard case .keyed(var dictionary)? = storage else { | ||
| preconditionFailure("set(key:value:) only available for .keyed") | ||
| } | ||
| storage = nil // Nil out first so `dictionary` is uniquely referenced (COW). | ||
| dictionary[key] = value | ||
| storage = .keyed(dictionary) | ||
| } |
There was a problem hiding this comment.
Instead of having these functions that are only applicable when the LSPAnyReference in in a certain mode, would it make sense to have separate types for the different cases (single, keyed, unkeyed) and then either unify them through a protocol they all conform to or a common superclass? That way we could ensure that the reference is of the expected type in the type system instead of having preconditions.
There was a problem hiding this comment.
Maybe, but I don't want to spend much time for designing such setup for now.
| reference.set(value: .string(value)) | ||
| } | ||
| func encode<T: BinaryInteger & Encodable>(_ value: T) throws { | ||
| reference.set(value: .int(Int(truncatingIfNeeded: value))) |
There was a problem hiding this comment.
Is truncatingIfNeeded really what we want here? Wouldn’t it be preferable to crash than to silently change the value of an encoded value?
There was a problem hiding this comment.
I will address this in followups
| private func withValueReference<T>( | ||
| forKey key: Key, | ||
| encode: (LSPAnyReference, [any CodingKey]) throws -> T | ||
| ) rethrows -> T { |
There was a problem hiding this comment.
Does this have to be a with-style method? Since we’re operating on reference types anyway, wouldn’t it be easier if this just returned (LSPAnyReference, [any CodingKey])?
Same for UnkeyedContainer.withAppendingReference
There was a problem hiding this comment.
No, but I wanted to align the style of these methods with LSPAnyDecoder.UnkeyedContainer.withCurrentValueAndAdvance where I do want with style method
| dictionary[key.stringValue] != nil | ||
| } | ||
|
|
||
| private func withValue<T>(forKey key: Key, decode: (LSPAny, [any CodingKey]) throws -> T) throws -> T { |
There was a problem hiding this comment.
Similar here: Does this have to be a with-style method instead of just returning a value?
| } | ||
|
|
||
| // TODO: Remove this extension after every clients migrated to `@CustomCodable<PositionRange>`. | ||
| extension Range: LSPAnyCodable where Bound == Position { |
3 participants
Implement
LSPAnyEncoder/LSPAnyDecoderand provide defaultinit(fromLSPAny:)/encodeToLSPAny()for anyLSPAnyCodabletype that also conforms toCodable.Conforming types can rely on auto-synthesized
Codableimplementations or provide custominit(from:)/encode(to:)methods.