Skip to content

Shebang error parsing: compatibility with tsc #153

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

Merged
merged 6 commits into from
Jan 7, 2025
Merged

Shebang error parsing: compatibility with tsc #153

merged 6 commits into from
Jan 7, 2025

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