Fix parameter parsing infinite loop #17354
Conversation
src/compiler/parser.ts
Outdated
| @@ -1881,6 +1881,11 @@ namespace ts { | |||
| if (considerSemicolonAsDelimiter && token() === SyntaxKind.SemicolonToken && !scanner.hasPrecedingLineBreak()) { | |||
| nextToken(); | |||
| } | |||
| else if (isJSDocParameterStart() && parsingContext & (1 << ParsingContext.Parameters)) { | |||
There was a problem hiding this comment.
parseDelimitedList is called in a lot of different places, so it seems wrong to put something in here that would only be used in one particular situation. It also looks like this would be the only place in the parser that we compare parsingContext to a particular value. Could this be moved into the parseElement handler?
src/compiler/parser.ts
Outdated
| token() === SyntaxKind.AtToken || token() === SyntaxKind.ThisKeyword || isJSDocParameterStart(); | ||
| } | ||
|
|
||
| function isJSDocParameterStart(): boolean { |
There was a problem hiding this comment.
It seems weird that isStartOfParameter calls isJSDocParameterStart and not the other way around. One is kind of a superset of the other.
|
@DanielRosenwasser @Andy-MS I think I have a much, much nicer fix now. I realized that a special case from three years ago was, on inspection, avoiding the same bug (but a different specialization of it) - I've made the check very broad now. If we consume no tokens when attempting to parse a parameter (for any reason), then we will consume a token to avoid an infinite loop (and thereby actually yield our parse errors). Additionally, while I was inside |
src/compiler/parser.ts
Outdated
| // contexts. In addition, parameter initializers are semantically disallowed in | ||
| // overload signatures. So parameter initializers are transitively disallowed in | ||
| // ambient contexts. | ||
| const zeroLengthName = getFullWidth(node.name) === 0; |
There was a problem hiding this comment.
We should consider leaving the previous check so that we advance to ? in foo(static?). We can then have a secondary check that advances the token if we haven't consumed anything by capturing scanner.getStartPos() at the top of the function (before line 2217) and comparing it at the bottom of the function to ensure we advance.
weswigham
force-pushed
the
fix-parameter-parsing-infinite-loop
branch
from
9312ad3 to
98d5830
Compare
@sandersn @DanielRosenwasser
We just started parsing jsdoc types in a lot of places in order to better provide errors when they are used outside of jsdoc positions. This caused us to think they we may still be parsing a parameter list when we think we're parsing an arrow function head, when it's really just a parenthesized expression split across two lines. This isn't in and of itself bad, as this will error when we realize its not an arrow function, BUT what was wrong was that nothing was consuming the token used to make the jsdoc-ish type members in these cases, causing us to repeatedly parse (and issue errors for, thereby running out of memory) the same expression, over and over.
With this small change, we simply consume the token appropriately when parsing parameters and have something jsdocish as the token, allowing us to leave the loop and realize our guess at an arrow function is completely wrong.
This fixes the errors caught by these tests (try them in your local checkout of master and be amazed! 7 characters to crash!) and fixes our internal RWC suite OOM failure. 🌞