Add a command-line flag to override the major/minor/patch release detection logic

[obi1kenobi]

Here's an idea: Add a command-line flag to say "assume that the next release is a minor release". Then we can just pass that in CI rather than messing around with the Cargo.toml.

We don't need the patch-release checks, and if we ever want to make a major release of anything again, we can turn off the semver check for that crate temporarily.

Originally posted by @Darksonn in tokio-rs/tokio#5437 (comment)

Possible workaround for users until we can come up with a more full-fledged solution for #58

[obi1kenobi]

Would probably take the form of an option, like --some-option-name minor or --some-option-name major.

Let's bikeshed some names for --some-option-name. Proposals so far:

• --assume-release
• --assume-change
• --assume-version
• --release-type
• --lint-level

Any others? Any strong feelings toward or away any of these?

@tonowak @epage would love your thoughts in particular. This addition would make cargo-semver-checks easier to use in tokio, and probably in other crates that don't want #[must_use] lints before #58 is implemented as well.

[Finomnis]

Although my opinion probably doesn't count much :)

I like --lint-level, although it kind of suggests that it only enables lints of that level and above instead of forcing the change version increment type to that level. Maybe --force-lint-level?

That's my 2 cents :D feel free to ignore

[epage]

This gets a bit into #58 territory. There are two angles to lint levels, compatibility level and reporting level. This is about the compatibility level.

I lean towards something like do --compat <level> or --semver <level>.

An interesting angle to this is "major", "minor", and "patch" will likely be relative (0.x.y has "x" as major in relative terms and minor in absolute terms). cargo release (not on the table for merging into cargo) and cargo set-version (possible but no one has requested it yet) both treat the terms as absolute as you need to be able to bump from "0.x" to "y.0" though relative terms would also be useful. I've not yet figured out how to solve the naming discrepancy between relative and absolute.

[epage]

Correction, there are three angles to all of this

• the compat level of a lint
• the reporting level of a lint
• the compat level of the project

