Consider making types non_exhaustive

[Marwes]

The need to make a breaking release every time a new field is added to a type causes a lot of churn in dependent crates (see for instance brendanzab/codespan#124).

We could reduce this significantly (perhaps completely) by making all enums and structs non exhaustive through adding an extra variant to all enums and an extra field to all structs which is not meant to be referred to directly.

pub enum Enum {
    A,
    B(String),

    #[doc(hidden)]
    _NonExhaustive,
}

#[derive(Default)]
pub struct Struct {
    pub a: i32,
    pub b: String,

    #[doc(hidden)]
    #[serde(skip)]
    pub _non_exhaustive: (),
}

Uses of any enums would then need to add a catch all case on each match which errors on unknown variants and use Struct { a, b, .. } when matching on structs. Further, constructing any struct would require Default::default() + functional record update which is perhaps the biggest ergonomic loss.

Struct {
    a: 1,
    b: "".into()
    .. Default::default(),
}

I haven't decided yet if I think being able to move to 1.0 is worth the ergonomic loss but I wanted to float the idea here.

@Xanewok
@matklad
@brendanzab
@autozimu
(more?)

[Marwes]

There is a #[non_exhaustive] which is in FCP rust-lang/rust#44109 but since it doesn't allow for structs with the attribute to be constructed we can't use it anyway (well, enums could still use it I believe)

brendanzab/codespan#124 (comment)

[CAD97]

Note that with private fields or #[non_exhaustive] either, FRU doesn't work with structs, so construction would require constructors or builders rather than structural construction.

[matklad]

I feel like there's no really good solution here. Backwards compatibility of the wire format does not translate nicely to the backwards compatibility of the Rust API (mostly, to due to the impedance mismatch between TypeScript's structural and Rust's nominal type systems).

My current view is that, instead trying to share lsp types as a whole, crates that act as building blocks for LSP servers/clients should just vendor the relevant types. For example, my lsp-server crate does not use lsp_types, and instead just looks manually for initialized, exit notifications.

And yes, I totally did make a typo there once, which broke rust-analyzer, but, with the typo fixed, I feel like this code won't be changed in the future.

Similarly, crates for diagnostics could vendor only diagnostics-related structs, and crates for VFS can vendor only change-notification related structs.

In other words, I advocate for "worse is better" approach in this instance :)

[Marwes]

Note that with private fields or #[non_exhaustive] either, FRU doesn't work with structs, so construction would require constructors or builders rather than structural construction.

Yes, #[non_exhaustive] can't be used (#117 (comment)). But having a public, #[doc(hidden)] field does work with FRU (forgot to mark any of the struct fields with pub).

Some simple types like https://docs.rs/lsp-types/0.61.0/lsp_types/struct.Location.html could fairly reasonably be seen as types which won't have fields added and could be spared the non-exhaustiveness. After all even if they do get a field it is not the end of the world if a 2.x has to be released, we just need to make breaking releases unlikely enough to not be much of an issue.

[Xanewok]

Since these types are inherently structural, maybe it'd make more sense to wait for rust-lang/rfcs#2584 instead?

[Marwes]

@Xanewok I don't see how would that help, what am I missing? Most types already have names in the LSP definition in any case.

[Marwes]

I feel like there's no really good solution here. Backwards compatibility of the wire format does not translate nicely to the backwards compatibility of the Rust API (mostly, to due to the impedance mismatch between TypeScript's structural and Rust's nominal type systems).

It isn't really structural vs nominal that is at play here, just that typescript (or in a way just plain JSON which is the actual data format) have data which are free to be extended with additonal fields or cases.

My current view is that, instead trying to share lsp types as a whole, crates that act as building blocks for LSP servers/clients should just vendor the relevant types. For example, my lsp-server crate does not use lsp_types, and instead just looks manually for initialized, exit notifications.

That works well in many cases such as that where the crate doesn't leak the LSP-types outside of its API (notably, lsp-server could just use an arbitrary version of lsp-types without affecting any users of the crate other than potentially needing to compile multiple versions of lsp-types).

It is always possible to vendor the types when needed, the question here is whether doing this change, which would make 1.0 possible, does not adversely affect crates using lsp-types when they do not need interop via lsp-types

[Xanewok]

It isn't really structural vs nominal that is at play here, just that typescript (or in a way just plain JSON which is the actual data format) have data which are free to be extended with additonal fields or cases.

You're right - apart from structural types we'd need structural subtyping (which is, rather, what makes these TS-side definitions extendable IIUC), which I believe is out of scope in the RFC I linked previously. Sorry.

[Marwes]

I looked it this a bit, but there are types which have fields without a sensible default so we can't use Default + functional record update (FRU).

Avoiding churn could perhaps be avoided by crates specifying a version range instead. Still forces some churn, but it would only be for crates like codespan and not downstream users of codespan (@brendanzab )

[brendanzab]

Yeah, I'm really thinking I want to avoid direct dependencies on lsp-types alas, which might cause me to want to deprecate both codespan and codespan-lsp entirely in favour of just supporting codespan-reporting. It just causes too much churn for me to keep up with. :(

[brendanzab]

Could you perhaps explain/give an example for what you mean by "specifying a version range instead" - I feel like I missed something there.

[Marwes]

In Cargo.toml you can do lsp-types = "0.70>=,<0.81". Since codespan-lsp doesn't construct any types that has changed recently it should work with a wide range of versions. Still forces one to update the maximum version every now and then but that can be released as non-breaking versions.

[brendanzab]

Ahhh, that is indeed handy!

[brendanzab]

Yeah, I'd be pretty conservative with the version bounds, only bumping when asked, and only specifying it up to a currently released version.

[brendanzab]

I'm wondering if we should put this in the README: "Note that due to frequent updates, we recommend library authors to specify a version range... (post example, with guidelines for how and when to update bounds)". cc. @ebkalderon

[ebkalderon]

This sounds like a good solution! I think this could be a great solution for codespan-lsp, which only constructs two relatively unchanging types as part of its public interface. I'm not completely sure about tower-lsp, however, since it couples closely to lsp-types by design and uses quite a few of its types internally. I'll do a bit of testing to make sure the >=0.73,<0.81 version range works as expected.

[brendanzab]

I probably wouldn't use >=0.73,<0.81 because lsp-types is only on 0.73 right now. That's kind of what I was saying about guidelines. For >=0.73,<0.81 you're kind of asserting that all of the changes from 0.73 to 0.81 will work with tower-lsp (based on what you're using in that library), but you can't really assert that if those versions haven't been published yet! So if Marwes publishes a change in 0.74 that breaks tower-lsp, then consumers of tower-lsp won't be able to compile their code.

[brendanzab]

Tbh I still think there might be value in making the definitions in lsp-types forwards-compatible if reasonably possible (without making the datatypes too annoying to work with), because the it may help to make this kind of decision a tad less error prone.

[ebkalderon]

Makes sense to me, yeah. I like your idea of selecting a version range from a past version up to the currently released version. I think that could be a good strategy, provided that the number of types your library directly references or constructs is relatively small.

[brendanzab]

I've made a PR on codespan-lsp that uses a version ranges for lsp-types. I'm still rather wary of this though, as it could potentially break cargo update if people get it wrong. This is pretty unforgivable from my perspective (have had enough bad experiences with non-Cargo package managers recently that tend to do this).