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

Kickstart the usage of let_chains #8360

Closed
wants to merge 2 commits into from

Conversation

c410-f3r
Copy link
Contributor

The time has come

@rust-highfive
Copy link

r? @giraffate

(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 Jan 27, 2022
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just though about doing this today 😄

This is great. With more of this, we should be able to get rid of if_chain! completely, right?

}
if attr.has_name(sym::cfg)
&& let Some(list) = attr.meta_item_list()
&& let mismatched = find_mismatched_target_os(&list)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, you can even put irrefutable patterns in let chains 👍

}
if let Some(parent) = get_parent_expr(self.cx, expr)
&& let ExprKind::MethodCall(_, [self_arg, ..], _) = &parent.kind
&&let caller = self.cx.typeck_results().expr_ty(self_arg)
Copy link
Member

@flip1995 flip1995 Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&&let caller = self.cx.typeck_results().expr_ty(self_arg)
&& let caller = self.cx.typeck_results().expr_ty(self_arg)

Formatting. Is rustfmt not able to format let chains yet?

This is the one major issue I have with if_chain! – code inside of it isn't automatically formatted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet -> rust-lang/rustfmt#5177

I guess it will be tackled eventually

@c410-f3r
Copy link
Contributor Author

I just though about doing this today smile

This is great. With more of this, we should be able to get rid of if_chain! completely, right?

Probably yes but also serves to battle-test the feature

@flip1995
Copy link
Member

also serves to battle-test the feature

There's probably no better place to test this than Clippy. It's ifs all the way down 😄

check_semver(cx, item.span(), lit);
}
}
if let Some(items) = &attr.meta_item_list() && let Some(ident) = attr.ident() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think && let should always wrap to the next line. Does rustfmt allow either way?

@camsteffen
Copy link
Contributor

I'm gonna miss our dear friend if_chain!. But this is really awesome! Congrats @c410-f3r for landing this feature in rustc!

@c410-f3r
Copy link
Contributor Author

Closing due to formatting concerns. If desired, feel free to use any content of this PR for posterior submissions

@c410-f3r c410-f3r closed this Jan 29, 2022
@camsteffen
Copy link
Contributor

I guess it makes sense to wait for rustfmt support? We already don't have formatting in if_chain!, although the lack of support is less surprising in a macro. Also formating by hand feels somewhat harder to get "right" with let-chains.

@xFrednet
Copy link
Member

xFrednet commented Jan 29, 2022

I also think we can accept PRs beforehand. As you said, if_chain!s also don't support formatting. In this case, we will even get formatting in the future in the worst case there will be one commit to align the formatting with rustfmt and that's it. 🙃

@camsteffen
Copy link
Contributor

I also think we can accept PRs beforehand.

I was mainly giving reasons to not migrate yet. But I don't have a strong opinion.

@xFrednet
Copy link
Member

Sorry, in that case I misunderstood you 😅. I thought it would be good to merge one PR like this one, to test the feature and to make sure that it work for us. Formatting is less important for a test. We can see if this causes any regressions in Nightly and there would still be time to fix it. However, I also don't have a strong opinion on this either xD

@flip1995
Copy link
Member

I guess we could merge a PR like this. But it doesn't give any benefit for Clippy until it can be automatically formatted. If we merge a PR like this, I would first limit it to clippy_utils and leave clippy_lints as-is for now.

@camsteffen
Copy link
Contributor

In any case, I got the ball rolling for rustfmt support! rust-lang/rustfmt#5203

bors added a commit that referenced this pull request Feb 1, 2022
Format `if_chain` invocations in clippy_utils

Not realizing it was [already obsolete](#8360), I built a [tool to format inside `if_chain` invocations](https://crates.io/crates/rustfmt_if_chain).

This PR applies the tool to clippy_utils. (If you apply it to clippy_lints, the changes are extensive.)

Anyway, I'm making it known here in case anyone wants to use it while `if-let` chain support is developed for `rustfmt`. (There could be a few Clippy PRs between now and then, and IMHO, the code looks better with the `if_chain` invocations formatted.)

Cheers.
bors added a commit that referenced this pull request Feb 1, 2022
Format `if_chain` invocations in clippy_utils

Not realizing it was [already obsolete](#8360), I built a [tool to format inside `if_chain` invocations](https://crates.io/crates/rustfmt_if_chain).

This PR applies the tool to clippy_utils. (If you apply it to clippy_lints, the changes are extensive.)

Anyway, I'm making it known here in case anyone wants to use it while `if-let` chain support is developed for `rustfmt`. (There could be a few Clippy PRs between now and then, and IMHO, the code looks better with the `if_chain` invocations formatted.)

Cheers.

---

changelog: none
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.

6 participants