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

Suggest the right help message for as_ref #100617

Merged
merged 1 commit into from
Aug 20, 2022
Merged

Suggest the right help message for as_ref #100617

merged 1 commit into from
Aug 20, 2022

Conversation

chenyukang
Copy link
Member

Fixes #100605

@rust-highfive
Copy link
Collaborator

r? @compiler-errors

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

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 16, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2022
@chenyukang
Copy link
Member Author

chenyukang commented Aug 16, 2022 •
edited
Loading

I thought it will broke some tests,
surprised that we don't have a unittest to cover this error message.

@chenyukang chenyukang changed the title Fix #100605, suggest the right help message for as_ref Suggest the right help message for as_ref Aug 16, 2022
@compiler-errors
Copy link
Member

Thanks

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 20, 2022

📌 Commit 3de74f7 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 20, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 20, 2022
@Dylan-DPC
Rollup merge of rust-lang#100617 - chenyukang:fix-100605, r=compiler-…
767e8db 
…errors

Suggest the right help message for as_ref

Fixes rust-lang#100605
@Dylan-DPC Dylan-DPC mentioned this pull request Aug 20, 2022
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2022
@bors
Auto merge of rust-lang#100793 - matthiaskrgr:rollup-dy7rfdh, r=matth…
36e530c 
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#100186 (Mention `as_mut` alongside `as_ref` in borrowck error message)
 - rust-lang#100383 (Mitigate stale data reads on SGX platform)
 - rust-lang#100507 (suggest `once_cell::Lazy` for non-const statics)
 - rust-lang#100617 (Suggest the right help message for as_ref)
 - rust-lang#100667 (Migrate "invalid variable declaration" errors to SessionDiagnostic)
 - rust-lang#100709 (Migrate typeck's `used` expected symbol diagnostic to `SessionDiagnostic`)
 - rust-lang#100723 (Add the diagnostic translation lints to crates that don't emit them)
 - rust-lang#100729 (Avoid zeroing a 1kb stack buffer on every call to `std::sys::windows::fill_utf16_buf`)
 - rust-lang#100750 (improved diagnostic for function defined with `def`, `fun`, `func`, or `function` instead of `fn`)
 - rust-lang#100763 (triagebot: Autolabel `A-rustdoc-json`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 61a529d into rust-lang:master Aug 20, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 20, 2022
WaffleLapkin
WaffleLapkin reviewed Aug 21, 2022
View reviewed changes
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

I don't think this fixes the problem fully. We have at least 3 other unary operators, namely *, ! and - that can return arbitrary values and for them the diagnostic is still broken. Maybe we should to do something more general with method suggestions?...

@chenyukang
Copy link
Member Author

chenyukang commented Aug 24, 2022 •
edited
Loading

I don't think this fixes the problem fully. We have at least 3 other unary operators, namely *, ! and - that can return arbitrary values and for them the diagnostic is still broken. Maybe we should to do something more general with method suggestions?...

Yes, I didn't consider these cases, this need some extra work.
Maybe we can remove this hard coded part in the help message:

*none.as_ref()

5 |     takes_option(*none);    // Suggests `*none.as_ref()`
  |     ------------ ^^^^^^^^^^^^^^
  |     |            |
  |     |            expected enum `Option`, found `&Option<String>`
  |     |            help: you can convert from `&Option<T>` to `Option<&T>` using `.as_ref()`: `*none.as_ref()`
  |     arguments to this function are incorrect
  |
  = note:   expected enum `Option<&String>`
          found reference `&Option<String>`

Ofcourse, the right and best diagnostic is we need when to add the '( )'

(*none).as_ref()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

@WaffleLapkin WaffleLapkin WaffleLapkin left review comments

Assignees

@compiler-errors compiler-errors

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.

rustc suggests &None.as_ref() as well as None
6 participants
@chenyukang @rust-highfive @compiler-errors @bors @WaffleLapkin @rustbot