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

Added from_over_into lint #6476

Merged
merged 5 commits into from
Dec 22, 2020
Merged

Added from_over_into lint #6476

merged 5 commits into from
Dec 22, 2020

Conversation

1c3t3a
Copy link
Contributor

@1c3t3a 1c3t3a commented Dec 19, 2020 •
edited by llogiq
Loading

Closes #6456
Added a lint that searches for implementations of Into<..> and suggests to implement From<..> instead, as it comes with a default implementation of Into. Category: style.

changelog: added from_over_into lint

@rust-highfive
Copy link

r? @llogiq

(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 Dec 19, 2020
llogiq
llogiq requested changes Dec 19, 2020
View reviewed changes
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

Only one nit and one suggestion for improvement. If you change the message, I'd be OK with merging and improving the span later.

clippy_lints/src/from_over_into.rs Outdated Show resolved Hide resolved
clippy_lints/src/from_over_into.rs
span_lint_and_help(
cx,
FROM_OVER_INTO,
item.span,
Copy link
Contributor

Choose a reason for hiding this comment

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

That item might be quite large. We may want to reduce the span to the impl header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How exactly would I extract the impl header? Didn't find a proper span while looking through documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's OK, we can do this as a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oke. If it's fine with you I could work on this, I might just need a hint on extracting the imp header Span.

@ghost
Copy link

ghost commented Dec 20, 2020

Does this require a MSRV setting? The From docs suggest that Into might be needed if supporting a version prior to 1.41.

Only implement Into when targeting a version prior to Rust 1.41 and converting to a type outside the current crate. From was not able to do these types of conversions in earlier versions because of Rust's orphaning rules. See Into for more details.

@1c3t3a
Copy link
Contributor Author

1c3t3a commented Dec 20, 2020

Does this require a MSRV setting? The From docs suggest that Into might be needed if supporting a version prior to 1.41.

Only implement Into when targeting a version prior to Rust 1.41 and converting to a type outside the current crate. From was not able to do these types of conversions in earlier versions because of Rust's orphaning rules. See Into for more details.

That's a good point, I've missed that!

@1c3t3a 1c3t3a requested a review from llogiq December 20, 2020 15:45
llogiq
llogiq approved these changes Dec 20, 2020
View reviewed changes
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

I forgot that we want items/types marked up as code in error messages. Once that is taken care of, I'll have this merged. Thank you!

@1c3t3a 1c3t3a requested a review from llogiq December 20, 2020 19:11
@1c3t3a 1c3t3a force-pushed the 1c3t3a-from-over-into branch from a7299c4 to 7e641c8 Compare December 20, 2020 21:00
@llogiq @giraffate
Update clippy_lints/src/from_over_into.rs
0d6c128 
Co-authored-by: Takayuki Nakata <f.seasons017@gmail.com>
@llogiq
Copy link
Contributor

llogiq commented Dec 21, 2020

Thanks everyone! @bors r+

@bors
Copy link
Contributor

bors commented Dec 21, 2020

📌 Commit 0d6c128 has been approved by llogiq

bors added a commit that referenced this pull request Dec 21, 2020
@bors
Auto merge of #6476 - 1c3t3a:1c3t3a-from-over-into, r=llogiq
74bce2d 
Added from_over_into lint

Closes #6456
Added a lint that searches for implementations of `Into<..>` and suggests to implement `From<..>` instead, as it comes with a default implementation of `Into`. Category: style.
@bors
Copy link
Contributor

bors commented Dec 21, 2020

⌛ Testing commit 0d6c128 with merge 74bce2d...

@bors
Copy link
Contributor

bors commented Dec 21, 2020

💔 Test failed - checks-action_test

@giraffate
Copy link
Contributor

We need to update .stderr file to pass a test.

@1c3t3a 1c3t3a requested a review from giraffate December 21, 2020 16:26
@llogiq
Copy link
Contributor

llogiq commented Dec 22, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Dec 22, 2020

📌 Commit 53f4d43 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Dec 22, 2020

⌛ Testing commit 53f4d43 with merge 6d73885...

bors added a commit that referenced this pull request Dec 22, 2020
@bors
Auto merge of #6476 - 1c3t3a:1c3t3a-from-over-into, r=llogiq
6d73885 
Added from_over_into lint

Closes #6456
Added a lint that searches for implementations of `Into<..>` and suggests to implement `From<..>` instead, as it comes with a default implementation of `Into`. Category: style.
@bors
Copy link
Contributor

bors commented Dec 22, 2020

💔 Test failed - checks-action_test

@giraffate
Copy link
Contributor

Oh, a test seems to fail due to lack of changelog: in PR.

@1c3t3a
Copy link
Contributor Author

1c3t3a commented Dec 22, 2020

Oh, a test seems to fail due to lack of changelog: in PR.

Ahh I forgot to add the comment. I edited my PR opening comment, is that enough to make bors happy?

@llogiq
Copy link
Contributor

llogiq commented Dec 22, 2020

Let's see. @bors r+

@bors
Copy link
Contributor

bors commented Dec 22, 2020

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Contributor

bors commented Dec 22, 2020

📌 Commit 53f4d43 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Dec 22, 2020

⌛ Testing commit 53f4d43 with merge 25e9acb...

@bors
Copy link
Contributor

bors commented Dec 22, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 25e9acb to master...

@bors bors merged commit 25e9acb into rust-lang:master Dec 22, 2020
@giraffate giraffate mentioned this pull request Jan 8, 2021
Merged
bors added a commit that referenced this pull request Jan 8, 2021
@bors
Auto merge of #6566 - giraffate:reduce_the_span_in_from_over_into, r=…
2950c8e 
…Manishearth

Reduce the span in `from_over_into` to impl header

A follow up of #6476 (comment)
> That item might be quite large. We may want to reduce the span to the `impl` header.

changelog: none
@Fishrock123 Fishrock123 mentioned this pull request Jan 19, 2021
Closed
@camsteffen camsteffen mentioned this pull request Oct 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llogiq llogiq Awaiting requested review from llogiq

@giraffate giraffate Awaiting requested review from giraffate

Assignees

@llogiq llogiq

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New lint: Implement From instead of Into
5 participants
@1c3t3a @rust-highfive @llogiq @bors @giraffate