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: keep parsing after error limit is reached? #29992

Open
stamblerre opened this issue Jan 30, 2019 · 12 comments
Open

go/parser: keep parsing after error limit is reached? #29992

stamblerre opened this issue Jan 30, 2019 · 12 comments
Assignees

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jan 30, 2019

This issue and repro case was raised by @rogpeppe:

This file fails to parse:

package m

func _()  {
	_(_
}

func _() {
	if _ {
		_
	}
	if _ {
	}
	_ = _
	if _ {
		_
	}
}

However, commenting out one line (unrelated to the error) causes the file to parse correctly.

package m

func _()  {
	_(_
}

func _() {
	if _ {
		_
	}
	if _ {
	}
	// _ = _
	if _ {
		_
	}
}
@stamblerre stamblerre self-assigned this Jan 30, 2019
@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Jan 30, 2019

FWIW you can delete any element of the second function and it will cause the file to parse OK.
Looking at the ast trace might be informative.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 30, 2019

CC @griesemer for go/parser

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 30, 2019

commenting out one line (unrelated to the error) causes the file to parse correctly.

Note that there is no “correct” parse for either file. It would certainly be nice to generate a robust parse tree anyway, but it would be helpful to be precise about which error is reported and which error you would prefer.

Playground links illustrating the behavior are ideal.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 30, 2019

I can't reproduce surprising behavior for these inputs: the two parse trees seem very nearly equivalent.

https://play.golang.org/p/oJdvPze37lP

@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Jan 30, 2019

FWIW this was the playground link I sent to @stamblerre: https://play.golang.org/p/8kA21r8jXzl

@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Jan 30, 2019

Ah, the bad behaviour only happens if you don't specify AllErrors in the call to ParseFile.

I didn't expect AllErrors to change the parse result - I thought it was just about error reporting...

@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Jan 30, 2019

It's perhaps interesting to note that this means that the introduction of AllErrors in https://golang.org/cl/7307085 was technically a breaking change.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jan 30, 2019

/cc @matloob, who authored that linked CL

@bcmills bcmills changed the title go/parser: failures on incorrect code in unexpected ways go/parser: keep parsing after error limit is reached? Jan 30, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 30, 2019

Ah, the bad behaviour only happens if you don't specify AllErrors in the call to ParseFile.

That CL appears to have been the fix for #3943, which reported high memory use and long running time for collecting errors. I wonder whether there is a related problem for aggregating invalid parse trees: I've noticed that, for example, some Emacs modes start to hang on syntax-highlighting if you introduce a small missing token near the beginning of a large file. It would be a shame if Go tools had the same sort of hangs.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Jan 30, 2019

FWIW the version of go/parser that godef used until recently was forked before that CL (it always gets all errors) and I never observed that kind of issue.

An interesting ramification of this problem: I changed godef to add the AllErrors flag, but I can't add a test because the golang.org/x/tools/go/expect package used for godef's testing doesn't use AllErrors so it ignores the test comments...

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 30, 2019

I can't add a test because the golang.org/x/tools/go/expect package used for godef's testing doesn't use AllErrors so it ignores the test comments...

Talk to @ianthehat? That seems eminently fixable.

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Jan 30, 2019

The parser will stop after 10 errors if AllErrors is not provided. That's the whole point. We don't want to change that. (One might argue that the default is wrong, but it's too late to change that.)

If anything I see at best documentation issue here for the go/parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.