Skip to content

Unexpected "expected function" error with unit type #106515

Closed
@Ezrashaw

Description

@Ezrashaw
Contributor

I tried this code:

fn main() {
    let x = 5

    ()
}

I expected to see this happen:

error: expected `;`, found `}`
 --> src/main.rs:2:14
  |
2 |     let x = 5
  |              ^ help: add `;` here
3 | }
  | - unexpected token

One error occurs from the missing semicolon.

Instead, this happened:

error: expected `;`, found `}`
 --> src/main.rs:4:7
  |
4 |     ()
  |       ^ help: add `;` here
5 | }
  | - unexpected token

error[E0618]: expected function, found `{integer}`
 --> src/main.rs:2:13
  |
2 |       let x = 5
  |  _____________^
3 | |
4 | |     ()
  | |______- call expression requires function

An additional unexpected error occurs when when I obviously do not want to invoke a closure. Also note that the semicolon error is after the supposed closure invocation, when I would expect it to be after the let x = 5.

Meta

Occurs on latest stable and nightly.

Activity

fmease

fmease commented on Jan 6, 2023

@fmease
added
A-diagnosticsArea: Messages for errors, warnings, and lints
D-newcomer-roadblockDiagnostics: Confusing error or lint; hard to understand for new users.
D-terseDiagnostics: An error or lint that doesn't give enough information about the problem at hand.
on Jan 6, 2023
lyming2007

lyming2007 commented on Jan 6, 2023

@lyming2007

The compiler would consider the case

fn main() {
    let x = 5

    ()
}

the same as

fn main() {
    let x = 5()
}

I think it's hard to determine if the user wants invoke a closure or not by writing the "()" to the next line.
Rust allows syntax like this(actually a lot of languages allow this type of syntax)

// Rust example  of a closure
#![allow(unused_variables)]
fn main() {
let closure_variable = |x: i32| x * 10;
  
assert_eq!(100,closure_variable
                    
(10));
}
fmease

fmease commented on Jan 6, 2023

@fmease
Member

I think it's hard to determine if the user wants invoke a closure or not by writing the "()" to the next line.

We should definitely be able to look at the spans of the callee and the argument list, determine if they are on separate lines (via the SourceMap) & don't originate from a macro expansion and if so at least add a note to the diagnostic suggesting to add a semicolon. Of course, there will always be false positives.

estebank

estebank commented on Jan 6, 2023

@estebank
Contributor

The case should be handled in both the parser and the call error. In the parser we can't be certain that the call expression is incorrect, except for maybe literals, but if you had

fn main() {
    let x = 5;
    let y = x
    ()
}

we are back to ambiguity, so that should be handled in E0618 and suggest a semicolon there as well. You could also have

fn main() {
    let x = 5;
    let y = x
    ();
}

which would parse successfully but fail only in the call.

lyming2007

lyming2007 commented on Jan 6, 2023

@lyming2007

@rustbot claim

Noratrieb

Noratrieb commented on Jan 6, 2023

@Noratrieb
Member

When adding a nice parser error you have to make sure that you never emit a parser error for valid rust syntax.
So for the example Esteban described above handling it in the parser is fine since the let statement is missing a semicolon, making it invalid syntax.
But you have to make sure that something like

fn foo() {
  1
  ()
}

doesn't give a parser error but only errors during type checking.

This is because of macros, which may transform code that is valid syntax but invalid semantically into something entirely valid.

estebank

estebank commented on Jan 7, 2023

@estebank
Contributor

At the very least, these cases should be handled

fn a() {
    let x = 5;
    let y = x //~ ERROR expected function
    ()
} //~ ERROR expected `;`, found `}`

fn b() {
    let x = 5;
    let y = x //~ ERROR expected function
    ();
}
fn c() {
    let x = 5;
    x //~ ERROR expected function
    ()
}
fn d() { // ok
    let x = || ();
    x
    ()
}
fn e() { // ok
    let x = || ();
    x
    ();
}
fn f()
 {
    let y = 5 //~ ERROR expected function
    ()
} //~ ERROR expected `;`, found `}`
fn g() {
    5 //~ ERROR expected function
    ();
}
Ezrashaw

Ezrashaw commented on Jan 7, 2023

@Ezrashaw
ContributorAuthor

I guess I should also be clear that the suggestion currently provided by rustc could never be correct: function (and closure) names can't start with integer

fmease

fmease commented on Jan 7, 2023

@fmease
Member

I guess I should also be clear that the suggestion currently provided by rustc could never be correct: function (and closure) names can't start with integer

The diagnostics currently emitted by the compiler are absolutely correct: An integer cannot be called and the let-statement must be followed by a semicolon. Indeed, identifiers (“function (and closure) names”) cannot start with a digit. However, this is not relevant here: Syntactically speaking, the callee (the thing being called) in a call expression can be an arbitrary expression. Consider (T {} + "")() (this might even be well-typed depending on the definition of T and its impls).

So 5() is syntactically well-formed but semantically it is not (according to the trait solver) as there is no primitive integer type (e.g. i32) that implements FnOnce or friends. Technically speaking, core could very well add a FnOnce implementation for i32 that makes 5() semantically well-formed, too.

