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

Add note if method is called on a function object #32053

Closed
wants to merge 6 commits into from
Closed

Add note if method is called on a function object #32053

wants to merge 6 commits into from

Conversation

deej-io
Copy link
Contributor

@deej-io deej-io commented Mar 5, 2016

Fixes issue #29124.

If method is called on a function type a note is generated to suggest
that the developer may have forgotten to call the base function.

e.g.

fn main() {
    let mut guess = String::new();
    std::io::stdin.read_line(&mut guess);
}

will generate the note:

note: called method on function type. did you mean `std::io::stdin().read_line(..)`?

Fixes issue #29124.

If method is called on a function type a note is generated to suggest
that the developer may have forgotten to call it.

e.g.

fn main() {
    let mut guess = String::new();
    std::io::stdin.read_line(&mut guess);
}

will generate the note:

note: called method on function type. did you mean `std::io::stdin().read_line(..)`?
@alexcrichton
Copy link
Member

r? @Manishearth

@deej-io
Copy link
Contributor Author

deej-io commented Mar 5, 2016

Hey, this is my first code pull request so any advice to get it up to standard would be great.

@@ -37,6 +37,42 @@ use std::cmp::Ordering;
use super::{MethodError, NoMatchData, CandidateSource, impl_item, trait_item};
use super::probe::Mode;

fn is_fn_ty<'a, 'tcx>(ty: &Ty<'tcx>, fcx: &FnCtxt<'a, 'tcx>, span: Span) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

hooray, DRY!

@Manishearth
Copy link
Member

Mostly looks good, some minor issues.

Also, this needs a test. See https://github.com/Nashenas88/rust/blob/bae1df65aaabf6129636364d86d918c76ed52b6b/src/test/compile-fail/issue-2392.rs for an example of a test for #26305 . You can run a particular test by using TESTNAME=issue-2392 make check-stage1-cfail (or whatever)

Split `fileline_note` into a `file_line note` and `span_suggestion` as per
@Manishearth's suggestions.

Change nested `match`es to `if let`s.
if let Ok (expr_string) = cx.sess.codemap().span_to_snippet(expr.span) {
err.fileline_note(
expr.span,
&format!("{} is a function, perhaps you wish to call it?",
Copy link
Member

Choose a reason for hiding this comment

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

in case the snippet doesn't exist, we should check if it's an ExprKind::Path, and use the last path segment as the function name. No suggestion in this case.

Also, still needs tests, but then it should be good to go

//~^^ NOTE obj::func is a function, perhaps you wish to call it
func.x();
//~^ ERROR no method named `x` found for type `fn() -> ret {func}` in the current scope
//~^^ NOTE func is a function, perhaps you wish to call it
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 some reason these pass without referencing the help note in the comments. The help note does show when this file is manually compiled though.

Copy link
Member

Choose a reason for hiding this comment

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

compiletest only errors for unexpected errors and warnings, if it gets a help that wasn't expected it silently pushes ahead. It only errors for helps when there is an expected help which doesn't match the found help.

Copy link
Member

Choose a reason for hiding this comment

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

You should add //~| SUGGESTION as well (see other instances of this in the tests)

@Manishearth
Copy link
Member

One last nit, otherwise looks good!

@bors
Copy link
Contributor

bors commented Mar 10, 2016

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

@deej-io
Copy link
Contributor Author

deej-io commented Mar 18, 2016

Apologies for the delayed fix. I've been on holiday for the last week.

@bors
Copy link
Contributor

bors commented Mar 19, 2016

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

@@ -1 +1 @@
Subproject commit 57315f7e07d09b6f0341ebbcd50dded6c20d782f
Copy link
Member

Choose a reason for hiding this comment

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

These shouldn't have changed. You may have done a git add * here. I can rebase it and fix it if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes please, Manishearth. If you could rebase it for me that would be great. I'm away with terrible internet and no dev machine at the moment.

@Manishearth
Copy link
Member

r=me post rebase

Manishearth pushed a commit to Manishearth/rust that referenced this pull request Mar 19, 2016
Split `fileline_note` into a `file_line note` and `span_suggestion` as per
@Manishearth's suggestions.

Change nested `match`es to `if let`s.
Manishearth pushed a commit to Manishearth/rust that referenced this pull request Mar 19, 2016
@Manishearth
Copy link
Member

superseded by #32358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants