Skip to content
New issue

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

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

Already on GitHub? # to your account

rust: Use f-string formatting syntax #984

Merged
merged 5 commits into from
Feb 14, 2023

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Feb 6, 2023

Or "variable capture" as it's called around here. Since of Rust 1.67, our God-Emperor Clippy started demanding this form of formatting.

error: variables can be used directly in the `format!` string
    --> src/wrappers/themis/rust/libthemis-sys/build.rs:49:13
    |
 49 |             eprintln!("{}", error);
    |             ^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
    = note: `-D clippy::uninlined-format-args` implied by `-D warnings`
help: change this to
    |
 49 -             eprintln!("{}", error);
 49 +             eprintln!("{error}");
    |

Given that we're running WITH_FATAL_WARNINGS, we must either cave in, or suppress the lint. Suppressing it on Rust 1.56 requires suppressing another lint about unknown lint being suppressed.

So, given that we're unlikely to have users which require Rust 1.56 specifically, I think it's okay to cave in, update MSRV to 1.58, and update all affect format strings so that Clippy stays happy.

Checklist

  • Change is covered by automated tests
  • The coding guidelines are followed
  • Example projects and code samples are up-to-date (in case of API changes)
  • Changelog is updated

@ilammy ilammy added the W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates label Feb 6, 2023
@ilammy
Copy link
Collaborator Author

ilammy commented Feb 6, 2023

You okay with this ugly #[allow(...)]?

An alternative is to apply the changes demanded by Clippy and update the minimum required version from 1.56 to 1.58. Since a) the current stable is 1.67, b) RustThemis does not really have that many users that should not be intrusive in practice.

EDIT: You know... Let's do it then. Fewer lints – less awful code. Who am I trying to satisfy with Rust 1.56 compatibility?

Or "variable capture" as it's called around here. Since of Rust 1.67,
our God-Emperor Clippy started demanding this form of formatting.

    error: variables can be used directly in the `format!` string
      --> src/wrappers/themis/rust/libthemis-sys/build.rs:49:13
       |
    49 |             eprintln!("{}", error);
       |             ^^^^^^^^^^^^^^^^^^^^^^
       |
       = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
       = note: `-D clippy::uninlined-format-args` implied by `-D warnings`
    help: change this to
       |
    49 -             eprintln!("{}", error);
    49 +             eprintln!("{error}");
       |

Given that we're running WITH_FATAL_WARNINGS, we must either cave in,
or suppress the lint. Suppressing it on Rust 1.56 requires suppressing
another lint about unknown lint being suppressed.

So, given that we're unlikely to have users which require Rust 1.56
specifically, I think it's okay to cave in, update MSRV to 1.58, and
update all affect format strings so that Clippy stays happy.
@ilammy ilammy requested a review from Lagovas as a code owner February 6, 2023 13:43
@ilammy ilammy changed the title rust: Suppress clippy::uninlined-format-args lint rust: Use f-string formatting syntax Feb 6, 2023
@Lagovas
Copy link
Collaborator

Lagovas commented Feb 6, 2023

just to clarify, after that change all code written 1-2 years ago with rust <1.58 version wouldn't be compiled with the current version of themis?

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 6, 2023

just to clarify, after that change all code written 1-2 years ago with rust <1.58 version wouldn't be compiled with the current version of themis?

Correct. This will cause syntax errors with Rust 1.56 and earlier, breaking the builds.

I think this is acceptable as long as we do not backport this change to 0.14.x.

Applications that depend like this

[dependencies]
themis = "0.14"

will still work.

When they upgrade to "0.15" they will need a newer compiler (at least 1.58, the next stable version after 1.56).

@Lagovas
Copy link
Collaborator

Lagovas commented Feb 6, 2023

Okay, understood. Is it a common approach to update rust compiler version on every release or how often usually it happens in rust projects? I looked on firefox and they upgrade compiler's version after 2+ releases (usually after 2 new versions) and currently uses 1.65 for stable builds. Now 1.67 is the latest version. But FF is a product, not library. So I looked at several popular rust libraries and what I found:

  • tokio has been supporting versions published at least 6 months ago. And now, it's supporting 1.49
  • serde doesn't have any policy of rust version support and declares 1.13 in cargo.toml
  • toml declares 1.60 in cargo.toml
  • slog supports at least 15 last versions of rust, and now it is 1.31.

