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

Suggest `.as_ref()` when appropriate for `Option` and `Result` #57158

Merged
merged 2 commits into from Dec 30, 2018

Conversation

Projects
None yet
5 participants
@estebank
Copy link
Contributor

estebank commented Dec 28, 2018

Fix #55198.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 28, 2018

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

@zackmdavis

This comment has been minimized.

Copy link
Member

zackmdavis commented Dec 28, 2018

(hope to catch up on reviews Saturday the twenty-ninth)

@Xanewok

This comment has been minimized.

Copy link
Member

Xanewok commented Dec 28, 2018

I know I missed this sorely when I was at the beginning of my Rust journey. Many thanks for working on this!

@estebank estebank force-pushed the estebank:as-ref branch 2 times, most recently from a945167 to 1c31447 Dec 28, 2018

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 28, 2018

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0ce7c7f2:start=1546020317087783815,finish=1546020374232458875,duration=57144675060
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[01:01:56] ................................................................i................................... 4400/5202
[01:02:02] .................................................................................................... 4500/5202
[01:02:05] .................................................................................................... 4600/5202
[01:02:08] .................................................................................................... 4700/5202
[01:02:12] .................F.................................................................................. 4800/5202
[01:02:19] .................................................................................................... 5000/5202
[01:02:19] .................................................................................................... 5000/5202
xt":[{"text":"  opt.and_then(|arg| Some(takes_ref(arg)));","highlight_start":37,"highlight_end":40}],"label":"expected &Foo, found struct `Foo`","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"expected type `&Foo`\n   found type `Foo`","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"consider using `as_ref` instead","code":null,"level":"help","spans":[{"file_name":"/checkout/src/test/ui/suggestions/as-ref.rs","byte_start":154,"byte_end":154,"line_start":8,"line_end":8,"column_start":7,"column_end":7,"is_primary":true,"text":[{"text":"  opt.and_then(|arg| Some(takes_ref(arg)));","highlight_start":7,"highlight_end":7}],"label":null,"suggested_replacement":"as_ref().","suggestion_applicability":"MachineApplicable","expansion":null}],"children":[],"rendered":null}],"rendered":"error[E0308]: mismatched types\n  --> /checkout/src/test/ui/suggestions/as-ref.rs:8:37\n   |\nLL |   opt.and_then(|arg| Some(takes_ref(arg)));\n   |       -                             ^^^ expected &Foo, found struct `Foo`\n   |       |\n   |       help: consider using `as_ref` instead: `as_ref().`\n   |\n   = note: expected type `&Foo`\n              found type `Foo`\n\n"}
[01:02:24] {"message":"mismatched types","code":{"code":"E0308","explanation":"\nThis error occurs when the compiler was unable to infer the concrete type of a\nvariable. It can occur for several cases, the most common of which is a\nmismatch in the expected type that the compiler inferred for a variable's\ninitializing expression, and the actual type explicitly assigned to the\nvariable.\n\nFor exam

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@estebank estebank force-pushed the estebank:as-ref branch from 1c31447 to 8da6727 Dec 28, 2018

let mut show_suggestion = true;
for (exp_ty, found_ty) in exp_substs.types().zip(found_substs.types()) {
if let TyKind::Ref(_, exp_ty, _) = exp_ty.sty {
match (&exp_ty.sty, &found_ty.sty) {

This comment has been minimized.

@estebank

estebank Dec 29, 2018

Contributor

I would like to replace this entire match with FnCtxt::can_coerce, but first I have to find a way to get ahold of a FnCtxt here :-|

@petrochenkov when you have the time, could you point me in the right direction for this? I've noticed that we have same_tys and same_type which don't do exactly the same thing, but are pretty similar...

@zackmdavis
Copy link
Member

zackmdavis left a comment

I have some quibbles, but I don't think they're actually blocking (as opposed to providing more pressure towards things like "We should really do that error-message language audit someday")

LL | opt.map(|arg| takes_ref(arg));
| - ^^^ expected &Foo, found struct `Foo`
| |
| help: consider using `as_ref` instead: `as_ref().`

This comment has been minimized.

@zackmdavis

zackmdavis Dec 30, 2018

Member

as_ref(). (trailing dot!) as the suggestion text faked me out for a moment; our insertion suggestions can look kind of awkward sometimes. (But making the span cover opt.map and suggesting opt.as_ref().map is ugly in its own way.)

In any case, the code that sets this was actually introduced in #51100, not here, so it doesn't affect the outcome of this review.

if let TyKind::Adt(found_def, found_substs) = found_ty.sty {
let path_str = format!("{:?}", exp_def);
if exp_def == &found_def {
let opt_msg = "you can convert from `&Option<T>` to `Option<&T>` using \

This comment has been minimized.

@zackmdavis

zackmdavis Dec 30, 2018

Member
Suggested change Beta
let opt_msg = "you can convert from `&Option<T>` to `Option<&T>` using \
let opt_msg = "`&Option<T>` can be converted to `Option<&T>` using \

I lean against the second person in diagnostic messages. (But that's my opinion; we haven't codifed that anywhere.)

match (&exp_found.expected.sty, &exp_found.found.sty) {
(TyKind::Adt(exp_def, exp_substs), TyKind::Ref(_, found_ty, _)) => {
if let TyKind::Adt(found_def, found_substs) = found_ty.sty {
let path_str = format!("{:?}", exp_def);

This comment has been minimized.

@zackmdavis

zackmdavis Dec 30, 2018

Member

Relying on debug output for functionality seems like poor form? (On the grounds that it's meant for debugging convenience and should be subject to change rather than guaranteed stable, even if it happens to be probably-stable in this case.)

@@ -972,6 +973,72 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
self.note_error_origin(diag, &cause);
}

/// When encountering a case where `.as_ref()` on a `Result` or `Option` would be appropriate,
/// suggest it.
fn suggest_as_ref_where_appropriate(

This comment has been minimized.

@zackmdavis

zackmdavis Dec 30, 2018

Member

I like _where_appropriate as part of a function name, but I don't think I've seen it used anywhere else, and I know we have a lot of these "conditionally set a diagnostic" functions ...

@zackmdavis

This comment has been minimized.

Copy link
Member

zackmdavis commented Dec 30, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 30, 2018

📌 Commit 0ecc128 has been approved by zackmdavis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 30, 2018

⌛️ Testing commit 0ecc128 with merge 499a5af...

bors added a commit that referenced this pull request Dec 30, 2018

Auto merge of #57158 - estebank:as-ref, r=zackmdavis
Suggest `.as_ref()` when appropriate for `Option` and `Result`

Fix #55198.
@bors

This comment was marked as outdated.

Copy link
Contributor

bors commented Dec 30, 2018

💔 Test failed - status-travis

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 30, 2018

The job dist-i686-freebsd of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Building stage2 tool cargo (i686-unknown-freebsd)
[01:15:00]  Downloading crates ...
[01:15:20] warning: spurious network error (2 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io)
[01:15:40] warning: spurious network error (1 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io)
[01:16:00] error: failed to download from `https://crates.io/api/v1/crates/openssl-src/111.1.0+1.1.1a/download`
[01:16:00] Caused by:
[01:16:00]   [6] Couldn't resolve host name (Could not resolve host: crates.io)
[01:16:00] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "i686-unknown-freebsd" "-j" "4" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/cargo/Cargo.toml" "--features" "rustc-workspace-hack/all-static" "--message-format" "json"
[01:16:00] expected success, got: exit code: 101
---
travis_time:end:0d066e10:start=1546153859804048951,finish=1546153859812849868,duration=8800917
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1484957d
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:094f3490
travis_time:start:094f3490
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:07407219
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Dec 30, 2018

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 30, 2018

⌛️ Testing commit 0ecc128 with merge 7155690...

bors added a commit that referenced this pull request Dec 30, 2018

Auto merge of #57158 - estebank:as-ref, r=zackmdavis
Suggest `.as_ref()` when appropriate for `Option` and `Result`

Fix #55198.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 30, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: zackmdavis
Pushing 7155690 to master...

@bors bors merged commit 0ecc128 into rust-lang:master Dec 30, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment