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.

Sign up for GitHub

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

Jump to bottom

redundant_slicing precedence fix #7976

Closed
wants to merge 1 commit into from
Closed

redundant_slicing precedence fix #7976

wants to merge 1 commit into from

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Nov 14, 2021

fixes: #7972
changelog: Fix redundant_slicing suggestion when the result is used in a postfix expression

@rust-highfive
Copy link

r? @xFrednet

(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 Nov 14, 2021
@Jarcho Jarcho force-pushed the redundant_slicing_method branch from 1b67c0b to 1571cc7 Compare November 14, 2021 14:25
xFrednet
xFrednet reviewed Nov 19, 2021
View reviewed changes
clippy_utils/src/source.rs
/// The `SyntaxContext` of the expression will be walked up to the given target context (usually
/// from the parent expression) before extracting a snippet. This allows getting the call to a macro
/// rather than the expression from expanding the macro. e.g. In the expression `&vec![]` taking a
/// snippet of the chile of the borrow expression will get a snippet of what `vec![]` expands in to.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// snippet of the chile of the borrow expression will get a snippet of what `vec![]` expands in to.
/// snippet of the child of the borrow expression will get a snippet of what `vec![]` expands in to.

Did you mean child here? Otherwise, I'm not sure what chile is referring to :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I swear I know how to type.

clippy_utils/src/source.rs

// Attempt to determine if parenthesis are needed base on the target position. The snippet may have
// parenthesis already, so attempt to find those.
// TODO: Remove parenthesis if they aren't needed at the target position.
Copy link
Member

Choose a reason for hiding this comment

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

We can create an issue for this enhancement, once the PR is merged 🙃

clippy_utils/src/source.rs
Comment on lines +403 to +408
ExprKind::Cast(lhs, rhs)
if (ctxt == lhs.span.ctxt() && expr.span.lo() != lhs.span.lo())
|| (ctxt == rhs.span.ctxt() && expr.span.hi() != rhs.span.hi()) =>
{
false
},
Copy link
Member

Choose a reason for hiding this comment

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

NIT: This can be combined with the first case (ExprKind::Binary(_, lhs, rhs) ...) as the guards and bindings are the identical :)

clippy_lints/src/redundant_slicing.rs
app,
);
}

Copy link
Member

@xFrednet xFrednet Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change

Really, nit picky, but I thought I mention it since, there are some other adjustments/comments :)

clippy_utils/src/source.rs
/// `MaybeIncorrect` to `HasPlaceholders`.
/// * If the snippet is taken from a macro expansion then it will be changed from
/// `MachineApplicable` to `MaybeIncorrect`.
pub fn snippet_expr(
Copy link
Member

@xFrednet xFrednet Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The tests don't have any instance where the parenthesis are added by this function. Can you please add some test cases to ensure that this works correctly.

@xFrednet xFrednet added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 19, 2021
@Jarcho
Copy link
Contributor Author

Jarcho commented Nov 19, 2021

Just btw I'm working on #7986 which is an alternative to the extra util functions.

@xFrednet
Copy link
Member

Hey @Jarcho, are you planning to continue working on this PR or are you waiting on #7986 before doing more work here? 🙃

@Jarcho
Copy link
Contributor Author

Jarcho commented Nov 25, 2021

I'll give it a fix for the specific issue as that doesn't need anything added to utils.

@Jarcho Jarcho mentioned this pull request Jan 4, 2022
Merged
@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 5, 2022

Closing in favour of #8218.

@Jarcho Jarcho closed this Jan 5, 2022
bors added a commit that referenced this pull request Feb 17, 2022
@bors
Auto merge of #8218 - Jarcho:redundant_slicing_deref, r=camsteffen
668b3e4 
Improve `redundant_slicing` lint

fixes #7972
fixes #7257

This can supersede #7976

changelog: Fix suggestion for `redundant_slicing` when re-borrowing for a method call
changelog: New lint `deref_as_slicing`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camsteffen camsteffen camsteffen left review comments

@xFrednet xFrednet xFrednet left review comments

Assignees

@xFrednet xFrednet

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
4 participants
@Jarcho @rust-highfive @xFrednet @camsteffen