in which the E0618 "expected function" diagnostic gets a makeover by zackmdavis · Pull Request #55862 · rust-lang/rust

zackmdavis · 2018-11-11T06:43:14Z

A woman of wisdom once told me, "Better late than never." (Can't reopen the previously-closed pull request from six months ago due to GitHub limitations.)

Now the main span focuses on the erroneous not-a-function callee, while showing the entire call expression is relegated to a secondary span. In the case where the erroneous callee is itself a call, we
point out the definition, and, if the call expression spans multiple lines, tentatively suggest a semicolon (because we suspect that the "outer" call is actually supposed to be a tuple).

The new bug! assertion is, in fact, safe (confirm_builtin_call is only called by check_call, which is only called with a first arg of kind ExprKind::Call in check_expr_kind).

Resolves #51055.

r? @estebank

Now the main span focuses on the erroneous not-a-function callee, while showing the entire call expression is relegated to a secondary span. In the case where the erroneous callee is itself a call, we point out the definition, and, if the call expression spans multiple lines, tentatively suggest a semicolon (because we suspect that the "outer" call is actually supposed to be a tuple). The new `bug!` assertion is, in fact, safe (`confirm_builtin_call` is only called by `check_call`, which is only called with a first arg of kind `ExprKind::Call` in `check_expr_kind`). Resolves rust-lang#51055.

estebank

Overall it looks great to me. I'm intrigued what you think about keeping the "not a function" pointing at the primary span, but I don't know how hard to understand it is when colorized. If it is too terrible, you can r=me.

estebank · 2018-11-11T20:45:05Z

src/test/ui/parse-error-correct.stderr

-   |             ^^^^ not a function
+   |             ^---
+   |             |
+   |             call expression requires function


I'm surprised at the parser taking this as a function call (but it's not problematic).

estebank · 2018-11-11T20:49:15Z

src/test/ui/block-result/issue-20862.stderr

+LL |       let x = foo(5)(2);
+   |               ^^^^^^---
+   |               |
+   |               call expression requires function


I wonder how muddled it'd be if you kept the "not a function" label pointing at the primary span. Even better, detecting chained calls would be nice to provide a custom message (follow up work that needs more thought put into it):

error[E0618]: expected function, found `()` --> $DIR/issue-20862.rs:17:13 | LL | / fn foo(x: i32) { LL | | |y| x + y LL | | //~^ ERROR: mismatched types LL | | } | |_- `foo` defined here returns `()` ... LL | let x = foo(5)(2); | ^^^^^^--- | | | call expression requires function = note: the highlighted code below resolves to `()`... LL | let x = foo(5)(2); | ^^^^^^ = note: ...which cannot be called with the arguments `(2)`

estebank · 2018-11-11T20:49:20Z

src/test/ui/block-result/issue-20862.stderr

+LL | |     |y| x + y
+LL | | //~^ ERROR: mismatched types
+LL | | }
+   | |_- `foo` defined here returns `()`


estebank · 2018-11-11T20:49:43Z

src/test/ui/suggestions/issue-51055-missing-semicolon-between-call-and-tuple.stderr

+   |  _____|
+   | |
+LL | |     (1, 2)
+   | |__________- call expression requires function


zackmdavis · 2018-11-13T05:56:24Z

I wonder how muddled it'd be if you kept the "not a function" label pointing at the primary span.

A little bit too cluttered, in my opinion.

@bors r=estebank

bors · 2018-11-13T05:56:25Z

📌 Commit f3e9b1a has been approved by estebank

@estebank

