Parser error type identifier

[jbondc]

This improves parsing an invalid type identifier #4067

let d: string | new () => void | number; // Invalid type. To avoid ambiguity, add parentheses: '(new () => void | number)'
let g: x.void; // Identifier expected, reserved word 'void' not allowed here.

Was tricky, added a ParserContextFlags.NoError flag to disable errors on parsing, then check for missing node(s) to improve the error(s).

[DanielRosenwasser]

predicateNode

[DanielRosenwasser]

@jbondc I don't think this is the correct approach for the original bug. As @CyrusNajmabadi pointed out, I think we'd be better off parsing the type and erroring afterwards.

Additionally, I think we should be reporting the specific errors suggested.

[jbondc]

Used this technique to avoid lookAhead() in isStartOfFunctionType() but haven't done any benchmarks. Don't mind any other approach. Surprised you prefer the more verbose errors though.

[jbondc]

Going to reference #3003 to this, having the ability to improve error messages seems valuable too.

[jbondc]

@DanielRosenwasser is performance the primary concern? Don't mind revisiting the approach.

[jbondc]

Another case 😢

[jbondc]

Would be nice to have a more generalized solution to parser error recovery, #5173, #4260

Do think the approach I took was a step in the right direction:
https://github.com/Microsoft/TypeScript/search?q=helpful+error&type=Issues&utf8=%E2%9C%93

[CyrusNajmabadi]

It seems like there should be a much easier fix here. Namely, after parsing out the | just allow parsing any sort of type. Then, later on, when we're type checking, see that you have a "construct signature type" in an innapropriate location (as a "union type element") and give a nice clear error that that's not allowed because of ambiguity, and you should wrap the type with parens.

I've very hesitant about having any sort of parser flags. Esp. around errors.

[jbondc]

@CyrusNajmabadi I'll try something else.

Is there any reason a missing node doesn't have a flag?
https://github.com/Microsoft/TypeScript/pull/4878/files#diff-ead24f0f0f59c0ea9c1c511052e8884bR1027

That seems useful. It could be a single check here:
https://github.com/Microsoft/TypeScript/blob/master/src/compiler/utilities.ts#L186

    export function nodeIsMissing(node: Node) {
        if (!node) {
            return true;
        }

        return node.flags & NodeFlags.Missing;
    }

[CyrusNajmabadi]

Yes. It was very intentional. Such a flag would be redundant with existing information already on the node. Redundant information in a tree is very concerning to me as it now means you have to ensure that they must stay in sync 100% of the time. If you screw that up then you get inconsistencies that make everything difficult.

Such a concern is not hypothetical either. Both the original TS parser and the Roslyn parsers suffered from this problem. They had redundant forms of information that weren't always kept in sync properly. And people used different mechanisms to grok out that information later in the pipeline, leading to all sorts of subtle and annoying bugs. I'd far prefer to have one uniform way to encode if a node is missing, and one way to check for it (through a function which everyone calls).

[jbondc]

@CyrusNajmabadi Refactored using a contextualError, in the long run patch could:
a) speedup parser by avoiding lookAhead/speculationHelper:
https://github.com/jbondc/TypeScript/blob/72337f05db6ce954b07f79fa3ea48ad91c80caf5/src/compiler/parser.ts#L869
b) enable extension points into error handling
jbondc@30ea460#diff-ead24f0f0f59c0ea9c1c511052e8884bR938

Let me know what you think

[DanielRosenwasser]

This is a regression.

[CyrusNajmabadi]

@jbondc I'm not a fan of this approach. As mentioned earlier, I don't believe the flag I appropriate. It's now confusing to users if they should check that flag or use the function to test if a node is missing.

Second, this adds a lot of complexity into the parser. A much simpler solution would be to just make parsing more lenient, and then check for invalid precedence later on during our normal syntactic checks.

[mhegazy]

As @CyrusNajmabadi explained, this is the correct approach. there has not been much activity here either, so closing. please resubmit with the outlined approach.

[jbondc]

k bye