Fix strict/es5+ octal literal 2x error #46810

[jayeclark]

Signed-off-by: Jay Clark jay@jayeclark.dev

Partially fixes #46810

Fixes the conditional in the binder to only issue an error for octal literals in strict mode if the target is before es5. (If es5 or after, this error becomes redundant.) Also fixes the same issue in tsserver.js

Please verify that:

• There is an associated issue in the Backlog milestone (required)
• Code is up-to-date with the main branch
• You've successfully run gulp runtests locally
• There are new or updated unit tests validating the change

[sandersn]

I like this fix. It's the smallest possible code change. So even though it doesn't fully fix #46810 or address the questionable usefulness of checkGrammarNumericLiteral in the checker, I think it's the most pragmatic change to make.

All you need to do is follow the testing instructions in CONTRIBUTING to update the tests. In a nutshell, gulp runtests-parallel then gulp baseline-accept and push a new commit containing the changed files.

[jayeclark]

Great, thanks so much for the quick feedback! The testing is a bit harder for me (I’m still very junior), so I just wanted to check my fix wasn’t completely off base before I dove into it. :)

[typescript-bot]

It looks like you've sent a pull request to update our 'lib' files. These files aren't meant to be edited by hand, as they consist of last-known good states of the compiler and are generated from 'src/lib' or possibly our lib generator. Unless this is necessary, consider closing the pull request and sending a separate PR to update 'src/lib' or https://github.com/microsoft/TypeScript-DOM-lib-generator

[ghost]

All CLA requirements met.

[jayeclark]

Lost a few days being under the weather from getting my COVID booster, but I think I've finally got the tests sorted. I also updated the logic in tsserver.js so that there would be consistency there, but I see the PR has been flagged because of that. It doesn't seem like that's a file to which the warning applies (and unless the logic is changed in it, tsserver will continue throwing two errors for octal literals in strict mode in ES5+).

[RyanCavanaugh]

Updates to lib/ should only happen as part of a separate LKG update; please revert this file

[jayeclark]

@RyanCavanaugh Sorry about that, reverting now!

[sandersn]

The only existing tests that change are of plain JS files. Can you add a TS test, based on the example in the bug? Probably the easiest place to put the new test is in tests/cases/compiler/someNameHere.ts.

[jayeclark]

Sure thing, sorry for the oversight!

[jayeclark]

Closing this out as I've realized I'm still a bit too junior to complete this at the level required. Will try again in a few months when I'm further along in learning CS.

[sandersn]

This is a good fix. Do you mind if I finish it by adding a test + accepting baselines? If you want to finish it yourself in a few months that's fine too.

[jayeclark]

@sandersn That would be fine. I meant that I'd try my hand at solving a different issue. I'm stuck on creating the TS test and need to learn the language, repo, and test strategy better before I'd be able to complete it (which realistically I wouldn't be able to spend time on until I'm done with my initial CS coursework in January.)

[jayeclark]

@sandersn Thank you so much for your help in closing this out. The added test may have only been a few lines of code, but I really could not wrap my head around how the test suite works in this repo (I've only really worked with jest) and would probably not have gotten there for quite some time.

Just so I understand it correctly for the next time I take a stab at an issue here, when adding a TS test all that's needed is to write the code snippet that will be evaluated. When I run the tests, the testing suite will create a .txt file of the errors that are thrown. Presumably these errors will differ from the baselines because something in the Typescript code has been changed/corrected. If the errors look correct, I then run the command to accept the new baselines.

TLDR: There is no need to code any assertions into the test regarding what errors are expected (this is where I kept getting hung up), instead the code throws & records the errors in the testing process and the developer evaluates that the errors are correct and adds them to the baselines.

[sandersn]

You got it exactly right. Our system is quite different from unit tests since the assertions are implicit in the baseline output. That means there are a lot of assertions — even ones that the author didn't intentionally put there.