Skip to content

Add a note when trying to call a non-function #30001

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

Merged
merged 2 commits into from
Nov 24, 2015
Merged

Conversation

Detegr
Copy link
Contributor

@Detegr Detegr commented Nov 23, 2015

The note added tells the definition location of the non-function
that is being called. Fixes rust-lang#10969
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (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. Due to 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.

@Detegr
Copy link
Contributor Author

Detegr commented Nov 23, 2015

Let's see what the build bot says about this PR. One test failed on my machine even with the master branch of Rust.
So, the error + note now looks following:

test.rs:6:5: 6:10 error: expected function, found `&str`
test.rs:6     foo();
              ^~~~~
test.rs:5:9: 5:12 note: defined here
test.rs:5     let foo="foo";

However, when the non-function is a function argument, the span is a bit funny, as it points to the function argument, but I think it is still quite informative:

test.rs:2:5: 2:8 error: expected function, found `i32`
test.rs:2     i();
              ^~~
test.rs:1:9: 1:10 note: defined here
test.rs:1 fn func(i: i32) {
                  ^

@Manishearth
Copy link
Member

Looks good. Could you add a compile-fail test for this? (tests/compile-fail). Just some code that throws this error, with //~ERROR foo and //~NOTE baz to mark where the notes occur. Use //~^ NOTE in case you want to refer to a note that should occur on the previous line.

Run the test with make check-stage1-cfail TESTNAME=name-of-test-file

@Detegr
Copy link
Contributor Author

Detegr commented Nov 23, 2015

Done. Should I rebase the PR to be a single commit only?

@Manishearth
Copy link
Member

In this case two commits is fine.

But in general, feel free to rebase. git commit --amend is good too.

@Manishearth
Copy link
Member

Do tests pass locally?

if let hir::ExprCall(ref expr, _) = call_expr.node {
let tcx = fcx.tcx();
if let Some(pr) = tcx.def_map.borrow().get(&expr.id) {
if pr.depth == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Good call on the depth check!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Some of the tests caught that one.

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 23, 2015

📌 Commit 210c435 has been approved by Manishearth

@Manishearth
Copy link
Member

Thanks!

bors added a commit that referenced this pull request Nov 24, 2015
@bors bors merged commit 210c435 into rust-lang:master Nov 24, 2015
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