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

Consider borrowing here: &0 #52544

Closed
jjl opened this issue Jul 19, 2018 · 15 comments · Fixed by #117893
Closed

Consider borrowing here: &0 #52544

jjl opened this issue Jul 19, 2018 · 15 comments · Fixed by #117893
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jjl
Copy link

jjl commented Jul 19, 2018

error[E0308]: mismatched types
  --> src/schema_parser.rs:91:20
   |
91 |             if i < 0 {
   |                    ^
   |                    |
   |                    expected &i64, found integral variable
   |                    help: consider borrowing here: `&0`
   |
   = note: expected type `&i64`
              found type `{integer}`

What I actually wanted was *i.

@oli-obk oli-obk added E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. A-diagnostics Area: Messages for errors, warnings, and lints labels Jul 19, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jul 19, 2018

The problem is by the time this error is emittedwe don't have the information about the root cause anymore. In this case that would be the type of i being checked for PartialOrd and figuring out the the rhs needs to be of the same type.

Clippy has a lint that will tell you to clean it up afterwards, but we should probably issue a better diagnostic to begin with.

@hellow554
Copy link
Contributor

hellow554 commented Jul 20, 2018

@jjl, can you please create a MCVE?

@csmoe
Copy link
Member

csmoe commented Jul 21, 2018

@hellow554

fn main() {
    let i: &i64 = &1;
    if i < 0 {}
}

http://play.rust-lang.org/?gist=23f2d7858abee581e2cc0221fb67a7bc&version=nightly&mode=debug&edition=2015

@hellow554
Copy link
Contributor

hellow554 commented Jul 23, 2018

Please excuse my naïvety, but why is this (according to csmoes MCVE) a problem? You can actually write

if i < &0 {}

and everything works just fine. Can you please explain this part (to me)?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 23, 2018

The comparison operator already takes references to both sides. so it seems simpler to dereference the lhs and save the monomorphizer and the optimizer some effort

@estebank estebank added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. labels Sep 26, 2019
@JohnTitor JohnTitor added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Oct 19, 2019
@rakshith-ravi
Copy link
Contributor

Resurrecting this.

@oli-obk what're the benefits of dereferencing the lhs instead of referencing the rhs?

Also, if I were to go about fixing this, where would I start?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 17, 2020

The benefit is that x < y expands to Ord::cmp(&x, &y), so if you start out with x < &y you get Ord::cmp(&x, &&y), seems better to just do Ord::cmp(&*x, y) instead (since &* is a no-op that gets easily removed).

I'm not sure how to solve this or whether we should try at all.

@rakshith-ravi
Copy link
Contributor

@rustbot claim

@rustbot rustbot self-assigned this Apr 17, 2020
@rakshith-ravi
Copy link
Contributor

I want to get started with this, but would appreciate some mentoring from someone before I do, to point me in the right direction please

@oli-obk
Copy link
Contributor

oli-obk commented Apr 22, 2020

There's a reason I marked this as E-hard. I have no idea how to do this reasonably. I'm sorry, but I have no hints for you, and I don't even know if it's possible to do in a reasonable way.

@rakshith-ravi
Copy link
Contributor

rakshith-ravi commented Apr 22, 2020

*cue terminator background music *
Leaving this here for future generations to see. I'm gonna try attempting to solve this and see if I can do something about it. I don't know if I can, or if I'll even survive this, but there's only one way to find out.

Lol in all seriousness, if someone wants to claim this up, feel free to. Until then, I'll try and fix this.

@Alexendoo
Copy link
Member

Hi, are you still working on this issue @rakshith-ravi?

@rakshith-ravi
Copy link
Contributor

Hey @Alexendoo. I've been meaning to fix this up. I've reached a point where I know exactly how to fix this as well. I just haven't found the time to fix this. Feel free to take it up, considering I wouldn't be having the time to work on this in the forseeable future.

@Alexendoo
Copy link
Member

@rustbot release-assignment

If do you find the time in the future, please claim this issue again. Until then I'll free it up in case anybody else wants it

@sjwang05
Copy link
Contributor

Is anyone currently working on this? If not, I'd be happy to give it a go.

@rustbot claim

rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 15, 2023
Rollup merge of rust-lang#117893 - sjwang05:issue-52544-take-1, r=wesleywiser

Suggest dereferencing the LHS for binops such as `&T == T`

Fixes rust-lang#52544
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants