Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KDL 2.0.0 compliance for implementations #372

Open
8 of 34 tasks
zkat opened this issue Feb 7, 2024 · 29 comments
Open
8 of 34 tasks

KDL 2.0.0 compliance for implementations #372

zkat opened this issue Feb 7, 2024 · 29 comments
Labels
epic This issue is a tracking issue for other issues. help wanted Extra attention is needed implementations

Comments

@zkat
Copy link
Member

zkat commented Feb 7, 2024

This issue is for tracking full test suite compliance for KDL implementations that support the new KDL 2.0.0 spec.

Implementations:

@zkat zkat added help wanted Extra attention is needed implementations labels Feb 7, 2024
@zkat zkat pinned this issue Feb 7, 2024
@zkat zkat added the epic This issue is a tracking issue for other issues. label Feb 7, 2024
@zkat zkat mentioned this issue Feb 7, 2024
@hdhoang
Copy link

hdhoang commented Feb 8, 2024

please consider pinging tree-sitter-kdl. thanks

@eugenesvk
Copy link
Contributor

What's the file marker that differentiates v1 from v2? File extension .kdl2? Inner top-line string kdl v=2? Something else?

@zkat
Copy link
Member Author

zkat commented Feb 8, 2024

There's no file marker, but the data model is exactly the same--it should be very easy to heuristically detect whether you're looking at 1.0 or 2.0 and react accordingly if you're working on a dual parser, and the resulting data will be the same in both cases.

@eugenesvk
Copy link
Contributor

What is the heuristic in a non-code syntax file to tell apart a syntax error of #invalid_node_in_a_v2_file vs a #valid_node_in_a_v1_file? I've only seen extensions and first line matches

I'd say that's an oversight of the v2 version, why not have something like
/-v 2 or
/-kdl v=2

@zkat
Copy link
Member Author

zkat commented Feb 8, 2024

you detect which one parses and go from there, basically :)

So, if you run into #invalid_node_in_a_v2_file, you set your parser to 1.0 mode, and if anything else in the file disagrees, it's a syntax error. Ditto the other way around.

Personally, I'm just going to support 2.0 for parsers I maintain going forward instead of trying to support both versions. Folks can use older versions of the parser to use 1.0 until they're ready to migrate.

When discussing it, we came to the conclusion that the burden of always having to tag your 2.0 files wasn't worth it when KDL is still relatively low usage and we expect 2.0 to quickly become the dominant format going forward.

@eugenesvk
Copy link
Contributor

eugenesvk commented Feb 8, 2024

I can't really use that trick since this isn't a parser in a programming lanuage where you have that kind of flexibility

also you can have both v1 and v2 errors, so this rule won't help you determine the version

the burden of always having to tag your 2.0

That's fine, put the burden on the v1 users! Or use a file extension. Also, after KDL becomes (world)dominant there might be v3! This future-proofs it.

So you'd have a rule like: the file is assumed to be of the latest version unless specifically tagged or

@tabatkins
Copy link
Contributor

That's fine, put the burden on the v1 users!

We can't. v1 users are already not tagging their content as v1. And adding something now for users to produce v1 content going forward isn't worth anyone's time; they should be writing v2, as that's what most parsers will be expected to (likely exclusively) accept going forward.

Also, after KDL becomes (world)dominant there might be v3!

And if there are, we can figure out compat at that point.

Note that "make v1 documents tag themselves, v2 can be assumed" is not compatible with a future v3 either; it'll mean that v3 also has required tagging (since a missing tag would indicate v2). So we don't need to worry about it right now; if a v3 ever appears, it'll need to be tagged anyway, so we can introduce the tag then.

the file is assumed to be of the latest version unless specifically tagged

This absolutely does not work, fwiw, unless the language is carefully designed to be back/forward-compatible, and losing data is acceptable. (CSS, for example, has this model.) A data format can't really do this.

@eugenesvk
Copy link
Contributor

We can't. v1 users are already not tagging their content as v1.

And as long as they don't encounter any v2 requiremens, they can continue to do so.

And adding something now for users to produce v1 content going forward isn't worth anyone's time; they should be writing v2

And what about all v1s that have already been written? I don't get it, why should there be no mechanism to differentiate them?

And if there are, we can figure out compat at that point.

then it'd be much more painful due to higher scale

This absolutely does not work