We need to be able to change the compat level of a lint (#58) and declare the compat level of the project (this project). We should decide on names in tandem between this and #58 to make sure they are aligned.

[obi1kenobi]

Although my opinion probably doesn't count much :)

I like --lint-level, although it kind of suggests that it only enables lints of that level and above instead of forcing the change version increment type to that level. Maybe --force-lint-level?

We appreciate your opinion!

I had the same thought process with --lint-level, it implies >= instead of > which seems problematic. And if we're getting into 3-word territory then I think I like --assume-release-type better than --force-lint-level.

There are two angles to lint levels, compatibility level and reporting level.

Would an example of reporting level be "for our crate, MSRV changes are patch, regardless of what cargo-semver-checks thinks"?

What's an example of the compat level of the project?

[epage]

rustc has the concept of "cap lints", meaning you can limit what lint levels get reported. This is currently used when building dependencies to prevent their warnings from being shown. Using similar terminology might help? So --cap-semver?

[Finomnis]

I do like --assume-release-type.

[epage]

Would an example of reporting level be "for our crate, MSRV changes are patch, regardless of what cargo-semver-checks thinks"?

In #58 I talked about compat level (major, minor, patch) in the same breath as reporting level (forbid, deny, warn, allow) but they are separate concepts in discussion, regardless of how we present them in UI

[obi1kenobi]

Re: --cap-semver, I'm worried that the "mental image" it gives is wrong, sort of like with --lint-level. "Cap" to me sounds like "don't report above this level" not "only report above this level" -- like capping a bottle of water. --cup-semver would be a funny joke but more visually appropriate :)

I lean towards something like do --compat <level> or --semver <level>.

Would you be opposed to --assume-semver <level>? The bare --semver seems not immediately obvious, and the "assume" really does the trick for me for some reason.

An interesting angle to this is "major", "minor", and "patch" will likely be relative (0.x.y has "x" as major in relative terms and minor in absolute terms). cargo release (not on the table for merging into cargo) and cargo set-version (possible but no one has requested it yet) both treat the terms as absolute as you need to be able to bump from "0.x" to "y.0" though relative terms would also be useful. I've not yet figured out how to solve the naming discrepancy between relative and absolute.

Yes, I think the semver levels have to be relative here, but I agree that it makes sense for cargo release and cargo set-version to use absolute naming instead, with an option to switch to relative. No ideas on the naming there, naming is hard!

[Finomnis]

@obi1kenobi Here a first draft. Review appreciated.

Didn't add a test yet; I was a little confused by how you guys lay out your tests or whether you do unit tests at all.

[obi1kenobi]

For CLI features so far we've been using CI jobs over real-world crates. I can add that in a separate PR if you'd prefer, it's not hard for me to do.

[epage]

There is something about --assume-semver that isn't clicking with me. Really, what we are doing is saying is "override the inferred (or assumed) semver level". In a lot of cases when overriding an explicit value, you make the flag name corrspond to what you are overriding. So I still see --semver making sense but could also seem something like something like "check this semver is compatible".

That spelled out idea is making me think this is a jumbled ball as it ties into #86

• what ideas are conveyed in the command vs needing to be explicit
• Should we be telling the checks what the state of the project is (--semver patch reports everything above patch) or what we want checked. Neither feels quite right for me.

[tonowak]

For CLI features so far we've been using CI jobs over real-world crates. I can add that in a separate PR if you'd prefer, it's not hard for me to do.

Alternatively, an integration test in the tests/ directory can run the CLI just like an user would run. Here's an example: https://github.com/obi1kenobi/cargo-semver-checks/blob/main/tests/verify_binary_contains_lints.rs

[tonowak]

I think it's much harder to understand the meaning of a flag checking > semver issues, compared to >= semver issues. My first thought behind the meaning of --lint-level flag was indeed "those issues and more serious issues are checked" and not "only more serious issues than the specified type are checked". That's how error levels are defined and defining it otherwise might be really misleading.

Should we be telling the checks what the state of the project is (--semver patch reports everything above patch) or what we want checked. Neither feels quite right for me.

If I was a first-time user of the tool, I would try to first run the tool and then limit the output to only those lints I care about -- I would prefer to specify what needs to be checked and experiment with it, and not specifying what the state of the project is (especially that "state of the project" is clear in the sense of PRs, but not necessarily for people experimenting with the CLI in their project).

Also, >= is less restrictive than >, so if someone would wrongly understand what the flag does, one would just get more lints than wanted and would probably notice it, which is not the case with >, where some lints would not run.

The meaning of the --assume-semver flag for me is not clear at all.

[obi1kenobi]

That's pretty compelling. I have changed my mind, I support --lint-level with >= semantics.

@epage how do you feel about --lint-level? Would you rather we set aside some time soon and hash out #86 more thoroughly, or is going forward with --lint-level worth it for now?

[Finomnis]

Reading those comments I think it's important to highlight it again: You are talking about two different mechanisms here.

• filtering: Meaning, hiding checks that are irrelevant for the needs of whoever calls it, additional to hiding the checks that are unnecessary to how the two versions relate to each other
• overriding: Ignoring the given versions and assuming that they are a certain type to each other, for example minor.

Which one do we want to implement now?

Or both, one being --assume-semver and the other one being --lint-level?

Just so I can still follow.

[Darksonn]

Here's my mental model. There are two level of lints:

1. Patch-lints. Lints that are only relevant to patch releases.
2. Minor-lints. Lints that are relevant to both minor and patch releases.

And then, depending on the kind of release you're making, you either get both lint types, or only the minor lints. Both --release-type and --lint-level make sense to me as an option for configuring which set of lints you get.

I don't really understand the idea behind the --assume-semver name.

[Finomnis]

@Darksonn So the question is, if the new version number in Cargo.toml is a minor increment, and we set --lint-level to minor, What lints should run?

• Only major checks, because that's all that's required if our version already got incremented by minor?
• major and minor checks, because the --lint-level is set to minor?

[obi1kenobi]

FWIW, to me it seems like it would be the latter: --lint-level X includes level X and above. --lint-level major means major-only. To disable even major lints, just don't run the tool because there's nothing it can run then.

[Finomnis]

@obi1kenobi, @Darksonn, @tonowak, @epage: Next review request, now with --lint-level :)

