Fix future clippy issues #14461
Fix future clippy issues #14461
Conversation
This commit fixes some issues the clippy on nightly has identified as issues in the code. These will all become CI failures at some future date (after we raise the MSRV to match the current rules nightly is adding). However, most of the changes are good improvements and in this case most are code style issues that make things a bit more concise. The bulk of the changes here are just moving to use captured identifiers in format strings.
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 15960219247Details
💛 - Coveralls |
There was a problem hiding this comment.
This looks straightforward and imho the new style is easier to read. I had one minor question (but no changes are required).
Are we planning to include this for 2.1? (Personally, I think it would be good to do so, but the PR is not marked for 2.1). In any case, I am approving it, and feel free to merge it in case this is for 2.1.
| std::mem::swap(self, &mut dag_builder.build()); | ||
| *self = dag_builder.build(); |
There was a problem hiding this comment.
This change makes sense. I am wondering though why this line had to change, but the line a few lines above it let mut dag_builder = replacement_dag.into_builder(); did not. A more global question, why do we even need to do the extending in a different replacement_dag DAG and not in the self DAG itself?
There was a problem hiding this comment.
This is a future clippy rule: https://rust-lang.github.io/rust-clippy/master/index.html#swap_with_temporary the builder line didn't change because it doesn't match the rule. But functional the semantics of .into_builder() are different, it creates a dag builder out of dag.
The dag builder is setup so that it avoids adding edges to the output node until the very end. For example if you have the dag A->B->C and want to add 2 nodes D and E between B and C. Instead of adding edges into C for D and then removing it when E is added the dag builder tracks the op nodes for each output in a vec and adds the final edges when .build() is called. It reduces the number of edge operations when building a dag up.
As for why there is a temporary dag created I think it's because the dag builder takes ownership of the dag to avoid any other mutation or reads while it's being built up. But the try_extend() method uses a mutable borrow so it can't call .into_builder() on self since the function doesn't own self. I didn't write this method but this is my guess from reading the code.
There was a problem hiding this comment.
Thanks for the detailed explanation!
I didn't feel it was critical or necessary for 2.1 I only opened this because I was trying nightly to test something locally and encountered the warnings for this and the changes were easy to make. |
There was a problem hiding this comment.
Can't comment on it since it isn't something you've changed but I found another one!
Line 881 in this file. Should change from:
return Self::from_label(&label).map_err(PyErr::from);to
return Self::from_label(&label);There was a problem hiding this comment.
This is actually a present clippy error; I see this warning on current stable. It's not on this file though but crates/quantum_info/src/pauli_linblad_map/qubit_sparse_pauli.rs which was added in #14283 (which merged after I opened this).
|
I've rebased the PR on main and updated it for the new places that violated the new rules. With the release of Rust 1.88 earlier today some of the rules I fixed in this PR are no longer "future" clippy rules as they've been stabilized and clippy will start flagging them in 1.88 now. |
|
Thanks Matthew. I am not sure what is the correct way to check for the new upcoming clippy warnings. The following command
|
|
I've just been running |
A new nightly rule was added to ensure that lifetime syntaxes are matched in function signatures. In many cases we were excluding the elided anonymous lifetime parameter being returned in the source which clippy asserts is potentially confusing. This commit adds the explicit anonymous paramter to these places to fix the warning.
a194ead to
bf7fa04
Compare
|
I've updated the PR with fixes for the new nightly rules. I'm not seeing any failures anymore. I will say this is a rolling thing and there is a reason we don't want to run nightly clippy regularly. So I'm not super concerned if this fixes every future rule. I only did this PR because I knew there were some big rules coming in 1.88 (mainly the string formatting one) and I was trying to get this merged before 1.88 released to minimize the disruption for those of us that build from the latest stable locally. That obviously didn't work though since this took > 1 month and rust 1.88 is released now with that rule included. |
Summary
This commit fixes some issues the clippy on nightly has identified as issues in the code. These will all become CI failures at some future date (after we raise the MSRV to match the current rules nightly is adding). However, most of the changes are good improvements and in this case most are code style issues that make things a bit more concise. The bulk of the changes here are just moving to use captured identifiers in format strings.
Details and comments