cossacklabs / themis

Easy to use cryptographic framework for data protection: secure messaging with forward secrecy and secure data storage. Has unified APIs across 14 platforms.
https://www.cossacklabs.com/themis
Apache License 2.0
1.87k stars 143 forks source link

rust: Use f-string formatting syntax #984

Closed ilammy closed 1 year ago

ilammy commented 1 year ago

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

ilammy commented 1 year ago

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?

Lagovas commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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

Lagovas commented 1 year ago

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 commented 1 year ago

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.

ilammy commented 1 year ago

@vixentael:

update product docs to mention the min version

https://github.com/cossacklabs/product-docs/pull/300

ilammy commented 1 year ago

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 commented 1 year ago

🤦

ilammy commented 1 year ago

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