no-octal-literal underdelivers

[IllusionMH]

Bug Report

• tslint-microsoft-contrib version: 6.0.0-beta (master branch)
• TSLint version: 5.1.1
• TypeScript version: 3.1.1
• Running TSLint via: CLI

During migation of tests I've noticed some strange behviors in this rule.

TypeScript code being linted

// first case
var x1 = 'Sample text \0111'; // longer than octal

// second case
var x2 = 'Sample text \0';

// third case
var x3 = 'Sample text \777';

// fourth case
var x4 = 0o123;

First case is part of passing test

with tslint.json configuration:

{
    "rules": {
        "no-octal-literals": true
    }
}

Actual behavior

First case: No error
Second case: Octal literals should not be used: \0
Third case: Octal literals should not be used: \777
Fourth case: No error

Expected behavior

First case:
Error because this string actually contains octal escape \011, and 1 is just following character.
And exactly this string will throw in strict mode

(function() {
    'use strict';
    var x1 = 'Sample text \0111'; // longer than octal
})()

Second case:
Should be no error because \0 is allowed in strict mode - it is NULL character (unless followed by decimal character)

Interesting observation

'\09' will throw because of octal escape in Edge & Opera (even if 9 can't be part of octal escape, but still considered octal, details here)

`\09` will throw in Opera, but not in Edge

Third case:
Should be: Octal literals should not be used: \77
Because octal escapes should not be bigger than 0o377 (255) therefore only \77 is escape and 7 is next character. '\777' === '?7'

Fourth case:
There are just no checks for octal literals (a.k.a. octal numeric literals) at all, however rule description is: "Do not use octal literals or escaped octal sequences." and code clearly only implements second part partially (it still allows escapes that will throw in strict mode).

I think that checks for escapes should be strict by default with proper handling of \0, but for numbers I see two options:

1. Add check for octal number literals by default.

2. Add check-numbers option that will enable check on octal literals.

What do you think?

[IllusionMH]

Another problem spotted is errors like Octal literals should not be used: \-035.
Ignoring "literal part" - octal escapes cannot include - character.
In test string return "Sample text \-035"; it would be just escaped - character.

> (function() {'use strict'; return 'Sample text \-035';})()
< "Sample text -035"

> (function() {'use strict'; return 'Sample text \035';})()
< Uncaught SyntaxError: Octal escape sequences are not allowed in strict mode.

I think this should be considered during fixing problems mentioned in original post.

[IllusionMH]

After second and third thoughts I think there is alternative approach:

1. Implement new separate rule no-octal-escapes (as I can see there is no such rule in tslint-eslint-rules but ESLint has one).
2. Mark no-octal-literal as deprecated and remove eventually (not sure if in 7.0 or 8.0).

Reasoning not to check number literals as I suggested earlier:

1. Problematic cases were 644 !== 0644 and 90 === 090.
   However TS already warns about problematic cases (when digits after 0 are 1-7).

error TS1085: Octal literals are not available when targeting ECMAScript 5 and higher. Use the syntax '0o644'.

Not sure if linter rule should go against this recommendation and flag also 0o664

2. 0o644 is explicit and doesn't produce problems. Only readability is a bit hurt in case of 0O644 but in editor with adequate font this shouldn't be a big problem.

Reasoning to implement no-octal-escapes:

1. It is runtime error in strict mode that better to catch earlier.
2. TS has long standing issue with marking octal escapes as errors Octal escape sequences should be a lexical/syntactic error in strict mode and ES5 TypeScript#396
3. Similarity with ESLint

And following question where no-octal-escapes should be implemented. I think that core package is a best place, but can be any of tslint, tslint-microsoft-contrib or tslint-eslint-rules.

[JoshuaKGoldberg]

+1 to pushing more ESLint rules into core, excluding formatting ones. It's a pretty common complaint that they're missing.

So just to confirm @IllusionMH, your suggestion is that once that rule goes into core / tslint-eslint-rules / here, no-octal-literal should be deprecated?

[IllusionMH]

Yes, create new rule in core and deprecate one in this repo.

[JoshuaKGoldberg]

💀 It's time! 💀

TSLint is deprecated and no longer accepting pull requests other than security fixes. See #876. ☠️
We recommend you instead use typescript-eslint to lint your TypeScript code with ESLint. ✅

👋 It was a pleasure open sourcing with you!