Is that what you had in mind?

[Finomnis]

@obi1kenobi I now implemented it so that two things have to be given for a lint to run:

• The version of the Cargo.toml and the currently published version must make the lint relevant; if the version in Cargo.toml is a major version change, nothing should run because everything is allowed
• The --lint-level flag must not hide the lint; if --lint-level is major, checks for minor lints are hidden

[Finomnis]

I think a lot of confusion in this entire discussion comes from the fact that there is a brainf*ck mismatch between version increments and filter levels.

A minor version change means that only major lints have to run, and if we do >= filtering with --lint-level (which I am strongly in favor of), that means that a minor version change corresponds to --lint-level major. Which does make sense, but it
takes a while to realize why, in my opinion.

[epage]

For me, the problem is there are two different kinds of lint levels, the semver level and the reporting level. Granted, rustc's version of "lint level" for reporting has a different name (cap lint) so we would unlikely use --lint-level for reporting. The only question is how much we confuse users over this.

I will say, runtime experience is one way to learn so we can go forward with a name but we should call out re-evaluating it in the stablization / cargo merge Issue.

[epage]

(also I disagree with the "override" framing as instead the flag is saying what is happening but the tool is inferring a default, same effect different mental model).

[Finomnis]

@epage No override is happening. It's an additional filtering. There is no override framing any more, that was in the previous change that was rejected for that reason.

As there still seems to be a lot of confusion, let me propose a new solution:

Reasoning

There are only two lint levels:

• major (which check if a major version increment is required)
• minor (which check if a minor version increment is required)

So having a --lint-level [major/minor] would simply mean:

• --lint-level minor: run all lints
• --lint-level major: skip minor lints

Proposal

As this is quite unintuitive, why not introduce a flag --skip-minor-lints?

Without the flag, it simply runs all lints, and with the flag, it skips minor lints.

Skipping major lints makes no sense, because this is equal to simply not running the tool.

