Skip to content
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

Rename eval_order_dependence to mixed_read_write_expression, move to nursery #8621

Merged
merged 3 commits into from
May 15, 2022

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Apr 2, 2022

As per the reference evaluation order is now defined.

I'm pretty sure rust always compiled with this evaluation order anyways so there's no reason the put an msrv limit on the lint.

changelog: Rename eval_order_dependence to mixed_read_write_expression, move to nursery

@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 2, 2022
@xFrednet
Copy link
Member

xFrednet commented Apr 2, 2022

I'm not sure if we should deprecate the lint just because the evaluation order has been defined. Looking at the example from the lint description, I would still expect this to be linted or later mark that as confusion in the code review.

// Example from the lint description
let a = {
    x = 1;
    1
} + x;

CC: @rust-lang/clippy

@llogiq
Copy link
Contributor

llogiq commented Apr 2, 2022

I also think deprecation is too far a step. It may be debatable if suspicious is still the right category or if it should be a restriction lint, though.

@Jarcho
Copy link
Contributor Author

Jarcho commented Apr 2, 2022

Two reasons for the deprecation.

  1. It should be renamed and moved into a different group at the very least. The current name is no longer accurate.
  2. It's current state is not good given the fact it's not catching any code which is wrong, only code of a questionable style. For catching style issues it lints on too many things, specifically using syn is broken.

If not deprecating it, I would say move it to the nursery with an issue open to deal with what should happen to it.

@xFrednet
Copy link
Member

xFrednet commented Apr 4, 2022

Given the background, I agree, it should at least be moved and renamed. Having a lint for the style from the example sounds good, but we can add that afterwards. 👍

@Jarcho
Copy link
Contributor Author

Jarcho commented Apr 4, 2022

Any nominations for a new name? If not I'll just update the PR to move it into the nursery with a note to deal with it later.

@flip1995
Copy link
Member

flip1995 commented Apr 5, 2022

unclear_eval_order / suspicious_eval_order?

I think restriction is a good category for this lint, even if not renamed right away. If someone really cares about eval order and enables this lint, some FPs shouldn't be too bad.

@bors
Copy link
Collaborator

bors commented Apr 6, 2022

☔ The latest upstream changes (presumably #8630) made this pull request unmergeable. Please resolve the merge conflicts.

@Jarcho
Copy link
Contributor Author

Jarcho commented Apr 7, 2022

Maybe mixed_read_write_in_expression. It's a little more direct as to what's linted. restriction sounds fine.

@xFrednet
Copy link
Member

I think this sounds like a plan. @Jarcho Could you rename the lint (I like mixed_read_write_in_expression) and move it to restriction? Then we can move forward with this 🙃

@Jarcho Jarcho force-pushed the eval_order_dependence_4637 branch from 2021198 to 2302909 Compare May 3, 2022 14:59
@Jarcho
Copy link
Contributor Author

Jarcho commented May 3, 2022

Had to add the renamed lints to the link list in the changelog. Do we want to have that link to the page for the new name? Currently I have it using the page for the old name.

It could be either

[`old_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#old_name

or

[`old_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_name

@flip1995
Copy link
Member

flip1995 commented May 4, 2022

Had to add the renamed lints to the link list in the changelog

You can just remove the [, ] around mentions of the old lint name.

@Jarcho
Copy link
Contributor Author

Jarcho commented May 4, 2022

Deprecated lints are already in the link list. This keeps renamed lints consistent with those. The links for deprecated lints aren't useful as they aren't in the docs. (They should be, but that's a different issue)

@xFrednet
Copy link
Member

xFrednet commented May 4, 2022

Adding renamed lints as an alias with a note to our lint list is still on my mind, somewhere at the bottom of my todo list 😅

@flip1995
Copy link
Member

flip1995 commented May 5, 2022

Deprecated lints are in the docs if you explicitly filter for them with the lint-group filter: https://rust-lang.github.io/rust-clippy/master/index.html#assign_ops (I consider it a bug that it doesn't show up with this link, because the filter excludes it. I think we have an issue open for this or at least talked about it before? 🤔)

Renamed lints can't be in the documentation (currently), because the old lint isn't defined anywhere anymore (except for its name).

@bors
Copy link
Collaborator

bors commented May 10, 2022

☔ The latest upstream changes (presumably #8796) made this pull request unmergeable. Please resolve the merge conflicts.

@llogiq llogiq changed the title Deprecate eval_order_dependence Rename eval_order_dependence to mixed_read_write_expression, move to nursery May 14, 2022
@llogiq
Copy link
Contributor

llogiq commented May 14, 2022

I was a bit discombobulated by the added links in the lint list, but I now understand. Sorry for taking so long to review.

r=me after rebase

@xFrednet
Copy link
Member

xFrednet commented May 14, 2022

@bors delegate=@Jarcho

For the r=me of @llogiq

@bors
Copy link
Collaborator

bors commented May 14, 2022

✌️ @Jarcho can now approve this pull request

@Jarcho Jarcho force-pushed the eval_order_dependence_4637 branch from f31250a to f7378da Compare May 15, 2022 21:10
@Jarcho
Copy link
Contributor Author

Jarcho commented May 15, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented May 15, 2022

📌 Commit f7378da has been approved by Jarcho

@bors
Copy link
Collaborator

bors commented May 15, 2022

⌛ Testing commit f7378da with merge 1c0a61e...

@bors
Copy link
Collaborator

bors commented May 15, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 1c0a61e to master...

@bors bors merged commit 1c0a61e into rust-lang:master May 15, 2022
@bors bors mentioned this pull request May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants