package manager: enforce semver between minor/patch versions by using decl tests

[kristoff-it]

Issue #404 proposed enforcing semver by looking at differences in pub function methods. For example, making a new minor release where a pre-existing function gains a new argument is most definitely an error under semver semantics (for versions >= 1).

The reason why that proposal was abandoned was because of false negatives due to the fact that the meaning of "public API" changes from project to project: source-level compatibility is expected (ie updating doesn't require changing any code), but what about ABI compatibility? For some projects it matters, for some others it might not.

This proposal offers a general way of approaching this problem in light of #7923 (which I will refer to as "decl tests" from now on). Since we consider decl tests part of documentation, I propose to use them as part of the public contract in this way:

Whenever a new minor/patch release is being packaged, we warn the programmer if we find any meaningful difference between the decl test of any public declaration compared to the previous version. We only show a warning instead of a hard error because not all changes will necessarily imply that a breaking change happened,, but the author will at least get warned of the consequences if they decide to proceed anyway, as explained next.

When a user updates their dependencies and the new version of such package contains changes in its decl tests, we also show the same warning to the user alongside a diff and ask for confirmation if they want to proceed with the update (ideally after ensuring that the diff doesn't show anything suspicious).

Example of a message shown to the package author:

$ zig pkg new-release minor

You're creating a new minor release of a stable (v1) package and we found the following problems:

--> src/foo.zig:50:5 The decl test for `createFoo` has changed. 
--- v1.1 src/foo.zig:20
+++ v1.2 src/bar.zig:50
@@ -1 +1 @@
-const foo = createFoo(bar);
+const foo = createFoo(bar.?);

--> src/bar.zig:25 The decl test for `Bar` has changed.
--- v1.1 src/bar.zig:80
+++ v1.2 src/bar.zig:105
@@ -1 +1 @@
-// Bar is used to create Foo.
+// Bar is used to create Foo, all Bar instances that you plan to use must be created before creating any Foo.

** WARNING **
If you decide to proceed with this release, all users who upgrade from an older release to this one will see this message!
Stable packages must respect the retrocompatibilty requirements as described by semver.

I also think that in light of this feature the signature check proposed in #404 would be a good addition. In the example above, createFoo originally accepted an optional. We see this change through the diff of its decl test, but it would be clearer to see a more explicit message about its signature:

--> src/foo.zig:50:5 The signature of `createFoo` has changed.
    Old: `createFoo(?*Bar)`, new: `createFoo(*Bar)`.

If we were to do that we would need to also come up with a set of rules for changes that we consider non-breaking. In this case (not accepting null pointers anymore) is a breach of retrocompatibility, but the inverse case (going from *Bar to ?*Bar), should not be, I think.

We should also consider rules for when creating a patch release: a patch release should not contain any new (public) declaration, while that's not a problem for a minor release.

Finally, I think the main point of this feature is to provide a bit of friction in the right places (instead of producing hard errors) to balance the fact that the tool can't determine with full confidence when a public API has changed or not. But, at the same time, I think that it would be a great idea to encourage the cultural norm of considering decl tests the authoritative place where the public API is described (and tested) past what Zig's type system can represent, and to have a tool that can bring to everyone's attention when something changed in a potentially dangerous way.

Imagine how this story could have played out with this system in place.

[nektro]

this is not enforceable without making Zig gather far more information than might be available.

[Snektron]

Would a change be detected by comparing the entire test decl or just the calls to the function in question?

[kristoff-it]

Would a change be detected by comparing the entire test decl or just the calls to the function in question?

@Snektron Everything, including comments (see the second part of the first example). The idea is that the decl test contains information about the decl that goes past what the type system can represent, so even comments become subject to this check.

this is not enforceable without making Zig gather far more information than might be available.

@nektro it would require either a versioning system, or information about the package index where the old version is hosted

[nektro]

right, if the code is not on an index, then the old version might not be available to check against

[nektro]

side note, but this proposal reminds me of https://www.youtube.com/watch?v=gCWtkvDQ2ZI

[daurnimator]

we would need to also come up with a set of rules for changes that we consider non-breaking. In this case (not accepting null pointers anymore) is a breach of retrocompatibility, but the inverse case (going from ?*Bar to *Bar), should not be, I think.

Indeed. https://xkcd.com/1172/

Such changes are visible e.g. imagine if a user introspects the signatures to generate bindings; and then it turns out their binding generator doesn't support optionals.

[kristoff-it]

we would need to also come up with a set of rules for changes that we consider non-breaking. In this case (not accepting null pointers anymore) is a breach of retrocompatibility, but the inverse case (going from ?*Bar to *Bar), should not be, I think.

Indeed. https://xkcd.com/1172/

Such changes are visible e.g. imagine if a user introspects the signatures to generate bindings; and then it turns out their binding generator doesn't support optionals.

There was a typo in the second example, the inverse situation would have been going from *Bar to ?*Bar. Fixed in the original comment.

[mrakh]

I think it’s important to avoid being prescriptive when it comes to interpreting semantic versions - as has been made apparent in this discussion, what constitutes a "breaking change" will be highly specific to how a particular project is expected to be consumed.

A classical example is ABI and API compatibility. Ideally, there would be separate tools to test each, and let project maintainers choose which they care about. Libraries distributed in binary form might care about ABI compatibility, but otherwise change API:

// Assuming target with 64-bit register size and return value stored in register
// Before
export fn foo(bar: u64, reserved: u64) void callconv(.C);
// After
export fn foo(bar: extern struct { low: u32, high: u32 }) u64 callconv(.C);

Or, if not breaking downstream builds is important but breaking ABI is okay, a project might care only about ensuring API compatibility:

// Before
fn foo(bar: extern struct { a: u8, }) void
// After
fn foo(bar: extern struct { b: u16, a: u8 }) void

And obviously, many will care about both, in which case both tests would be appropriate.