Well, you could add or the parser has heuristically determined the version. My point is about a SPEC'ed mechanism for an explicit (optional) signal so that

@hdhoang
Copy link

hdhoang commented Feb 9, 2024

There are also emacs support packages at:

@juh9870
Copy link

juh9870 commented Feb 11, 2024

you detect which one parses and go from there, basically :)

This is not possible. There are files valid both in v1 and v2 that produce different data.

Example from my app:

enum "Item" editor="enum" port="hollow" {
    generic "some" "Item"
    const "none" null
}

If I understand the v2 change correctly, this will change meaning with v2, resulting in the const item having the second argument be a string instead of null, and my app (node-based configuration generator) will parse it just fine without any errors, but it will then propagate into other parts of my app workflow, and result in the app output having "null" strings all over the place instead of the expected null. I am lucky since my app is still in development, and I'm the only user, but apps with any measurable user base going to encounter silent troubles like this.

@zkat
Copy link
Member Author

zkat commented Feb 12, 2024

No, v2 makes null an illegal string, so this will actually be a syntax error, telling you to use #null instead. It would otherwise be a perfectly valid v2 document.

@zkat
Copy link
Member Author

zkat commented Feb 12, 2024

The same goes for the other keywords: #true and #false are both prefixed now as well.

Likewise, there's the very rare "gotcha" where you might have a node name called #null, that would be a syntax error in v2.

Scanning again through the changelog, the vast majority of changes make v2 more strict or use incompatible-with-v1 syntax, so it would be an error in either one.

The only change I can think of that could "silently" change the data without being a syntax error in v1 or v2 is the new multiline string indentation stripping, where the following string means two different things whether you're in v1 or v2:

node "
  indented string
  "

in v1, this yields:

node "\n  indented string\n  "

and in v2:

node "indented string"

There's no way to distinguish which of these is v1 and v2.

@juh9870
Copy link

juh9870 commented Feb 12, 2024

I see, thanks for the clarification. As long as no v1 document can change meaning in v2 without syntax error, migrations would be much smoother.

@tjol
Copy link
Contributor

tjol commented May 26, 2024

As ckdl is a single-pass streaming parser (and I want to keep it that way), I couldn't do a "if KDLv2 gives a syntax error, retry with the v1 parser" loop. Still, it was pretty straightforward to build a parser that starts out agnostic about the KDL version until the first time it sees a construct that is only valid in one version or the other.

