Allow comments in tsconfig.json

[sarod]

This pull requests allows comments in tsconfig.json #4987
Comments can be single line comments using // or multiline comments using / * * /

[msftclas]

Hi @sarod, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[msftclas]

@sarod, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

[mhegazy]

use isLineBreak instead.

[weswigham]

VSCode only allows // and /**/ comments in its json parser - we should do the same (or it won't be able to validate our tsconfigs).

[weswigham]

This doesn't technically capture all escape patterns - specifically, this ignores the unicode escape sequences of the form \u0000. It probably doesn't matter for just parsing out comments - in fact, we really only care if we see \", so we know to continue processing the string.

[sarod]

I think it is functionally equivalent to handle this the way it is without dealing with \u special case.
However have to at least handle \ in addition to " in order to handle the second assertion in "it("handles escaped characters in strings correctly")".

[DanielRosenwasser]

I don't think this is necessarily the right approach. Why are we building a scanner to handle a JSON file instead of leveraging the existing scanner and writing a resilient JSON parser?

[DanielRosenwasser]

This implies you're going to be concatenating a string for every character in your tsconfig.json. You should instead concatenate the longest sequences of non-comment substrings.

For an example of where we do this, check out scanString in scanner.ts. We just loop on an index and check the character code at every index. When we hit something like a backslash, we just concatenate a range:

function scanString(): string {
    let quote = text.charCodeAt(pos++);
    let result = "";
    let start = pos;
    while (true) {
        if (pos >= end) {
            result += text.substring(start, pos);
            tokenIsUnterminated = true;
            error(Diagnostics.Unterminated_string_literal);
            break;
        }
        let ch = text.charCodeAt(pos);
        if (ch === quote) {
            result += text.substring(start, pos);
            pos++;
            break;
        }
        if (ch === CharacterCodes.backslash) {
            result += text.substring(start, pos);
            result += scanEscapeSequence();
            start = pos;
            continue;
        }
        if (isLineBreak(ch)) {
            result += text.substring(start, pos);
            tokenIsUnterminated = true;
            error(Diagnostics.Unterminated_string_literal);
            break;
        }
        pos++;
    }
    return result;
}

[sarod]

@DanielRosenwasser What do you mean exactly by

I don't think this is necessarily the right approach. Why are we building a scanner to handle a JSON file instead of leveraging the existing scanner and writing a resilient JSON parser?

Are you talking about using elements of typescript scanner instead of using comment removal than JSON.parse?

If that's the case could you please clarify what direction I should follow between:

1. Restarting from scratch using the typescript scanner
2. Applying other suggestions on top of my current PR

[mhegazy]

One nice thing about using the TS parser/scanner, is you can be much more forgiving with invalid input, and you can give helpful messages too.

[DanielRosenwasser]

I think it would be a lot better to create a custom parser, however, if you don't feel entirely comfortable with that, I think the current approach isn't too bad. Still, I think the long-term vision is to use our scanner to parse things out.

[vladima]

I don't think we should be putting yet another scanner in our codebase unless we have a very compelling reason. Given that implementation that uses existing scanner can be as simple as code below I would vote to reuse code that we already have

    function stripComments(text: string): string {
        let output = "";
        let scanner = createScanner(ScriptTarget.ES5, /* skipTrivia */ false, LanguageVariant.Standard, text);
        let token: SyntaxKind;
        let newLine: string;
        while((token = scanner.scan()) !== SyntaxKind.EndOfFileToken) {
            switch(token) {
                case SyntaxKind.SingleLineCommentTrivia:
                case SyntaxKind.MultiLineCommentTrivia:
                    break;
                default:
                    output += scanner.getTokenText();
                    break;
            }
        }
        return output;
    }

[DanielRosenwasser]

👍 for Vlad's solution. You can still have the same behavior where you pad any trivia with spaces the same way you did.

[sarod]

OK. I'll give a try to Vlad's solution.

[DanielRosenwasser]

There's a tab on this line

[DanielRosenwasser]

Can you switch this file to use 4 spaces per indentation?

[vladima]

👍

[mhegazy]

Thanks @sarod!

[sarod]

Thanks for the guidance and suggestions.