This is the reason why you see the error messages you posted. Edit: I hope it's already clear from context but I'd like to emphasize that I am not saying that the diagnostics as emitted today should stay the way they are (we should definitely suggest to add a ; as mentioned above), I am just trying to clarify the semantics and disputing the quoted statement.

Ezrashaw

Ezrashaw commented on Jan 7, 2023

@Ezrashaw
ContributorAuthor

Ahh right, I forget briefly that function calls could be on values, I was thinking of the integer literal being seen as an identifier only (and of course integer identifiers don't exist). Silly me!

estebank

estebank commented on Jan 7, 2023

@estebank
Contributor

@Ezrashaw the point you're making is why we can proactively catch the "literal followed by ()" case in the parser, and avoid the second, unnecessary "invalid call" error. But for the general case, where the parser doesn't have enough context (a, b and c in my examples), we would have to emit two errors, or delay the parse error until resolve.

lyming2007

lyming2007 commented on Jan 9, 2023

@lyming2007

@estebank @Ezrashaw
According to the discussion, the current diagnostics emitted by the compiler are correct. Can we close the issue now?

estebank

estebank commented on Jan 9, 2023

@estebank
Contributor

@lyming2007 the compiler errors are correct, but they are misleading, so we'll put some effort to improve the output to make it clear what is happening to everyone, regardless of expertise level, at a glance.

lyming2007

lyming2007 commented on Jan 9, 2023

@lyming2007

ok. So we still let the compiler emit 2 diagnostics, one is from the parser, another one is error[E0618]. How about the output being like this?

error: expected `;`, found `}`
 --> src/main.rs:2:14
  |
2 |     let x = 5
  |              ^ help: add `;` here
3 | }
  | - unexpected token
error[E0618]: expected function, found `{integer}`
 --> src/main.rs:2:13
  |
2 |       let x = 5
  |  _____________^
3 | |
4 | |     ()
  | |______- call expression requires function

Which won't be misleading the user to program code like 5();

Ezrashaw

Ezrashaw commented on Jan 9, 2023

@Ezrashaw
ContributorAuthor

@estebank @lyming2007 Hmm, I haven't really been keeping up to date with this so this might not be ideal/possible but:

  • This parser error is, IMO, wrong. It should say "expected ;, found (". We shouldn't leave out the () here.
error: expected `;`, found `}`
 --> src/main.rs:2:14
  |
2 |     let x = 5
  |              ^ help: add `;` here
3 | }
  | - unexpected token
  • I don't really like the idea of just emitting a "expected function" error. Ideally this would be removed (in this context), or at least a note is added when newlines are detected. The compiler should be able to recognize that the unit type as the last expression in a function is probably not a function call.
fmease

fmease commented on Jan 10, 2023

@fmease
Member

Disclaimer: I might be biased by the phrasing of existing diagnostics. The error message expected ;, found ( looks like it's describing a syntax error, even though it's not (it's a type error) as I've previously mentioned. The wording expected / found [token] is exclusively used by the parser as far as I know. If I see that message I assume that annotating the container with #[cfg(FALSE)] or adding the CLI flag -Zparse-only would not make the error go away. Because of this, I caution against using such phrasing.

I expect it to look similar to:

error[E0618]: expected function, found `{integer}`
 --> src/main.rs:2:13
  |
2 |       let x = 5
  |  _____________^
3 | |
4 | |     ()
  | |______- call expression requires function
  |
help: consider adding a semicolon here:
  |
2 | let x = 5;
  |          +
Ezrashaw

Ezrashaw commented on Jan 10, 2023

@Ezrashaw
ContributorAuthor

@fmease Sorry, I should have been more clear:

error: expected `;`, found `}`
 --> src/main.rs:2:14
  |
2 |     let x = 5
  |              ^ help: add `;` here
3 | }
  | - unexpected token

doesn't really make sense to me, it leaves out the () which is obviously important in this context. EDIT: AFAIK, it still remains in the parser, and it definitely is still a syntax error.

As for your suggested diagnostic, the proposed suggestion doesn't seem quite right in terms of wording. Adding a semicolon does not fix the error; it does not provide a function, but rather makes the error go away by removing the expectation of a function in the first place. This has to be explained to the user, otherwise it just looks even more confusing.

added
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
on Feb 24, 2023
added a commit that references this issue on Aug 8, 2023
8ecb486
added a commit that references this issue on Aug 10, 2023

Rollup merge of rust-lang#114474 - estebank:missing-semi, r=compiler-…

0f79f91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

A-diagnosticsArea: Messages for errors, warnings, and lintsC-bugCategory: This is a bug.D-newcomer-roadblockDiagnostics: Confusing error or lint; hard to understand for new users.D-terseDiagnostics: An error or lint that doesn't give enough information about the problem at hand.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

    Development

    Participants

    @lyming2007@estebank@fmease@Ezrashaw@rustbot

    Issue actions

      Unexpected "expected function" error with unit type · Issue #106515 · rust-lang/rust