fix: incompatibility of non_canonical_clone_impl and implicit_return

[Kokoro2336]

View all comments

changelog: fix [non_canonical_clone_impl] incompatibility with [implicit_return]

Fixes #16945

Note

Concerns (0 active)

• Using return *self is not the canonical implementation of Clone resolved in this comment

Managed by @rustbot—see help for details.

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, dswij, llogiq

[Kokoro2336]

@rustbot ready

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[samueltardieu]

I don't think non_canonical_clone_impl should stop linting unless implicit_return is also active at the point of the check.

[Kokoro2336]

I don't think non_canonical_clone_impl should stop linting unless implicit_return is also active at the point of the check.

@samueltardieu But, you know, checking whether there's an explicit return is not the job of non_canonical_clone_impl. The existence of return should not affect the detection of non_canonical_clone_impl. And adding a special check of whether implicit_return is enabled would be dirtly.

[samueltardieu]

@samueltardieu But, you know, checking whether there's an explicit return is not the job of non_canonical_clone_impl. The existence of return should not affect the detection of non_canonical_clone_impl. And adding a special check of whether implicit_return is enabled would be dirtly.

Agreed. But adding a return makes, at least for me, the implementation not canonical, so the lint should trigger. I was proposing adding a mechanism to support your particular use case.

@rustbot concern Using return *self is not the canonical implementation of Clone

[Kokoro2336]

@samueltardieu I think your idea is better. So I add a special check for implicit_return. By the way, we can't peel the blocks, or a body with multiple layers of blocks might be still considered canonicalized, so I revert the logic of this part.

[Kokoro2336]

@rustbot ready

[github-actions]

Lintcheck changes for 0d1e095

Lint	Added	Removed	Changed
clippy::non_canonical_clone_impl	0	0	2

This comment will be updated if you push new changes

[Kokoro2336]

r? @samueltardieu

[samueltardieu]

Can you please squash the commits?

@rustbot concern resolve Using return *self is not the canonical implementation of Clone

View changes since this review

[Kokoro2336]

@samueltardieu Resolved

[Kokoro2336]

@rustbot ready

[Kokoro2336]

@samueltardieu Commits squashed

[samueltardieu]

is_lint_allowed() does several lookups. I would prefer that it is invoked only when necessary, that is:

• when a return *self; is found, to check if the return is legitimate there
• when a non-canonical impl is found, to determine whether we want to suggest *self or return *self;.

This way, the lookup won't take place when a regular *self canonical implementation is found (fast path).

Suggested change

	let implicit_return_lint_enabled = !is_lint_allowed(cx, IMPLICIT_RETURN, body_expr.hir_id);
	match is_canonical_clone_body(body_expr) {
	IsCanonical::WithoutReturn => return,
	IsCanonical::WithReturn if implicit_return_lint_enabled => return,
	IsCanonical::WithReturn | IsCanonical::No => {},
	}
	span_lint_and_sugg(
	cx,
	NON_CANONICAL_CLONE_IMPL,
	block.span,
	body_expr.span,
	"non-canonical implementation of `clone` on a `Copy` type",
	"change this to",
	"{ *self }".to_owned(),
	if implicit_return_lint_enabled {
	"{ return *self; }"
	} else {
	"{ *self }"
	}
	.to_owned(),
	Applicability::MaybeIncorrect,
	);
	let add_return = match is_canonical_clone_body(body_expr) {
	IsCanonical::WithReturn if is_lint_allowed(cx, IMPLICIT_RETURN, body_expr.hir_id) => false,
	IsCanonical::WithReturn | IsCanonical::WithoutReturn => return,
	IsCanonical::No => !is_lint_allowed(cx, IMPLICIT_RETURN, body_expr.hir_id),
	};
	span_lint_and_sugg(
	cx,
	NON_CANONICAL_CLONE_IMPL,
	body_expr.span,
	"non-canonical implementation of `clone` on a `Copy` type",
	"change this to",
	if add_return { "{ return *self; }" } else { "{ *self }" }.to_owned(),
	Applicability::MaybeIncorrect,
	);

View changes since the review

[Kokoro2336]

@rustbot ready @samueltardieu Suggestion applied

[samueltardieu]

Did you forget to push?

[Kokoro2336]

@samueltardieu Sorry.

[samueltardieu]

Please squash the commits

View changes since this review

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Kokoro2336]

@rustbot ready @samueltardieu

[Kokoro2336]

@samueltardieu thanks for your patience. this is my first pr merged into clippy's main repo :)