Skip to content

enhancement request - derive sorting #1867

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sagiegurari opened this issue Aug 9, 2017 · 10 comments
Closed

enhancement request - derive sorting #1867

sagiegurari opened this issue Aug 9, 2017 · 10 comments

Comments

@sagiegurari
Copy link
Contributor

maybe like you are sorting the 'use' lines and values, you could also sort the derive values, for example

#[derive(Serialize, Deserialize, Debug, Clone)]

can be sorted by abc order and keep the code consistent all over.

@nrc
Copy link
Member

nrc commented Aug 9, 2017

We can't do this because the order of derives can be semantically important (a derive only sees following derives, so in your example, Debug only sees Clone) and re-ordering could thus break some custom derives.

@nrc nrc closed this as completed Aug 9, 2017
@sagiegurari
Copy link
Contributor Author

sagiegurari commented Aug 10, 2017

Ok didnt know

@upsuper
Copy link

upsuper commented Jun 3, 2019

Although order of derives can be semantically important, I believe majority of custom derives don't care about other derives, and it could almost certainly be surprising if a custom derive is sensitive to the order.

So I think it is probably a good idea for rustfmt to support sorting derives (maybe as an opt-in for now), and discourage custom derive to rely on its position in derive list.

There are projects, e.g. Stylo, try to sort the derives. An example can be seen here. Without rustfmt's help, it needs to be maintained manually, which is error-prone.

@scampi
Copy link
Contributor

scampi commented Jun 5, 2019

I didn't know before now the order is important. If a derive is sensitive to order, then I imagine the confusion from the user in case of an error.
Even if the option is an opt-in, a user may enable it knowingly but forget about it later, and would be caught by it...

@upsuper
Copy link

upsuper commented Jun 5, 2019

Even if the option is an opt-in, a user may enable it knowingly but forget about it later, and would be caught by it...

As I mentioned, I think it should generally be considered a bad practice to have derive order sensitive.

Doing so would surprise users anyway, given how few people are aware of it, regardless of whether rustfmt would sort it.

@thomaseizinger
Copy link
Contributor

Even if there is a derive that is dependent on the order, it would be good practice by the maintainer to chose the name in a way so that the order is enforced by rustfmt :)

@kangalio
Copy link

This issue should be reopened. Many options could already theoretically break custom proc macros (e.g. all options that modify AST: 1, 2, 3, 4, 5, 6, 7...). That is fine because proc macros shouldn't depend on such details and deserve to break if they do. Same applies here.

Plus, this option would be opt-in anyways, so users would know what they're getting into when enabling this option.

It would be nice to have this option for serenity

@joao-conde
Copy link

Any updates on this? Think this is really nice to bring consistency to codebases (less cognitive load when reading code too).

@epilys
Copy link

epilys commented Aug 31, 2024

@joao-conde https://github.com/dcchut/cargo-derivefmt

@joao-conde
Copy link

@joao-conde https://github.com/dcchut/cargo-derivefmt

Seems to be a simple sorting tool only, which doens't address the problem of some derives that depend on others, where order matters, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants