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

More helpful note message when confusing a field with a method #26305

Merged
merged 7 commits into from
Jun 20, 2015

Conversation

Nashenas88
Copy link
Contributor

This fixes #2392

I'd like to thank @eddyb for helping me on this one! I wouldn't have gotten the complicated FnOnce check done without his help.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Nashenas88
Copy link
Contributor Author

r? @catamorphism @huonw

// Determine if the field can be used as a function in some way
let fn_once_trait_did = match cx.lang_items.require(FnOnceTraitLangItem) {
Ok(trait_did) => trait_did,
Err(err) => cx.sess.fatal(&err[..])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should fall back gracefully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. I meant to ask about this case before submitting the PR, but I forgot. What should I do here?

Copy link
Member

Choose a reason for hiding this comment

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

Either display the message unconditionally, or only if the type is a function pointer or closure, or never display the message.

cx.sess.span_note(span, &format!("did you mean to write `{0}.{1}`?",
expr_string, item_ustring));
}
};
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 forgot to ask before pushing these changes, is it alright to use a macro here? Is a closure preferred?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say closures are preferred.

@bors
Copy link
Contributor

bors commented Jun 19, 2015

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

@Nashenas88
Copy link
Contributor Author

bump

@huonw
Copy link
Member

huonw commented Jun 20, 2015

I'm unfamiliar with the best ways to drive the type checker for stuff like this, maybe @nikomatsakis or @eddyb are better reviewers.

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned huonw Jun 20, 2015
@Nashenas88
Copy link
Contributor Author

@huonw, @eddyb has already reviewed this commit.

@huonw
Copy link
Member

huonw commented Jun 20, 2015

He can make the final determination then. :)

@eddyb
Copy link
Member

eddyb commented Jun 20, 2015

@bors r+

@bors
Copy link
Contributor

bors commented Jun 20, 2015

📌 Commit bae1df6 has been approved by eddyb

bors added a commit that referenced this pull request Jun 20, 2015
This fixes #2392

I'd like to thank @eddyb for helping me on this one! I wouldn't have gotten the complicated FnOnce check done without his help.
@bors
Copy link
Contributor

bors commented Jun 20, 2015

⌛ Testing commit bae1df6 with merge 7d4d77f...

@bors bors merged commit bae1df6 into rust-lang:master Jun 20, 2015
@Nashenas88 Nashenas88 deleted the field-method-message-2392 branch June 20, 2015 14:45
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.

Helpful error message when confusing a field with a method
5 participants