Move inconsistent_struct_constructor to pedantic

[wchargin]

The whole point of named fields is that we don't have to worry about
order. The names, not the position, communicate the information, so
worrying about consistency for consistency's sake is pedantic to a T.

Closes #7192.

changelog: [inconsistent_struct_constructor] is moved to pedantic.

wchargin-branch: inconsistent-struct-constructor-pedantic

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @camsteffen (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[matthiaskrgr]

Well I think the thing is that because of this inconsistency (which is probably ok in most cases) you MIGHT write code that does not work the way you think it does and that can cause bugs.
We don't say that every finding is a bug (that would be deny severity anyway) but I think moving the lint to pedantic kind of defeats its purpose.

[camsteffen]

This seems pedantic to me. I'll wait for other opinions.

[Y-Nak]

I agree with @matthiaskrgr.

One of the motivations for introducing this lint was to indirectly address FN of if_same_then_else in addition to finding bugs. As #6352 says, the problem was due to the apparent difference of the code, and to solve it directly, we need to normalize a struct constructor in some way anyway. I think this problem of the apparent difference also applies to humans, that is, we need to normalize it in our head to determine whether instances are identical or not. This places an unnecessary burden on readers of the code especially when there are many fields.

Also, the applicability of this lint is MachineApplicable, so I think this doesn't require so much work to fix it.

[camsteffen]

Those are not good motives for re-categorizing a lint IMO. If another lint has a false positive, then that should be fixed.

My reasoning for thinking it should be pedantic is that the significance of the order of fields in a struct varies widely between structs. Considering structs where the order is meaningless, it is pedantic to tell people to reorder the fields to match the definition.

[Y-Nak]

What I meant was

I think this problem of the apparent difference also applies to humans, that is, we need to normalize it in our head to determine whether instances are identical or not. This places an unnecessary burden on readers of the code especially when there are many fields.

Not to fix another lint's FN.

[elomatreb]

I also think this is too pedantic for general applicability, I always liked to group my initializers with "simple" fields that use the syntax shorthand first and more "complicated" fields that use expressions or variables with other names last, to make it more obvious which variables are trivially reused.

[flip1995]

I always liked to group my initializers with "simple" fields that use the syntax shorthand first and more "complicated" fields that use expressions or variables with other names last

This lint only triggers if only the shorthands are present in the constructor (at least that is what is documented).

---

But I'm also leaning towards pedantic here. I think the only case where this may cause problems in real code is when the struct field names are almost identical / closely related. For example in a struct where you only have single character fields warning-by-default is probably warranted. But if you have fields like in #7192 with names like "http" and "token", there is not much room for error, if you swap the order.

Maybe this lint should also consider the types of the fields, but I'm not sure if that would actually help in real world code.

In its current state, aggressively linting all structs, this lint should be in pedantic though.

[camsteffen]

This lint only triggers if only the shorthands are present in the constructor

That is a little surprising to me. The lint could include cases without shorthand where there are no side-effects. But in any case I don't think that has any bearing on whether it should be pedantic.

[flip1995]

That is a little surprising to me. The lint could include cases without shorthand where there are no side-effects. But in any case I don't think that has any bearing on whether it should be pedantic.

I haven't looked at the code, but that is what the documentation says. And yeah, I agree this has little influence on the decision if it should be pedantic.

[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.

[camsteffen]

@bors r+

Between comments here and on the issue, many people believe that that out of order fields is not bad style, at least not enough to warn by default.

[bors]

📌 Commit 1b2ca30 has been approved by camsteffen

[bors]

⌛ Testing commit 1b2ca30 with merge 0d4e24e...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 0d4e24e to master...