Fix4894 Using `Into` and `TryInto` instead of `From` and `TryFrom` #6620

xiongmao86 · 2021-01-22T06:35:03Z

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

Followed [lint naming conventions][lint_naming]
Added lint documentation

check again after every change:

cargo test passes locally
Executed cargo dev update_lints
Run cargo dev fmt
Added passing UI tests (including committed .stderr file)

Fixes #4894. Using Into and TryInto instead of using From and TryFrom.

Please write a short comment explaining your change (or "none" for internal only changes)
changelog:
Using Into and TryInto instead of using From and TryFrom.

rust-highfive · 2021-01-22T06:35:06Z

r? @phansch

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

xiongmao86 · 2021-01-22T06:35:43Z

Oh, this is a draft for discussion, how can I disable r?

clippy_lints/src/into_and_try_into_instead_of_from_and_try_from.rs

xiongmao86 · 2021-01-22T08:36:18Z

r? @flip1995
Can you take a look when you have time?

clippy_lints/src/into_and_try_into_instead_of_from_and_try_from.rs

clippy_lints/src/lib.rs

…jasper Make more traits of the From/Into family diagnostic items Following traits are now diagnostic items: - `From` (unchanged) - `Into` - `TryFrom` - `TryInto` This also adds symbols for those items: - `into_trait` - `try_from_trait` - `try_into_trait` Related: rust-lang/rust-clippy#6620 (comment)

Make more traits of the From/Into family diagnostic items Following traits are now diagnostic items: - `From` (unchanged) - `Into` - `TryFrom` - `TryInto` This also adds symbols for those items: - `into_trait` - `try_from_trait` - `try_into_trait` Related: rust-lang#6620 (comment)

flip1995 · 2021-01-31T15:01:05Z

@xiongmao86 You now should also have access to the diagnostic items for try_from_trait (and to both Into traits if needed). Implementation LGTM otherwise.

xiongmao86 · 2021-01-31T15:09:56Z

@flip1995 Thanks for the guidance, I found toolchain update today and I git pull upstream master, and after git rebase master fix4894, I can't git push -u origin fix4894.

The error message is:

 ! [rejected]            fix4894 -> fix4894 (non-fast-forward)
error: failed to push some refs to 'https://github.com/xiongmao86/rust-clippy.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

and git branch -v says fix4894 is [ahead 243, behind 6].

What is the problem?

flip1995 · 2021-01-31T15:13:48Z

When you rebase, you have to force push. Force pushing deletes your previous commits from the repository and you can't get them back (well you can if you still have them locally, but that requires much more git-fu), so you should be careful.

If your rebase goes through without any conflicts (which I assume is the case here) force pushing is usually save after a rebase. So git push -u origin fix4894 --force-with-lease should solve your problem. (Always use --force-with-lease instead of -f or --force, it gives you another safeguard).

xiongmao86 · 2021-01-31T15:43:15Z

@flip1995 , May be I mistake -f for -u. The git docs says "-u" means adding upstream reference. I don't really understand, what is it used for in this case?

xiongmao86 · 2021-02-01T09:08:24Z

@flip1995, I was thinking maybe I expressed this item in the wrong way, maybe the name should be FromInsteadOfInto, so I can said "allow FromInsteadOfInto"? Is it right? I am not sure?

xiongmao86 · 2021-02-01T09:18:24Z

And about the -u option, I read the branch.<name>.merge part of git config, there are a lot concept to make clear, so I don't know how to ask properly.

The description is:

-u
--set-upstream
For every branch that is up to date or successfully pushed, add upstream (tracking) reference, 
used by argument-less git-pull(1) and other commands. For more information, see branch.<name>.merge 
in git-config(1).

And I see now the point is to understand the concept of upstream(tracking) reference, could you tell me what is it, or could you show me the way to make sense of it?

flip1995 · 2021-02-01T09:22:19Z

The git docs says "-u" means adding upstream reference. I don't really understand, what is it used for in this case?

So git push needs an address to know where to push the commits. So git push is actually just a shortcut for git push <remote> <branch> (git push origin fix4894). To be able to use this shortcut you first have to tell git about the address it should use for this shortcut. You do this with -u aka --set-upstream. You know if you have set this shortcut if you see the line

Your branch is up-to-date with 'origin/master'.

when running git status.

I was thinking maybe I expressed this item in the wrong way, maybe the name should be FromInsteadOfInto, so I can said "allow FromInsteadOfInto"? Is it right? I am not sure?

I'm not really sure what you mean. The name of the struct does not really matter for the lint. The lint name comes from the all-caps name in the declare_clippy_lint! macro.

flip1995 · 2021-02-01T09:27:32Z

