[Serialization] Give swiftdocs a stable version #19451
Conversation
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Could you provide more context why do we need this version number while it's both forward and backward compatible? |
|
This format is basically semantic versioning, but a file format isn't exactly the same thing as an API, so I didn't mention semver by name in the doc comment. |
|
Build failed |
|
Thanks for the clarification! Can we derive |
lib/Serialization/DocFormat.h
Outdated
| /// | ||
| /// Be very careful when changing this block; it must remain stable. Adding new | ||
| /// records is okay---they will be ignored---but modifying existing ones must be | ||
| /// done carefully. You may need to update the version when you do so. |
There was a problem hiding this comment.
What is the effect of adding a new 'field' at the end of an existing record, could you clarify it in the comments ?
Can we determine how many fields a record has at deserialization so we don't have to update and check for a version number ?
There was a problem hiding this comment.
You can kind of do that – see https://github.com/apple/swift/blob/bae3964c07a976ee0978c0c2400e23e3252e60e8/lib/Serialization/ModuleFile.cpp#L200-L216 – but BCRecordLayout doesn't support it. You have to read records manually in that case. Adding new records isn't really that expensive, though, so I don't think it's too much of a problem to do that instead.
For a stable format, the version number is supposed to be higher-level than just "did I change something". If it makes no difference, it's fine not to update it (and if you really don't want to use it, we could just drop the "minor" version entirely).
There was a problem hiding this comment.
I think it's good that you added the minor version, as an extra thing we may find useful to check, I'm mainly trying to understand if we should update it when adding a new field.
Adding a new field would force us to switch to fully manually reading the record ? Would we be able to do a check and switch to using a different BCRecordLayout to deserialize that record, and could this check occur without needing to update the minor version ?
Pointing in the documentation to the example that you gave on how to manually check for the number of fields would be useful.
There was a problem hiding this comment.
It's unfortunately worse than that: adding a new field will break existing tools, because BCRecordLayout currently asserts that the number of fields matches exactly.
We could change that, of course, or add a new API readIgnoringTrailingFields or something, but the point is that you can only add new fields if there aren't existing tools expecting a fixed number of fields.
lib/Serialization/DocFormat.h
Outdated
| /// | ||
| /// Increment this value when making a backwards-compatible change that might | ||
| /// be interesting to test for. However, if old swiftdoc files are fully | ||
| /// compatible with the new change, you do not need to increment this. |
There was a problem hiding this comment.
I'm probably reading this paragraph wrong but it seems to contradict itself, could you rephrase for clarify, are you making a distinction between "backwards-compatible" and "fully compatible" ? If yes I'd recommend to define in details (and maybe with some examples) what you mean by each.
Unfortunately no. The goal here is to have a version number for a format that's stable across compiler versions. The binary swiftmodule format definitely isn't.
I'm not quite sure what you're asking here. The way the version checks are written, all our tools are compiled with exactly one expected major version for any swiftdoc they read. If that version ever changes, we'll have to decide how the new tools handle the differing major versions, but the old tools will correctly reject the newer generated files. (It's also theoretically possible to decide what version to use when generating the file—say, if a user doesn't use any complicated features—but that's probably not something you'll need, especially since the way your deserialization is written will already ignore records it doesn't understand.) |
|
@jrose-apple thanks so much for doing this! My main concern is for |
ah, so the versioning is entirely transparent to compiler clients like sourcekitd, and its purpose is to separate the evolution of doc-serialization with compiler and module format because in practice it's way more stable, right? |
Yep, that sounds right to me!
Agreed. I'll add more comments based on the discussions we're having now (once they've mostly wrapped up). |
jrose-apple
force-pushed
the
stable-view
branch
2 times, most recently
from
e40652e to
ab731b8
Compare
jrose-apple
force-pushed
the
stable-view
branch
2 times, most recently
from
d8da38a to
bc4fffd
Compare
|
@swift-ci Please smoke test |
No functionality change.
Extract the common utilities into a SerializerBase class, and make a new DocSerializer.
The functionality change in this commit is that the control block in a swiftdoc file is validated rather than just being ignored. Tests in following commit.
We're committing to this as a forwards-compatible format, and in most cases probably backwards-compatible as well!
BCRecordLayout currently assumes that the layout described in source always matches the layout in the bitstream being read. Since we want swiftdocs to be a forward-compatible format, avoid using it for deserialization. (In the future, we may want to augment BCRecordLayout to handle records with more fields than expected. For now, though, this is a sufficient change.)
jrose-apple
force-pushed
the
stable-view
branch
from
bc4fffd to
76e5488
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
We're committing to this as a forwards-compatible format, and in most cases probably backwards-compatible as well! Break it apart from the AST/SIL swiftmodule format, give it its own version number, and…probably not have any problems, because there's nothing complicated here.