…you_call_them, r=estebank in which the E0618 "expected function" diagnostic gets a makeover A woman of wisdom once told me, "Better late than never." (Can't reopen the previously-closed pull request from six months ago [due to GitHub limitations](rust-lang#51098 (comment)).) Now the main span focuses on the erroneous not-a-function callee, while showing the entire call expression is relegated to a secondary span. In the case where the erroneous callee is itself a call, we point out the definition, and, if the call expression spans multiple lines, tentatively suggest a semicolon (because we suspect that the "outer" call is actually supposed to be a tuple). ![not_a_fn_1](https://user-images.githubusercontent.com/1076988/48309935-96755000-e538-11e8-9390-02a048abb0c2.png) ![not_a_fn_2](https://user-images.githubusercontent.com/1076988/48309936-98d7aa00-e538-11e8-8b9b-257bc77d6261.png) The new `bug!` assertion is, in fact, safe (`confirm_builtin_call` is only called by `check_call`, which is only called with a first arg of kind `ExprKind::Call` in `check_expr_kind`). Resolves rust-lang#51055. r? @estebank

@ghost

Rollup of 25 pull requests Successful merges: - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets) - #55564 (test/linkage-visibility: Ignore on musl targets) - #55827 (A few tweaks to iterations/collecting) - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI) - #55857 (remove unused dependency) - #55862 (in which the E0618 "expected function" diagnostic gets a makeover) - #55867 (do not panic just because cargo failed) - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef) - #55916 (Make miri value visitor usfeful for mutation) - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code) - #55923 (reword #[test] attribute error on fn items) - #55935 (appveyor: Use VS2017 for all our images) - #55949 (ty: return impl Iterator from Predicate::walk_tys) - #55952 (Update to Clang 7 on CI.) - #55953 (#53488 Refactoring UpvarId) - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors) - #55963 (Stress test for MPSC) - #55968 (Clean up some non-mod-rs stuff.) - #55970 (Miri backtrace improvements) - #56007 (CTFE: dynamically make sure we do not call non-const-fn) - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.) - #56012 (avoid shared ref in UnsafeCell::get) - #56016 (Add VecDeque::resize_with) - #56027 (docs: Add missing backtick in object_safety.rs docs) - #56043 (remove "approx env bounds" if we already know from trait) Failed merges: r? @ghost

Rollup of 25 pull requests Successful merges: - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets) - #55564 (test/linkage-visibility: Ignore on musl targets) - #55827 (A few tweaks to iterations/collecting) - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI) - #55857 (remove unused dependency) - #55862 (in which the E0618 "expected function" diagnostic gets a makeover) - #55867 (do not panic just because cargo failed) - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef) - #55916 (Make miri value visitor useful for mutation) - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code) - #55923 (reword #[test] attribute error on fn items) - #55949 (ty: return impl Iterator from Predicate::walk_tys) - #55952 (Update to Clang 7 on CI.) - #55953 (#53488 Refactoring UpvarId) - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors) - #55963 (Stress test for MPSC) - #55968 (Clean up some non-mod-rs stuff.) - #55970 (Miri backtrace improvements) - #56007 (CTFE: dynamically make sure we do not call non-const-fn) - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.) - #56012 (avoid shared ref in UnsafeCell::get) - #56016 (Add VecDeque::resize_with) - #56027 (docs: Add missing backtick in object_safety.rs docs) - #56043 (remove "approx env bounds" if we already know from trait) - #56059 (Increase `Duration` approximate equal threshold to 1us)

rust-highfive assigned estebank Nov 11, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2018

estebank approved these changes Nov 11, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2018

emilyalbini mentioned this pull request Nov 18, 2018

Rollup of 25 pull requests #56051

Merged

bors merged commit f3e9b1a into rust-lang:master Nov 19, 2018

zackmdavis deleted the but_will_they_come_when_you_call_them branch November 19, 2018 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

in which the E0618 "expected function" diagnostic gets a makeover#55862

in which the E0618 "expected function" diagnostic gets a makeover#55862
bors merged 1 commit intorust-lang:masterfrom
zackmdavis:but_will_they_come_when_you_call_them

zackmdavis commented Nov 11, 2018

Uh oh!

estebank left a comment

Uh oh!

estebank Nov 11, 2018

Uh oh!

estebank Nov 11, 2018

Uh oh!

estebank Nov 11, 2018

Uh oh!

estebank Nov 11, 2018

Uh oh!

zackmdavis commented Nov 13, 2018

Uh oh!

bors commented Nov 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Comments

Conversation

zackmdavis commented Nov 11, 2018

Uh oh!

estebank left a comment

Choose a reason for hiding this comment

Uh oh!

estebank Nov 11, 2018

Choose a reason for hiding this comment

Uh oh!

estebank Nov 11, 2018

Choose a reason for hiding this comment

Uh oh!

estebank Nov 11, 2018

Choose a reason for hiding this comment

Uh oh!

estebank Nov 11, 2018

Choose a reason for hiding this comment

Uh oh!

zackmdavis commented Nov 13, 2018

Uh oh!

bors commented Nov 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants