Skip to content

Conversation

sandersn
Copy link
Member

In tsc, #! anywhere except pos=0 in a file gives an error and increments past #. In tsgo, it was incrementing past #!. tsc's behaviour seems bad, but not bad enough to differ from, especially since the parse tree will be wrong either way.

In tsc, `#!` anywhere except pos=0 in a file gives an error and increments
past `#`. In tsgo, it was incrementing past `#!`. tsc's behaviour seems
bad, but not bad enough to differ from, especially since the parse tree
will be wrong either way.
@jakebailey
Copy link
Member

The original code was:

case CharacterCodes.hash:
    if (pos !== 0 && text[pos + 1] === "!") {
        error(Diagnostics.can_only_be_used_at_the_start_of_a_file, pos, 2);
        pos++;
        return token = SyntaxKind.Unknown;
     }

What is all of that logic above the code you're modifying?

@sandersn
Copy link
Member Author

The original code checked for valid #! separately, at the beginning of the function. Since shebang is rare, and # is rare, it makes more sense to move and inline the code for the valid case. So in tsgo it sits right next to the invalid case.

(This is speculation, maybe @ahejlsberg had some other reason for inlining it.)

@DanielRosenwasser
Copy link
Member

It never made sense to check specifically at the beginning of the function if you want to give a good error message anyway. I did the same thing at microsoft/TypeScript#59244

@DanielRosenwasser
Copy link
Member

Is it just me or does that inlined code not work for a shebang terminated by any newline character other than \n?

@sandersn
Copy link
Member Author

Shebang only works in Unix doncha know 🙃
In JS, it's a regex, so whatever a regex considers a newline. I'll switch it to stringutils.IsLineBreak.

@sandersn sandersn merged commit 74f6f0c into microsoft:main Jan 7, 2025
12 checks passed
@sandersn sandersn deleted the shebang-error-parse-bug-compatibility branch January 7, 2025 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants