Fix style check for Rust 1.88

[qinsoon]

Mostly https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args. Rust allows inlining identifiers for format args, such as println!("a = {a}"), clippy by default deney uninlined format args if they can be inlined (println!("a = {}", a)) is not allowed).

[qinsoon]

However, Rust does not seem to allow inlining format args that are expressions. E.g. in println!("{a} = {}", a + 1), a + 1 cannot be inlined, as it is an expression.

By default, allow-mixed-uninlined-format-args is true for clippy. So clippy thinks println!("{}", a) is a violation, but println!("{}, {}", a, a + 1) is not a violation, as a + 1 cannot be inlined.

I find this a bit inconsistent. We may want to set allow-mixed-uninlined-format-args to false. So clippy will enforce all the identifiers to be inlined, like println!("{a}, {}", a + 1).

@wks What do you think?

[k-sareen]

I feel like this lint is silly. It doesn't really help with readability specially if the string gets too long/there is a mix of the arguments. I'm inclined to say that we just disable uninlined_format_args globally.

[qinsoon]

I feel like this lint is silly. It doesn't really help with readability specially if the string gets too long/there is a mix of the arguments. I'm inclined to say that we just disable uninlined_format_args globally.

I don't quite like this lint either, but it does help in some cases.

Like, our current code has this (this is the required format by cargo fmt).

trace!(
    "Immix nursery object {} is being traced without moving",
    object
);

When the arg gets inlined, it becomes one line:

trace!("Immix nursery object {object} is being traced without moving");

[wks]

The uninlined_format_args lint has caused much controversy in the past. It happened in 1.67.0 (rust-lang/rust-clippy#10087), and Rust 1.67.1 downgraded it back to "pedantic" (https://releases.rs/docs/1.67.1/, rust-lang/rust-clippy#10265). And they are upgrading it again in 1.88, and someone has already reported it as an issue: rust-lang/rust-clippy#15151

In my opinion, inlining format arguments is not really more readable than not inlining them, and mixing inlined and un-inlined args makes it even less readable. As the author of rust-lang/rust-clippy#15151 said, the inlined syntax still doesn't support field access expressions, not to mention arithmetic expressions like a + b. In mmtk-core, we frequently use complex expressions in log messages and panic messages. I suggest disabling that lint completely.

[k-sareen]

Ah. The rust-analyzer reason is even more relevant. I didn't realize that use-case would break.

[qinsoon]

The uninlined_format_args lint has caused much controversy in the past. It happened in 1.67.0 (rust-lang/rust-clippy#10087), and Rust 1.67.1 downgraded it back to "pedantic" (https://releases.rs/docs/1.67.1/, rust-lang/rust-clippy#10265). And they are upgrading it again in 1.88, and someone has already reported it as an issue: rust-lang/rust-clippy#15151

In my opinion, inlining format arguments is not really more readable than not inlining them, and mixing inlined and un-inlined args makes it even less readable. As the author of rust-lang/rust-clippy#15151 said, the inlined syntax still doesn't support field access expressions, not to mention arithmetic expressions like a + b. In mmtk-core, we frequently use complex expressions in log messages and panic messages. I suggest disabling that lint completely.

I read through the issues you mentioned. There were actual issues with this lint, which was the reason why the lint got downgraded. I assume those issues were resolved at some point, and this lint was upgraded again as a default lint.

Our general philosophy is to stick with the community's standards, and only deviate from it when we have good reasons. The following is one example where we disabled a lint, as we have MMTK in the code and we don't want to write it as Mmtk

mmtk-core/src/lib.rs

Line 9 in efe7b41

#![allow(clippy::upper_case_acronyms)]

However, for this case, I don't see clear reasons yet why we should disable this lint.

inlining format arguments is not really more readable than not inlining them

Inlining the args does not make things worse though.

mixing inlined and un-inlined args makes it even less readable

Yes. But if we set allow-mixed-uninlined-format-args to false, all args that can be inlined will be required to be inlined. This could make things clearer.

[qinsoon]

Expressions as inlined format args seem to be explicitly not supported: https://rust-lang.github.io/rfcs/2795-format-args-implicit-identifiers.html#alternative-solution---interpolation

[wks]

Our general philosophy is to stick with the community's standards, and only deviate from it when we have good reasons.

The community is different from the Rust developers. From rust-lang/rust-clippy#15151, I am seeing push-backs from the community.

However, for this case, I don't see clear reasons yet why we should disable this lint.

In this case, the preference of style varies from people to people. For example,

-error!("LOS Object {} is not marked", object);
+error!("LOS Object {object} is not marked");

This is a simple case. It makes the expression terse.

- trace!(
-            "Reserved pages = {}, used pages: {}, collection reserve: {}, VM live pages: {}",
-            total,
-            used_pages,
-            collection_reserve,
-            vm_live_pages,
-        );
+trace!("Reserved pages = {total}, used pages: {used_pages}, collection reserve: {collection_reserve}, VM live pages: {vm_live_pages}");

This is a bit complex. We may argue that inlining the args makes the trace! macro more terse. But because the string literal is long, cargo fmt is no longer able to break this 136-chars-long line into multiple lines. And the long variable names like "collection_reserve" make it hard for human to get the overall structure of the message compared to the simple place-holder format "Reserved pages = {}, used pages: {}, collection reserve: {}, VM live pages: {}".

What could make things worse, is something like this:

            assert!(
                size >= bin_range.unwrap().0 && bin < bin_range.unwrap().1,
                "Assigning size={} to bin={} ({:?}) incorrect",
                size,
                bin,
                bin_range.unwrap()
            );

This is kept as is because it contains a method call expression. But think if the message only has the two args at one time, which is "Assigning size={size} to bin={bin}". Then when we refactor the code to add the third argument, we will have to manually un-inline the first two args to make all args un-inlined, or have mixed inlined and un-inlined args.

So I would prefer allowing us to have both styles, as long as they are both reasonably readable.

And we can wait for a few weeks to see how the Rust community react to this change. In other words, Let The Bullets Fly for a while before we make the decision.