-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix loop in parser #15421
Fix loop in parser #15421
Conversation
I noted a case in Metals where the compiler would keep running at 100+ % until the process was killed. Using jstack I tracked it down to an infinite `skip` caused by a `syntaxError` in `pattern3`. In fact, the syntaxError should not skip at this point since the offending expression was already fully parsed. I fixed this in this commit. The parser was invoked from the `signatureHelp` method. It seems it parsed something that was not syntactically correct (specifically, a postfix `*` appeared in a pattern where none was allowed). `
The fix is actually not sufficient. I just found another infinite loop in the Parser, this time here:
It's again called from def atStop =
token == EOF
|| (currentRegion eq lastRegion)
&& (isStatSep
|| closingParens.contains(token) && lastRegion.toList.exists(_.closedBy == token)
|| token == COMMA && lastRegion.toList.exists(_.commasExpected)
|| token == OUTDENT && indentWidth(offset) < lastKnownIndentWidth)
// stop at OUTDENT if the new indentwidth is smaller than the indent width of
// currentRegion. This corrects for the problem that sometimes we don't see an INDENT
// when skipping and therefore might erroneously end up syncing on a nested OUTDENT.
if debugTokenStream then
println(s"\nSTART SKIP AT ${sourcePos().line + 1}, $this in $currentRegion")
while !atStop do
nextToken() It looks like something is wrong with the way the virtual file passed to the parser is set up that makes |
Another thing to try would be to make indent syntax be false for signature helps. (I assume it's not needed). That would prevent spurious insertions of indent and outdent. You can achieve this by passing a context with |
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.
Thanks for looking into this! This might have caused the issue with the stuck compiler
@tgodzik I've hit this issue several times in the past week using Metals 0.11.6 with projects that are on Scala 3.1. Is there a corresponding workaround on the Metals side for those codebases not on 3.2? (since this looping bug in parser seems to be longstanding) |
oh this looks so bad |
I am trying to fix it for older versions in https://github.com/scalameta/metals/pull/4074/files and it seems to work, but I am not sure if it should. Does anyone knows how to get an infinitly compiling code? Would be cool to test out cancelation in that case. |
I noted a case in Metals where the compiler would keep running at 100+ % until the process was killed.
Using jstack I tracked it down to an infinite
skip
caused by asyntaxError
inpattern3
. In fact,the syntaxError should not skip at this point since the offending expression was already fully parsed.
I fixed this in this commit.
The parser was invoked from the
signatureHelp
method. It seems it parsed something that was notsyntactically correct (specifically, a postfix
*
appeared in a pattern where none was allowed).`