In our docs we have been published 1.31 version or later. I think we should choose a policy for ourselves how old rust compiler we want to support...
@vixentael , what do you think?

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 13, 2023

This becomes less of a problem with Rust 1.67.1, but only temporarily.

In 1.67.1 they have downgraded this lint to "pendatic" (ignored by default, unless explicitly enabled). However, this is only a temporary measure, until support for auto-fixing code to avoid this lint is there. They are still going to have it warn-by-default in the future, so this PR is still valid.

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 13, 2023

A common support policy that I've seen in Rust libraries:

  • Hard commitment to support N latest stable versions, where N is often 3 or 6 (which is about 3–6 months).

    That is, library must work with those versions without any breaking changes, period.

    Say, we take N = 3, then we must support at least 1.65.0 released on 2022-11-03.

    This is “hard” minimum version. We cannot use features added in later versions under any circumstances.

  • Minimum supported Rust version stays pinned at a particular version unless it is necessary to upgrade.

    This is “soft” minimum version. Currently on master, this is 1.56.

    We test against it, but it can be bumped if necessary. However, no frivolous bumps without a good reason (improved maintainability, security, whatever). It also cannot be bumped higher than the hard minimum, obviously.

  • No minimum supported version bumps in bugfix patch releases. Always release it with a minor version bump of the library, so that people can pin a minor version to stay on a particular old compiler until they are ready.

    We're in 0.x phase so approaches here vary, but usually this rule is relaxed and MSRV bump in 0.x.(y+1) is acceptable, since incrementing the “minor” version is like a major version bump in practice.

@Lagovas
Copy link
Collaborator

Lagovas commented Feb 13, 2023

However, no frivolous bumps without a good reason (improved maintainability, security, whatever).

Do we really need to bump the version now? Can we temporarily suppress such warnings for some time? until we decide to bump the version for some serious reason instead of syntax sugar?

@vixentael
Copy link
Contributor

vixentael commented Feb 13, 2023

I see no reason why we shouldn't use 1.58 as minimum instead of 1.56. The Rust compiler is always updating, the more versions we're behind, the more technical debt we have.

I'd bump to 1.58 min + update product docs to mention the min version.

Copy link
Collaborator

@G1gg1L3s G1gg1L3s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the previous orator. I'm not a fan of using the variable capture syntax everywhere (until it starts to support expressions; before that, mixing variable and expression looks awkward, IMHO), but everything for happy clippy!

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 14, 2023

@vixentael:

update product docs to mention the min version

cossacklabs/product-docs#300

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 14, 2023

Unit tests (1.58)

  Downloaded csv v1.2.0
error: package `csv v1.2.0` cannot be built because it requires rustc 1.60 or newer, while the currently active rustc version is 1.58.1

FFFFFFUUUUUUUU—

@G1gg1L3s
Copy link
Collaborator

🤦

Looks like the recently required 1.2 requires Rust 1.60 and that's too
young for us. Stay on the older version to keep 1.58 compatibility.

This is not strictly required, since a dev-dependency of utility,
but I'd rather run the whole test suite than mix and match depending
on Rust version.
Now that we require Rust 1.56+, we can finally put this in Cargo.toml
and do not get a warning which would be treated as an error.
@ilammy
Copy link
Collaborator Author

ilammy commented Feb 14, 2023

Aight. So I've pinned that one dev-dependency that didn't work with 1.58, now it's all green again ☀️

criterion, the benchmark framework, requires csv ^1.1 (i.e, >=1.1, <2). About 10 hours ago csv 1.2 got released and they require Rust 1.60+. Since themis is a library, Cargo.lock is not committed into the repo, so the build selects the latest versions that conform to the requirements. I added an extra requirement to restrict csv to 1.1.x

@ilammy ilammy merged commit d9a96e4 into cossacklabs:master Feb 14, 2023
@ilammy ilammy deleted the rust-f-strings branch February 14, 2023 12:58
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants