Add rust-version to Cargo.toml and pull request template

[youknowone]

fix #742

[dburgener]

If lalrpop is making a conscious commitment to support two previous releases, do you want to consider adding targets to the CI to test 1.67 and 1.68 (or maybe just 1.67), and update those periodically as new rust versions come out?

I think it's pretty hard to be on top of the timeline of new rust features enough to be confident that you're catching recently stabilized features in code review and not breaking N-2 toolchains.

[youknowone]

I feel like that's a soft rule. Better to keep without cost.

I am not opposite to the way you suggested, but unless there is a way to mark the version the-version-before-stable, it needs to be manually managed. This week was irregular, but I think this project was not that much active to take many patches. I'd like to avoid that kind of regular jobs for comparably less benefit.

On the other hand, even if we (unintentionally) break it at the moment, it will be automatically fixed in a few weeks.

[dburgener]

Fair enough. As a maintainer, you'd be the one bearing the cost of regular updates and the (potential) cost of user reports of breakage on N-2 compiler versions, so your call on the cost/benefit is reasonable.

[jannic]

I am not opposite to the way you suggested, but unless there is a way to mark the version the-version-before-stable, it needs to be manually managed.

You could just use 1.67 in CI and only bump that version in case some future PR actually needs newer features from a version between 1.67 and then-stable minus 2. That way you don't need to proactively change CI every time a new rust version is released.

[dburgener]

You could just use 1.67 in CI and only bump that version in case some future PR actually needs newer features from a version between 1.67 and then-stable minus 2. That way you don't need to proactively change CI every time a new rust version is released.

One aspect to that approach that could have positive and negative components to be considered is clippy.

Clippy adds new lints in every version. If lalrpop tracks stable, then when clippy publishes a new version, those new lints will apply, and could cause PRs to fail CI due to the new version of clippy. On the other hand, keeping our rust version stable prevents us from getting these new lints, which could improve lalrpop's code (and could cause some developer frustration if a developer uses a newer version of clippy and gets unexpected lints while building master).

