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

Suggest solutions for fn foo(&foo: Foo) #38605

Merged
merged 5 commits into from
Jan 12, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/librustc_typeck/check/_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ use syntax_pos::Span;

impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
pub fn check_pat(&self, pat: &'gcx hir::Pat, expected: Ty<'tcx>) {
self.check_pat_arg(pat, expected, false);
}

pub fn check_pat_arg(&self, pat: &'gcx hir::Pat, expected: Ty<'tcx>, is_arg: bool) {
let tcx = self.tcx;

debug!("check_pat(pat={:?},expected={:?})", pat, expected);
Expand Down Expand Up @@ -212,7 +216,19 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
let mt = ty::TypeAndMut { ty: inner_ty, mutbl: mutbl };
let region = self.next_region_var(infer::PatternRegion(pat.span));
let rptr_ty = tcx.mk_ref(region, mt);
self.demand_eqtype(pat.span, expected, rptr_ty);
let err = self.demand_eqtype_diag(pat.span, expected, rptr_ty);
if let Some(mut err) = err {
if is_arg {
if let Ok(snippet) = self.sess().codemap()
.span_to_snippet(pat.span)
{
err.help(&format!("did you mean `{}: &{}`?",
Copy link
Contributor

Choose a reason for hiding this comment

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

One concern -- what happens in more extreme cases? I think we should check that the inner pattern (represented by inner, here) is a binding pattern, so as not to trigger for something like fn foo(&&bar: &u32) or fn foo(&[bar]: &u32). Suggesting &bar: &&u32 and [bar]: &&u32 respectively doesn't seem all that helpful (that is what we will do, right?)

Copy link
Contributor Author

@estebank estebank Jan 6, 2017

Choose a reason for hiding this comment

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

I tried adding these cases to the test and the results where:

fn ugh(&[bar]: &u32) {
}

error: slice pattern syntax is experimental (see issue #23121)
  --> ../../src/test/ui/mismatched_types/issue-38371.rs:26:9
   |
26 | fn ugh(&[bar]: &u32) {
   |         ^^^^^
fn agh(&&bar: &u32) {
}

error[E0308]: mismatched types
  --> ../../src/test/ui/mismatched_types/issue-38371.rs:29:9
   |
29 | fn agh(&&bar: &u32) {
   |         ^^^^ expected u32, found reference
   |
   = note: expected type `u32`
   = note:    found type `&_`

without changes to the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, the first one is a case of needing a feature-gate, but I'm surprised by the second one. What does &&bar: &Foo do? I can't tell why the help doesn't trigger here...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't tell why the help doesn't trigger here...?

Neither can I. I've never even thought about trying to use something like (&&bar: &u32).

the first one is a case of needing a feature-gate

I figured as much. Last time I looked at this I couldn't find the appropriate feature gate, but just googled again and found it. I added the test back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'll pull your branch and build it locally and try to figure out what is going on then. I mean the tests look pretty good but I'd rather not enable misleading output if we can help it. Might take a day or two though.

&snippet[1..],
expected));
}
}
err.emit();
}
(rptr_ty, inner_ty)
}
};
Expand Down
18 changes: 14 additions & 4 deletions src/librustc_typeck/check/demand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use syntax_pos::{self, Span};
use rustc::hir;
use rustc::hir::def::Def;
use rustc::ty::{self, AssociatedItem};
use errors::DiagnosticBuilder;

use super::method::probe;

Expand All @@ -38,20 +39,29 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
}

pub fn demand_eqtype(&self, sp: Span, expected: Ty<'tcx>, actual: Ty<'tcx>) {
self.demand_eqtype_with_origin(&self.misc(sp), expected, actual);
if let Some(mut err) = self.demand_eqtype_diag(sp, expected, actual) {
err.emit();
}
}

pub fn demand_eqtype_diag(&self,
sp: Span,
expected: Ty<'tcx>,
actual: Ty<'tcx>) -> Option<DiagnosticBuilder<'tcx>> {
self.demand_eqtype_with_origin(&self.misc(sp), expected, actual)
}

pub fn demand_eqtype_with_origin(&self,
cause: &ObligationCause<'tcx>,
expected: Ty<'tcx>,
actual: Ty<'tcx>)
{
actual: Ty<'tcx>) -> Option<DiagnosticBuilder<'tcx>> {
match self.eq_types(false, cause, actual, expected) {
Ok(InferOk { obligations, value: () }) => {
self.register_predicates(obligations);
None
},
Err(e) => {
self.report_mismatched_types(cause, expected, actual, e).emit();
Some(self.report_mismatched_types(cause, expected, actual, e))
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ fn check_fn<'a, 'gcx, 'tcx>(inherited: &'a Inherited<'a, 'gcx, 'tcx>,
fcx.register_old_wf_obligation(arg_ty, arg.pat.span, traits::MiscObligation);

// Check the pattern.
fcx.check_pat(&arg.pat, arg_ty);
fcx.check_pat_arg(&arg.pat, arg_ty, true);
fcx.write_ty(arg.id, arg_ty);
}

Expand Down
4 changes: 3 additions & 1 deletion src/librustc_typeck/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,9 @@ impl<'ccx, 'gcx> CheckTypeWellFormedVisitor<'ccx, 'gcx> {
debug!("check_method_receiver: receiver ty = {:?}", rcvr_ty);

let cause = fcx.cause(span, ObligationCauseCode::MethodReceiver);
fcx.demand_eqtype_with_origin(&cause, rcvr_ty, self_arg_ty);
if let Some(mut err) = fcx.demand_eqtype_with_origin(&cause, rcvr_ty, self_arg_ty) {
err.emit();
}
}

fn check_variances_for_type_defn(&self,
Expand Down
26 changes: 26 additions & 0 deletions src/test/ui/mismatched_types/issue-38371.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

struct Foo {
}

fn foo(&foo: Foo) { // illegal syntax
}

fn bar(foo: Foo) { // legal
}

fn qux(foo: &Foo) { // legal
}

fn zar(&foo: &Foo) { // legal
}

fn main() {}
12 changes: 12 additions & 0 deletions src/test/ui/mismatched_types/issue-38371.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0308]: mismatched types
--> $DIR/issue-38371.rs:14:8
|
14 | fn foo(&foo: Foo) { // illegal syntax
| ^^^^ expected struct `Foo`, found reference
|
= note: expected type `Foo`
= note: found type `&_`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have to fix this here necessarily, but I feel like these note: messages are kind of distracting from the help, which is more likely to be of use. But I'm not sure how to fix without doing potentially more harm than good, so let's leave as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a while I've been thinking wether it would make sense to make the expected/found notes a first class comment, so that these errors would look closer to

error[E0308]: mismatched types
  --> ../../src/test/ui/mismatched_types/issue-38371.rs:14:8
   |
14 | fn foo(&foo: Foo) {
   |        ^^^^ expected struct `Foo`, found reference
   |
   = expected type `Foo`
   =    found type `&_`
   = help: did you mean `foo: &Foo`?

error[E0308]: mismatched types
  --> ../../src/test/ui/mismatched_types/issue-38371.rs:26:9
   |
26 | fn agh(&&bar: &u32) {
   |         ^^^^ expected u32, found reference
   |
   = expected type `u32`
   =    found type `&_`

If you feel there'd be value on that, I could create an issue and start a PR for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that looks better

Copy link
Contributor Author

@estebank estebank Jan 7, 2017

Choose a reason for hiding this comment

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

Opened issue #38901 and PR #38902 for this.

= help: did you mean `foo: &Foo`?

error: aborting due to previous error