inconsistent_struct_constructor doesn’t justify its existence

[wchargin]

The inconsistent_struct_constructor lint fires when a struct with
named fields is constructed with fields in a different order from the
order used in the struct definition. The docs say:

Why this is bad

Since the order of fields in a constructor doesn’t affect the resulted
instance […] inconsistent order can be confusing and decreases
readability and consistency.

But this doesn’t explain anything. It claims that “inconsistency
[…] decreases consistency”, which is trivially true; thus, the only
claim of substance is that it “decreases readability”. Does it really?

The whole point of using named fields rather than positional fields is
that we don’t have to care about the position. Each call site can choose
the order that is most natural: perhaps to align with local variable
definition order (which may be dataflow-dependent and not commutative),
or to put the most unusual/interesting field first to draw attention.
In no case is there reasonable opportunity for confusion, because the
fields are literally named.

Forcing authors to ensure that named fields are consistent with the
(arbitrary!) order at the struct definition is just make-work, and a
classic example of “foolish consistency” hobgoblinery.

This lint was introduced in #6769, which doesn’t discuss why it was
introduced. The linked issue, #6352, lists the sole motivating example:

struct SortedEdge {
    a: usize,
    b: usize,
}

// ...
SortedEdge { b, a } // Uh oh! We wanted: `SortedEdge { a: b, b: a }`

…but really the right fix here is just to make SortedEdge a tuple
struct, because the names a and b do not actually communicate any
information, whereas their positions do.

By contrast, this lint is false-positive firing on my code, which looks
like this:

pub struct Client {                
    token_store: Arc<TokenStore>,  
    http: HttpClient,              
}                                  

// ...
pub fn new(creds: Credentials) -> Result<Self, ClientError> {             
    let http = HttpClient::builder()                                      
        .user_agent(format!("tensorboard-data-server/{}", crate::VERSION))
        .build()                                                          
        .map_err(ClientError)?;                                           
    let token_store = Arc::new(TokenStore::new(creds));                   
    Ok(Self { http, token_store })                                        
}                                                                         

Here, we declare http before token_store because only that one can
fail, so if we’re going to fail we might as well do that first to avoid
wasted work. Then, initializing Self { http, token_store } seems
perfectly reasonable: if you want to make a consistency argument, it’s
generally held that local consistency trumps global consistency.

IMHO, this lint should clearly not be on by default. The clippy::style
category is described as “code that should be written in a more
idiomatic way” (emph. mine), which does not apply here. If we want to
keep this lint around, clippy::pedantic seems like the right fit,
since the lint is indeed “rather strict” and will inevitably have
false positives.

[Matthias247]

+1

I just stumbled on this. Changing code for this seems not worthwhile. Neither style nor correctness are improved on it.
However it will cause a lot of pain getting projects updated due to clippy now failing. The efforts to fix that are far better spent elsewhere.

[silverweed]

+1
I agree that this warning goes against the convenience of having the named fields instead of positional fields in the language and should definitely not be on by default.