The (experimental/draft) hybrid parser in ckdl processes all valid KDLv2 documents correctly and (currently) only compromises on KDLv1 compatibility in a few cases (that I know of), if they occur before any v1-only construct:

  • multi-line strings that happen to be valid in both versions (unavoidable)
  • nodes named #null, #inf, etc. (this could be fixed with some effort)
  • characters illegal in KDLv2 (could be fixed with some effort, probably shouldn't be)

I really wouldn't expect other hybrid parser implementations to have a compatibility story worse than this.

@Patitotective
Copy link
Contributor

Hi, I'm updating kdl-nim to match KDL v2 draft 4 and from what I see on the SPEC, the BOM (U+FEFF) code point is only allowed as the first character in a KDL file, so that means it is disallowed anywhere else and I think it should be included in the Disallowed Literal Points section in the SPEC.

@tjol
Copy link
Contributor

tjol commented Nov 21, 2024

@Patitotective As far as I can see, it is included in that section. https://github.com/kdl-org/kdl/blob/kdl-v2/SPEC.md#disallowed-literal-code-points

@Patitotective
Copy link
Contributor

@tjol You're right! Seems like I was confused because the tag for 2.0.0-draft.4 is not actually the latest draft but 9 months old, I'll be looking at latest commit then 😅

@zkat
Copy link
Member Author

zkat commented Dec 20, 2024

@juh9870 worth noting here, by the way, that as of the latest (probably final) draft of 2.0.0, parsing v1 vs v2 is completely unambiguous. Multiline strings are now syntactically distinguishable and the only documents that will parse as both v1 and v2 are documents that will result in the exact same data.

@juh9870
Copy link

juh9870 commented Dec 20, 2024

That is amazing! Sadly, my app will likely have to release with v1 specs support only, since the kdl lib I'm using is no longer getting updates (knuffel)

@zkat
Copy link
Member Author

zkat commented Dec 20, 2024

@juh9870 There's now a fork of knuffel over at https://github.com/TheLostLambda/knus by @TheLostLambda, and they're planning on adding v2 support at some point.

I'm also thinking about somehow integrating knus directly into kdl-rs so that the main parse happens through kdl-rs, but you can still use the serde-style deserialization from knus.

@TheLostLambda
Copy link

Hi @zkat! Personally I'd love to integrate the two — I've not quite been keeping up, but does kdl-rs handle 2.0 and 1.0 as a fallback currently?

I think a sort of derive feature would be fantastic to have in the KDL reference implementation!

@zkat
Copy link
Member Author

zkat commented Dec 20, 2024

does kdl-rs handle 2.0 and 1.0 as a fallback currently?

@TheLostLambda the main branch does. I need to fix some formatting-related stuff that has already been there for a while, and then I'm gonna do another 6.0 prerelease, though. If it happens after tomorrow, it's very likely not even going to be a prerelease, since the 2.0 spec is getting published tomorrow barring any final, sudden blockers.

What I want for kdl-rs is to have a "proper" Serde implementation that works kinda like quick-xml, but that's a lot of work and serde is hard, so I was thinking of just having a feature flag that pulled in knuffel (now knus) as a dependency, and then soft-fork knus-derive to just make it so attribute labels are kdl instead of knus to make things feel more "native". But that would only be there until the serde implementation is finally in place.

@TheLostLambda
Copy link

@zkat Sounds like a good plan! I'll take a look and try to gauge the effort involved in some sort of quick-xml-like solution and compare it to integrating knus!

I'd also prefer things to be done via serde if possible! I'll check it out!

@zkat
Copy link
Member Author

zkat commented Dec 20, 2024

@TheLostLambda yeah that sounds good.

Though honestly, I think I'm going to semi-integrate knus as a stopgap because I want serde-like behavior now, and even though serde is the ideal, it's going to take a lot of time and effort to write. And integrating knus is going to be practically trivial (including having it "support" v2).

For that latter piece, my evil plan is to parse a v2 doc normally, then format it into a valid v1 string if it completes the parse, and then pass it to knus for the data bit. Not the most efficient thing in the world, but it means you get kdl-rs' error reporting, which is REALLY rich (it can resume parsing and collect most errors in a single pass, instead of one at a time). knus, then, would effectively never fail, or shouldn't, unless there's a bug, because bad kdl will never make it to it, so I don't have to worry about translating its errors or anything. Writing a v2 -> v1 translation layer is fairly easy if your goal is just to convert it to a string.

@TheLostLambda
Copy link

TheLostLambda commented Dec 20, 2024

@zkat I'm a big fan of that evil plan! I'm happy to help with that if needed!

The fork of knus-derive could be just kdl-derive and should be easy to change knus -> kdl if you'd like me to do that?

Is your plan to have kdl depend on the new kdl-derive and existing knus code and for those to stay outside of the kdl-rs repo, then down the line write the serde stuff in the main repo dropping the dependencies?

EDIT: Also, does the V2 -> V1 conversion exist already?

@zkat
Copy link
Member Author

zkat commented Dec 20, 2024

@TheLostLambda I'm perfectly happy with kdl-derive to exist separately. It really does just need to be knus -> kdl. I also think it might be prudent to have it in the kdl-rs repo itself, though. It would need to be behind a feature flag, too, which is prominently marked as unstable because I will absolutely be removing it when serde support comes along.

EDIT: Also, does the V2 -> V1 conversion exist already?

Not yet. I just did the v1 -> v2 conversion last night and tbh it would really not be a lot of work at all, especially since we don't need to preserve formatting/comments/etc when passing things down to knus.

All that needs to be done is implement the inverse of this From impl. The rest is just calling .clear_fmt() on the resulting v1 doc, and then stringifying it, and we're good to go.

@zkat
Copy link
Member Author

zkat commented Dec 21, 2024

Anyway I'm working on the -> v1 formatter now. Hopefully ready tonight

@danini-the-panini
Copy link
Contributor

@zkat I checked off kdl-rb in the list, I hope that's ok.

Also can we add Swift to the list: danini-the-panini/kdl-swift#1

@zkat
Copy link
Member Author

zkat commented Dec 24, 2024

@danini-the-panini done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic This issue is a tracking issue for other issues. help wanted Extra attention is needed implementations
Projects
None yet
Development

No branches or pull requests

9 participants