On my repo (https://github.com/dburgener/cascade), what we do is build both a fixed rust version and stable. The fixed version is the only one that can fail CI on clippy failures. If stable reports clippy failures, instead of failing the CI, it posts a comment in the PR for the maintainers so that the PR can proceed, but maintainers see the new failures and can address them. When a new rust version comes out, once we are clean on new clippy lints we bump the known good rust version forward. It's a little more manual work, but it's nice to both track with the latest clippy lints and also be reproducible-ish for PRs.

[Pat-Lafon]

To throw in my two cents, since the ci uses dtolnay/rust-toolchain, I believe you can actually set this to have a rolling n-2 versions(I have not used this feature before). https://github.com/dtolnay/rust-toolchain#toolchain-expressions

In terms of a lower maintenance burden, I think this plus maybe not putting the msrv in the cargo.toml is the easiest. Then just a statement in the readme about the msrv policy should be enough together?

[youknowone]

Thank you for many advices. I learned a lot of things from your comments.

I realized I was not regarding about future users who wants to lookup working lalrpop version for their rust version.
Because we are already committing Cargo.lock, adding rust-version will be helpful to look it up.

I think adding both stable and marked rust-version to CI and adding a comment to let future contributors to change the versions easy when they need makes sense. How do you think about this one?

@Pat-Lafon This one is I never expected even exists. 👀 Thank you!

[youknowone]

@jannic @dburgener @Pat-Lafon could you review it again?

[Pat-Lafon]

One aspect to that approach that could have positive and negative components to be considered is clippy.

Clippy adds new lints in every version. If lalrpop tracks stable, then when clippy publishes a new version, those new lints will apply, and could cause PRs to fail CI due to the new version of clippy. On the other hand, keeping our rust version stable prevents us from getting these new lints, which could improve lalrpop's code (and could cause some developer frustration if a developer uses a newer version of clippy and gets unexpected lints while building master).

@dburgener Quick question, in your experience with clippy, did you include the msrv in one of the toml files? And how recently was this? I want to double check my understanding of https://github.com/rust-lang/rust-clippy#specifying-the-minimum-supported-rust-version which was improved in Rust 1.64 https://github.com/rust-lang/rust-clippy/blob/master/CHANGELOG.md#rust-164 to check the Cargo.toml file for the msrv.

It looks to me that with rust-version set, even running clippy on the stable release might not have any of the more recent lints fire? If I understand correctly, then more recently language feature lints will need to be explored manually.

[youknowone]

For CI, will --ignore-rust-version option be helpful?
https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field

[dburgener]

@Pat-Lafon Interesting. I didn't know about the clippy/rust-version interaction. I'll need to play with that some more.

I started pinning to specific rust toolchain versions for clippy with rust 1.65, because new lints introduced in 1.66 were breaking my CI and I wanted some time to address them. I had not set rust-version in my Cargo.toml at that point.

Playing with it locally a little now, I'm not seeing any effect onsetting rust-version at preventing future clippy lints. I put a quick instance of this lint https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_command_arg_space in a rust project, ran clippy, tried setting different values for rust-version, and got the lint consistently. If I instead install an older toolchain and do "rustup run 1.68 cargo clippy", then I don't get the new 1.69 lint.

My reading of https://github.com/rust-lang/rust-clippy#specifying-the-minimum-supported-rust-version sounds to me like it's specifically lints "pertaining to newer features". So I think it wouldn't suggest using a feature in a more recent rust version than your MSRV. That page does list a list of lints that respect the rust version, and it's a much smaller subset than the full set.

[dburgener]

For CI, will --ignore-rust-version option be helpful? https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field

I'm no expert, but I think not. My understanding is that this tells built in cargo commands to attempt to build anyways, even if they are an older version than the minimum rust version, which is a state that shouldn't occur in our CI, specifying stable, beta, nightly and possible a hardcoded version equal to rust-version.

In terms of clippy specifically, --ignore-rust-version doesn't appear supported by clippy. If I try it, clippy appears to try to pass it down to rustc, which complains that it doesn't recognize it. I think it would be really nice to get the behavior of specifying MSRV so that users trying to build with an older toolchain get a clear error message, but also have clippy suggest lints requiring newer versions to help us know to take advantage of new features, but if there's a way to get that behavior, I don't know what it is.

[Pat-Lafon]

Playing with it locally a little now, I'm not seeing any effect onsetting rust-version at preventing future clippy lints. I put a quick instance of this lint https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_command_arg_space in a rust project, ran clippy, tried setting different values for rust-version, and got the lint consistently. If I instead install an older toolchain and do "rustup run 1.68 cargo clippy", then I don't get the new 1.69 lint.

That makes sense to me, more so I'm thinking about new features like #[default] which came out in a specific, new(er) Rust version. These are the kinds of things we would want clippy to point out as being more idiomatic or a possible improvement.

My reading of https://github.com/rust-lang/rust-clippy#specifying-the-minimum-supported-rust-version sounds to me like it's specifically lints "pertaining to newer features". So I think it wouldn't suggest using a feature in a more recent rust version than your MSRV. That page does list a list of lints that respect the rust version, and it's a much smaller subset than the full set.

Yeah, I think I agree. It's like, I would want clippy to suggest a new feature via a lint which could raise the msrv. Your point is taken about the lint subset, but I'm particularly thinking about the percentage of new or useful lints going forwards which I think may be more likely to be in this subset. Purely speculation on my part.

[youknowone]

It seems we don't have a perfect choice to fulfill everything.

Maybe someone will try to update rust-version to reveal if there are more clippy errors they can fix 😄

[dburgener]

Sorry, I should have been more explicit last time. I think this one could do with less specificity as well (just 1.64).

[youknowone]

Thank you! I fixed here too

[Pat-Lafon]

For CI, will --ignore-rust-version option be helpful? https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field

I'm no expert, but I think not. My understanding is that this tells built in cargo commands to attempt to build anyways, even if they are an older version than the minimum rust version, which is a state that shouldn't occur in our CI, specifying stable, beta, nightly and possible a hardcoded version equal to rust-version.

In terms of clippy specifically, --ignore-rust-version doesn't appear supported by clippy. If I try it, clippy appears to try to pass it down to rustc, which complains that it doesn't recognize it. I think it would be really nice to get the behavior of specifying MSRV so that users trying to build with an older toolchain get a clear error message, but also have clippy suggest lints requiring newer versions to help us know to take advantage of new features, but if there's a way to get that behavior, I don't know what it is.

Maybe relevant rust-lang/rust-clippy#10709 (reply in thread)