Skipping patch lints makes no sense, because there are no patch lints. (Every version increment is at least a patch increment, so there's not point in checking whether a patch increment is required)

So from a filter point of view, enabling or disabling minor lints is really all the lint level sensitivity filtering that can every be performed. And the name --skip-minor-lints would be trivial to understand for everyone.

Thoughts, @obi1kenobi?

[epage]

@Finomnis regardless of the name, I've always seen this as --lint-level being defaulted by the version change, not that they are separate checks. Is there a part of the discussion I glossed over where that changed?

[Finomnis]

@epage I think different people are talking about different things here.

I'm pretty sure @Darksonn is talking about filtering/separate check:

Here's my mental model. There are two level of lints:

1. Patch-lints. Lints that are only relevant to patch releases.
2. Minor-lints. Lints that are relevant to both minor and patch releases.

And then, depending on the kind of release you're making, you either get both lint types, or only the minor lints. Both --release-type and --lint-level make sense to me as an option for configuring which set of lints you get.

I don't really understand the idea behind the --assume-semver name.

Apart of the fact that she's calling it patch and minor lints, based on what what version increments they are relevant for, and the code calls them minor and major lints, based on what version increment those changes would require.

Also, @tonowak said:

If I was a first-time user of the tool, I would try to first run the tool and then limit the output to only those lints I care about -- I would prefer to specify what needs to be checked and experiment with it, and not specifying what the state of the project is (especially that "state of the project" is clear in the sense of PRs, but not necessarily for people experimenting with the CLI in their project).

And "limit the output to only those lints I care about" to me sounded a lot like filtering, after which you replied that he convinced you. He didn't write "overwrite lints to the ones I care about".

I assumed you were talking about the same thing, I think I also misread this message of yours:

There is something about --assume-semver that isn't clicking with me. Really, what we are doing is saying is "override the inferred (or assumed) semver level".

I think I read that as "you don't want an override".

It might make sense to have an actual override, but after talking about it, I think what we really need in tokio is a --skip-minor-lints type flag. (correct me if I'm wrong, @Darksonn) This flag would then only run checks that would require a major version increment, or as @Darksonn puts it, that are relevant if you want to publish a new minor version.

[Finomnis]

New attempt to implement what we need in tokio. Feedback appreciated.

@Darksonn From the perspective of tokio, this would now mean we can run it all the time, and it will only check whether or not we would require a major new version for a given PR.

Once we do want to publish a new major version, we can simply increment it in tokio to new.major.version-alpha.0 and the linter would be satisfied again. No further CI reconfiguration required.

But as long as we don't, the linter would always assume that the next version we publish is at least a minor version increment. It shouldn't trigger on #[must_use] and will trigger directly in the offending PR. I think that's all the requirements you mentioned so far.

[Finomnis]

@epage I just read through #58 and your point of view makes a little more sense to me now. With that, let me talk about this post of yours again, because I think it's very relevant to which direction to go into:

Correction, there are three angles to all of this

• the compat level of a lint
• the reporting level of a lint
• the compat level of the project

We need to be able to change the compat level of a lint (#58) and declare the compat level of the project (this project). We should decide on names in tandem between this and #58 to make sure they are aligned.

I'm not 100% sure what you mean with that.

The "compat level of a lint" makes sense to me, this is what type of version increment is required if the lint fails.

The "compat level of the project" kind of confuses me, why would a project be of a specific compat level? Shouldn't it depend on the specific PR, and which version the project wants to publish next?

I don't completely understand the "reporting level of a lint". Is this at what level to report, but without making the check fail?

---

The level that I'm interested in is the "filter level of the project", meaning, downgrade lints that are less than a specific lint level to "informal", meaning they don't break the CI check. Although I agree that this might be a very specific usecase and might not be generic enough to fit into your model.

I understand that you have more complex plans with lint levels, so I am open to what you suggest. I'm confused, to be honest, about all the different types of levels we are talking about now.

[Finomnis]

I apologize if I am too opinionated here. I'll take a step back and let you guys talk it out. Make of my pull requests what you want :)

[epage]

The "compat level of the project" kind of confuses me, why would a project be of a specific compat level? Shouldn't it depend on the specific PR, and which version the project wants to publish next?

I'm meaning "the current state of the project" or "what compat level someone is interested in"

I don't completely understand the "reporting level of a lint". Is this at what level to report, but without making the check fail?

rustc lints can be allow, warn, deny, and forbid. This is a separate axis from the compat level. You might have compat checks that you only want to to warn and not deny as whether they require a version bump requires enough context that the caller should decide. For example, I would want -Ddeprecation but -Wadd for clap as I require all deprecations to be in a minor release but I don't strictly enforce that new API items require a minor release.

[epage]

Also, I thought had said this but can't find it: I'm talking there are these different levels in principle but that doesn't dictate how we expose them or that we might comingle them. For example, maybe there are auto-lint groups based on the semver level of the lint so you can do -Wminor to say all lints set to "minor" level are only warnings and not denies (errors)

[Darksonn]

Maybe the discussion shouldn't be about the lints in the first place? You have a feature that detects what kind of release we're going to make next (patch, minor, major) by looking at the toml, and the flag overrides what release type that logic detects.

The release type affects which lints trigger and which don't, but fundamentally what I wanted to override is the release type detection.

[Finomnis]

The release type affects which lints trigger and which don't, but fundamentally what I wanted to override is the release type detection.

OK by now I think I completely misunderstood what the goals were :D so my first PR was what we needed, except that the name confused everyone?

[obi1kenobi]

This is a tricky issue. Thank you @Finomnis and @Darksonn for thinking through it with @epage and me — I think our solution will be that much better for it.

I want to summarize my understanding so far, make a concrete proposal to move us forward, and offer justification for it with my opinions.

Summary

I believe there are three separate-but-interconnected things in play:

1. when a check should run (minor / major / always) — proposed term "compat level"
2. if it finds anything, what to do about it (deny / warn / etc.) — proposed term "reporting level"
3. what kind of version change the tool should believe it's look at, for the purpose of 1. — proposed term "version detection"

There's overlap between 1. and 2., in that setting a check to a hypothetical "patch" compat level (if one existed) would be equivalent to setting it to "allow" reporting level. This is why there's no "patch" compat level — it's redundant.

It's in principle possible to express 3. as just a more concise way to set a group of 1. and/or 2. settings. For example: --release-type major is equivalent to "set all minor lints to allow reporting level." If a "patch" compat level had existed, setting all minor lints to that would have been equivalent as well — as mentioned above.

(Aside: here's a plausible example of a compat-always, reporting-warn lint, which is not something we have today: "added a new exhaustive pub enum, are you sure you don't want to make it #[non_exhaustive]?")

Opinion / Justification

I think we want all three of these.

It's reasonable for a project to change the compat level: "for us, MSRV bumps are minor-deny", if the default had for example been set to "major-allow". (Setting completely aside whether this is a good default or not.)

It's also reasonable for a project to change the reporting level: "API additions are minor-deny" changing from a presumed default of "minor-allow".

It's also reasonable for either a project or for a human using the CLI to want to set a group of settings more quickly than typing in a bunch of individual overrides, even if there are lint groups that allow setting compat/reporting across a group like -Wadd. While the tokio use case would use this flag in CI, I think it's very likely that a human at a keyboard would use this much more often. cargo has similar "convenience CLI flags," for example --all-features and --exclude which are both technically just re-expressions of what the other flags can do.

Proposal

• --release-type patch/minor/major for version detection, purely as an convenience CLI flag
• -W/-A/-D/-F + their corresponding long forms for reporting levels — the same as rustc/clippy
• TBD flags for compat level (punt to Lint level control #58)

In terms of merging into cargo:

• The reporting level flags are a clear fit.
• The version detection is "syntax sugar" so regardless of whether cargo adopts it or goes for something different, it won't be a loss of functionality in the merge — in the absolute worst case it would be a loss of convenience.

Thoughts?

[epage]

@obi1kenobi one nit pick on my side:

what kind of version change the tool should believe it's look at, for the purpose of 1. — proposed term "version detection"

For me, I feel like the "version detection" framing focuses on the wrong part, on the mechanism for how we get the default value (that we are now wanting to explicitly set) rather than how its used.

With that said, my earlier comment stands that I am mostly fine with us merging whatever to collect real-world feedback and marking this as an open question for the cargo merge. I appreciate that we've talked this through and I think its productive all around for understanding use cases, designs, etc but as we still have the power to change things, we should time-box the conversation and move on.

[obi1kenobi]

For me, I feel like the "version detection" framing focuses on the wrong part, on the mechanism for how we get the default value (that we are now wanting to explicitly set) rather than how its used.

I understand. Perhaps we can find a better name for it.

In my head it continues to be just a shorthand for setting a bunch of compat and reporting levels at once, with explicit overrides of either of those dominating. The underlying implementation isn't that important to me.

Let's move forward! I'll merge the --release-type PR.

[Finomnis]

I like --release-type. Makes sense to you as well, @Darksonn?

[Darksonn]

The name --release-type was my favorite from the beginning, so I'm happy.

[Finomnis]

Re-opened and updated #361. Review required :)

[obi1kenobi]

Awesome, merging! Thanks again for both the patience and the contributions!

Tentatively planning to release on Monday morning US/Eastern time, so afternoon in Europe.

[Finomnis]

Thanks everyone for the heated but fruitful discussion :)

[obi1kenobi]

Passionate collaborative problem-solving!