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

go/parser: poor error recovery after missing function call argument #58833

Open
adonovan opened this issue Mar 2, 2023 · 5 comments
Open

go/parser: poor error recovery after missing function call argument #58833

adonovan opened this issue Mar 2, 2023 · 5 comments
Labels
gopls/parsing Issues related to parsing / poor parser recovery. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Mar 2, 2023

This program contains a syntax error in Abs(,) that causes the parser's recovery to generate a BadExpr for math.Lde.

func g() {
        math.Abs(,)

        //

        math.Lde()
}

This causes go/types not to recognize math as a reference to a package, and causes completions in gopls to misbehave.

Screenshot 2023-03-02 at 1 06 40 PM

I think the parser could do a better job of recovery. @griesemer gives reasons why this is difficult in #24327 (comment):

go/parser is likely falling behind the compiler due to the compiler's parser (package syntax) being actively improved whenever we run into unsatisfying error messages. It's fairly time-consuming to back-port those fixes because the parsers don't have the same structure, and small local changes sometimes have unexpected consequences with respect to error handling (hence those changes, even if small, are time-consuming to get right).

Ideally, in the long run we should migrate to package syntax. Failing that, one option might be to replace go/parser with the syntax package parser while keeping the same API. That might be not too hard since the syntax trees are reasonably close.

Related issue of gopls completion after a syntax error:

@griesemer
Copy link
Contributor

This specific error is perhaps not too hard to fix individually.

Re-using the compiler's syntax package's parser code, but producing go/ast may be a good solution in the long run. Perhaps parts of it can be copied in pieces. E.g., the parts the parse parameter lists could be copied and adjusted to produce go/ast, and as a "side-effect" this bug might get fixed.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 2, 2023
@dmitshur dmitshur added this to the Backlog milestone Mar 2, 2023
@adonovan
Copy link
Member Author

adonovan commented May 3, 2023

Another version of the same recovery cascade:

package main

import "fmt"

func nop(...any){}

func main() {
        nop(1, 2, 3 fmt.Println) // notice missing comma after 3

        nop()
        nop()
        nop()

        var a, b, c int
        println(a, b, c)
}

Errors:
play.go:8:21: missing ',' in argument list
play.go:8:24: expected operand, found '.'
play.go:14:9: missing ',' in argument list
play.go:14:21: missing ',' in argument list
play.go:14:24: expected operand, found newline
play.go:16:3: expected ')', found 'EOF'
play.go:16:3: expected ';', found 'EOF'
play.go:16:3: expected ';', found 'EOF'
play.go:16:3: expected '}', found 'EOF'
play.go:16:3: missing ',' in argument list
play.go:14:13: undeclared name: a
play.go:14:16: undeclared name: b
play.go:14:19: undeclared name: c
play.go:3:8: "fmt" imported but not used

FWIW, pretty printing the damaged tree produces only a small part of it:

func main() {
    nop(1, 2, 3, BadExpr,

        a, b, c, BadExpr,
    )
}

@griesemer
Copy link
Contributor

For reference, for the above program, the compiler produces simply

foo.go:8:21: syntax error: unexpected fmt in argument list; possibly missing comma or )

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/493616 mentions this issue: gopls/internal/lsp/cache: skip type errors after parse errors

gopherbot pushed a commit to golang/tools that referenced this issue May 9, 2023
Parser error recovery can delete large swathes of source code;
see golang/go#58833 for examples. Type checking syntax trees
containing syntax errors may therefore result in a large number
of spurious type errors. So, this change suppressed type errors
in the presence of syntax errors.

Fiddling with these tests is really surprisingly time consuming.

Fixes golang/go#59888

Change-Id: Ib489ecf46652c5a346d9caad89fd059434c620f8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/493616
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
@adonovan adonovan added the gopls/parsing Issues related to parsing / poor parser recovery. label Jul 25, 2023
@adonovan
Copy link
Member Author

adonovan commented Aug 27, 2024

Here's a variant distilled from #68918 in which the parser swallowed (originally) thousands of lines during recovery:

package p

func f() {
	h(a b, 

	case c:
		if e.Len != nil {
		} else {
		}
	}
}

func longfunc() {
   //...
}

func g() {
	if stmt.Tok == token.DEFINE {
	}
}

Using the types playground, this parse tree is pretty printed as:

package p

func f() {
h(a, BadExpr,

e.Len != nil{}, BadExpr,

//...

stmt.Tok == token.DEFINE{},
BadExpr)
}

Observe that recovery consumes the if of various statements of the form if x == y {, changing the parser's disposition towards the { so that it becomes a composite literal y{...}. There are two examples above: nil{...} and token.DEFINE{...}. It also sucks in and discards a potentially long function in the middle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls/parsing Issues related to parsing / poor parser recovery. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants