Fix hashbang checks not using all line terminator chars

[CanadaHonk]

Should make 2 more test262 hashbang tests pass.

[gbrail]

Can you please "./gradlew spotlessApply" this PR so that it fixes the formatting so that the CI tests work? This will probably work best on Java 8.

If you want to use Java 11 for that instead, then if you pull "master" and merge the changes there, it should work with that version too. (Making "spotless" work with later Java versions will take more changes that'll have to happen later.)

[gbrail]

Since this is a simple fix and I'm aligned with making things better, I'm going to fix it in a different PR. Stay tuned!

[CanadaHonk]

Sorry, I completely missed the previous comment. It turned out this isn't for hashbangs and only the shell, so the entire logic should be moved into the general parser probably?

[gbrail]

Yes -- if you'd like to fix "spotless" and find a way to make the check more generic, I'm happy to wait! I'm honestly not sure where we check for hashbangs in the main parser, but it shouldn't be too hard to find. As it is this change didn't affect any tests (if it had caused additional test262 tests to pass then the build would have failed so we can update the test properties file).

[p-bakker]

I've done a hashbang implementation for Rhino a long time ago when the proposal was still in stage 3, by adding the below code here in the TokenStream class:

                case '#': // #! hashbang: only on the first line of a Script, no leading whitespace
                    if (cursor == 0 && peekChar() == '!') {
                        skipLine();
                        return Token.COMMENT;
                    }

Now, as it has been a while I don't remember the specifics, but I had a TODO on my implementation that said that the hashbang logic should only go into effect if the 'parse goal is Script and not function'

Maybe the above helps in implementing this in a more generic fashion, so it also works outside of the shell

[gbrail]

I don't see any reason to hold this particular fix up for much longer -- it's a perfectly reasonable thing to fix regardless and I'm happy to fix the formatting problem and merge it.

Are there particular test262 tests that we are trying to fix, though? I don't see anything changing when I run the tests, and our test suite would let me know if previously-excluded tests were passing. I just want to make sure that we're fixing a thing that needs to be fixed...

[CanadaHonk]

Sorry for my inactivity, it should make 2 more pass but not sure if there's something else at play. What was said before is probably the best approach, I'll likely close this and hopefully make a different PR for generally rewriting the hashbang logic into the main parser instead of the shell.

[p-bakker]

Also see the feedback in the accompanying issue: this should not be implemented in the shell, but in the parser: #1274 (comment)

[p-bakker]

@CanadaHonk think you'll be able to provide a revised PR?

[p-bakker]

Added PR #1417 to implement hashbang in the Parser, as per #1341 (comment)

[CanadaHonk]

Apologies for inactivity again, I'll close this out then. Thank you!