-
Notifications
You must be signed in to change notification settings - Fork 13k
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 semicolon on double-call that doesn't typecheck as callable #51098
suggest semicolon on double-call that doesn't typecheck as callable #51098
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll write something more in depth later, but what do you think about the feedback?
| ^^^^^^^^^ not a function | ||
| ^^^^^^-^^ | ||
| | | ||
| help: try adding a semicolon: `;` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check wether the spans are in different lines and only suggest then.
error[E0618]: expected function, found `bool` | ||
--> $DIR/issue-51055-missing-semicolon-between-call-and-tuple.rs:14:5 | ||
| | ||
LL | vindictive() //~ ERROR expected function, found `bool` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice if the main span here pointed at only vindictive()
, given that that's where a function was expected (while also pointing at the entire span or the arguments, to make it clear what is happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, although I think this will require a bit of restructuring (need to assert that the expression is an ExprCall
before you can get the callee span).
(regretfully, I'm out of rustc-dev time for at least a few days)
@zackmdavis: Could you give us an update on the status of this PR? If you don't have the time right now to work on this, that is fine, we just like to hear from PR authors every once in a while. |
Regrets: #51149 (comment) I don't think this will make the 1.28 cutoff, but I can reopen later |
@zackmdavis priorities. Deal with whatever is happening to you. Don't hesitate to reach out if there's anything I could do to help. |
@zackmdavis just wanted to know if you could have any time to get back to this. I feel that the changes needed to merge this should be relatively small. |
Oh, right; rust-lang/rustfix#141 (blocking edition-relevant #53013) and #52537 (false-positive I introduced) are higher-priority for me, then this |
@zackmdavis yes, I've been bitten by this too in the past 🤷♀️ |
…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
This provides the desired suggestion for #51055.
The suggestion in changed UI test output for test/ui/block-result/issue-20862 doesn't actually solve that test file's problems, but you can at least see how it's a plausible guess, and arguably less confusing than the "not a function" label in that context.
r? @estebank