And I see now the point is to understand the concept of upstream(tracking) reference, could you tell me what is it, or could you show me the way to make sense of it?

You can understand upstream as the online repository vs your local checkout on your computer.

xiongmao86 · 2021-02-01T23:50:01Z

@flip1995, thanks for your explanation, I see what -u is using now, it's for shortcut using.

About the lint name, I have not been clear. I used to write lint name IntoInsteadOfFrom Into in front and From at last, but I think FromInsteadOfInto, From in front, and Into at last would be more appropriate.

And to read#[allow(from_instead_of_into)] as "allow using from trait instead of using into trait". If the lint wouldn't be triggered if we allow it. Is this understanding correct?

xiongmao86 · 2021-02-02T08:37:59Z

I think I may have made a mistake when I think of naming the lint. I want to make sure the naming is right.

flip1995 · 2021-02-02T15:23:42Z

Ah good catch. Yeah from_instead_of_into is the better name 👍

Co-authored-by: Philipp Krones <hello@philkrones.com>

xiongmao86 · 2021-02-24T08:13:45Z

tests/ui/from_instead_of_into.stderr

+help: remove From bound
+   |
+LL |     ,
+   |    --


This is probably not the best diagnostic message, From<T> is removed and only comma left.

Yeah, I don't even know if that still compiles. I think you have to track how many bounds there are and tweak the suggestion depending on this.

xiongmao86 · 2021-02-24T08:15:14Z

tests/ui/from_instead_of_into.stderr

+help: Add this bound predicate
+   |
+LL |     u32: From<T>T: Into<u32>,
+   |                 ^^^^^^^^^^^^
+


I made a mistake here. Didn't notice until now. I'll fix this.

xiongmao86 · 2021-02-24T08:16:56Z

tests/ui/from_instead_of_into.stderr

+   |                     --
+help: Add this bound predicate
+   |
+LL |     u32: Copy + Clone + From<T>, T: Into<u32>,


I think this probably should be u32: Copy + Clone, T: Into<u32>, but I don't know how to remove From showing in this span.

You have to get the span of the From<T> bound and extend it to the end of the span of the Clone bound with span.with_lo(clone_bound_span.hi()) And as the suggestion you can just use String::new(), which will automatically suggest removing it.

xiongmao86 · 2021-02-28T05:49:55Z

@flip1995, are you suggesting using multispan_sugg?

bors · 2021-02-28T21:09:12Z

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

flip1995 · 2021-03-10T16:21:51Z

You can use multispan_sugg or just add 2 suggestions, by calling diag.span_suggestion 2 times with different spans and suggestions. I would first try the latter. If that doesn't work or looks weird, I would try mutlispan_sugg.

flip1995 · 2021-03-10T16:25:31Z

Sorry for being absent for so long. I currently can't really keep up with my reviews...

xiongmao86 · 2021-03-23T07:37:40Z

Sorry, @flip1995, seems github.com is ban again at my location. I am trying to get to github.com(read prs, push commits etc.) in a secure way.

xiongmao86 · 2021-03-23T07:38:55Z

Sorry for being absent for so long. I currently can't really keep up with my reviews...

Thank you very much for your help. Please take your time.

giraffate · 2021-04-08T01:35:49Z

ping from triage @xiongmao86. Can you have any update on this?

xiongmao86 · 2021-04-09T09:41:24Z

@giraffate, I recently don't have time for this, if you want to take over, please go ahead.

giraffate · 2021-04-28T14:27:57Z

ping from triage @xiongmao86. According to the triage procedure, I'm closing this because 2 weeks have passed with no activity.

rust-highfive assigned phansch Jan 22, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 22, 2021

xiongmao86 commented Jan 22, 2021

View reviewed changes

clippy_lints/src/into_and_try_into_instead_of_from_and_try_from.rs Outdated Show resolved Hide resolved

xiongmao86 changed the title ~~Fix4894~~ Fix4894 Using Into and TryInto instead of From and TryFrom Jan 22, 2021

rust-highfive assigned flip1995 and unassigned phansch Jan 22, 2021

flip1995 reviewed Jan 22, 2021

View reviewed changes

clippy_lints/src/into_and_try_into_instead_of_from_and_try_from.rs Outdated Show resolved Hide resolved

clippy_lints/src/into_and_try_into_instead_of_from_and_try_from.rs Outdated Show resolved Hide resolved

clippy_lints/src/lib.rs Outdated Show resolved Hide resolved

flip1995 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 Jan 22, 2021

flip1995 mentioned this pull request Jan 22, 2021

Make more traits of the From/Into family diagnostic items rust-lang/rust#81277

Merged

xiongmao86 force-pushed the fix4894 branch from cb1c415 to 40d74fd Compare January 31, 2021 15:24

xiongmao86 and others added 12 commits February 24, 2021 16:06

Using From trait in this test is irrelevant.

c4cae20

Suggestion may be incorrect.

236b82e

Co-authored-by: Philipp Krones <hello@philkrones.com>

Migrate to use find.

95c1123

Work with multiply bounds now.

867a07f

Update due to review suggestion.

cf542a2

Fix wrong filter closure.

32d4643

Finish lint logic, add 1 more case for test.

04b00ea

Satisfy with test.

655520f

Refactor and update test.

9eea1b6

Fix workspace warning.

ae2b38d

Using span_lint_and_then method to lint.

94232f3

Reference for pull request.

785d607

xiongmao86 force-pushed the fix4894 branch from a8d0f64 to 785d607 Compare February 24, 2021 08:11

xiongmao86 commented Feb 24, 2021

View reviewed changes

flip1995 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 Mar 19, 2021

giraffate closed this Apr 28, 2021

giraffate added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix4894 Using `Into` and `TryInto` instead of `From` and `TryFrom` #6620

Fix4894 Using `Into` and `TryInto` instead of `From` and `TryFrom` #6620

xiongmao86 commented Jan 22, 2021 •

edited

Loading

rust-highfive commented Jan 22, 2021

xiongmao86 commented Jan 22, 2021

xiongmao86 commented Jan 22, 2021

flip1995 commented Jan 31, 2021

xiongmao86 commented Jan 31, 2021

flip1995 commented Jan 31, 2021 •

edited

Loading

xiongmao86 commented Jan 31, 2021

xiongmao86 commented Feb 1, 2021

xiongmao86 commented Feb 1, 2021 •

edited

Loading

flip1995 commented Feb 1, 2021

flip1995 commented Feb 1, 2021

xiongmao86 commented Feb 1, 2021 •

edited

Loading

xiongmao86 commented Feb 2, 2021

flip1995 commented Feb 2, 2021

xiongmao86 Feb 24, 2021

flip1995 Mar 10, 2021

xiongmao86 Feb 24, 2021

xiongmao86 Feb 24, 2021

flip1995 Mar 10, 2021

xiongmao86 commented Feb 28, 2021

bors commented Feb 28, 2021

flip1995 commented Mar 10, 2021

flip1995 commented Mar 10, 2021

xiongmao86 commented Mar 23, 2021

xiongmao86 commented Mar 23, 2021

giraffate commented Apr 8, 2021

xiongmao86 commented Apr 9, 2021

giraffate commented Apr 28, 2021

Fix4894 Using Into and TryInto instead of From and TryFrom #6620

Fix4894 Using Into and TryInto instead of From and TryFrom #6620

Conversation

xiongmao86 commented Jan 22, 2021 • edited Loading

rust-highfive commented Jan 22, 2021

xiongmao86 commented Jan 22, 2021

xiongmao86 commented Jan 22, 2021

flip1995 commented Jan 31, 2021

xiongmao86 commented Jan 31, 2021

flip1995 commented Jan 31, 2021 • edited Loading

xiongmao86 commented Jan 31, 2021

xiongmao86 commented Feb 1, 2021

xiongmao86 commented Feb 1, 2021 • edited Loading

flip1995 commented Feb 1, 2021

flip1995 commented Feb 1, 2021

xiongmao86 commented Feb 1, 2021 • edited Loading

xiongmao86 commented Feb 2, 2021

flip1995 commented Feb 2, 2021

xiongmao86 Feb 24, 2021

Choose a reason for hiding this comment

flip1995 Mar 10, 2021

Choose a reason for hiding this comment

xiongmao86 Feb 24, 2021

Choose a reason for hiding this comment

xiongmao86 Feb 24, 2021

Choose a reason for hiding this comment

flip1995 Mar 10, 2021

Choose a reason for hiding this comment

xiongmao86 commented Feb 28, 2021

bors commented Feb 28, 2021

flip1995 commented Mar 10, 2021

flip1995 commented Mar 10, 2021

xiongmao86 commented Mar 23, 2021

xiongmao86 commented Mar 23, 2021

giraffate commented Apr 8, 2021

xiongmao86 commented Apr 9, 2021

giraffate commented Apr 28, 2021

Fix4894 Using `Into` and `TryInto` instead of `From` and `TryFrom` #6620

Fix4894 Using `Into` and `TryInto` instead of `From` and `TryFrom` #6620

xiongmao86 commented Jan 22, 2021 •

edited

Loading

flip1995 commented Jan 31, 2021 •

edited

Loading

xiongmao86 commented Feb 1, 2021 •

edited

Loading

xiongmao86 commented Feb 1, 2021 •

edited

Loading