Octal escape sequences should be a lexical/syntactic error in strict mode and ES5

[JsonFreeman]

The following code should be disallowed in strict mode and ES5:

"\5";
"\05";
"\55";
"\055";

This is disallowed by Annex B.1.2 of the EcmaScript 5 spec.

[alexgorbatchev]

This has become an a problem since 1.8 release. I had ANSI escape codes in my TypeScript files, which were compiled to JS just fine, but 1.8 started adding use strict and all of a sudden node started freaking out.

I think a compiler error is due here if use strict is inserted.

[xogeny]

I don't quite understand the status of this issue? It is still present in 2.1.4. Is the TypeScript leaving this for the community to fix? (if so, fine...I just want to know).

I've run into this a number of times when using @basart's typestyle package to import raw css via template strings. It is very unintuitive that template strings allow embedded octal literals (since the contents of the template string are NOT TypeScript or Javascript). Nevertheless, that is the standard behavior. As such, the TypeScript compiler shouldn't be outputting invalid template strings (i.e., in my opinion the TypeScript compiler should consider an invalidate template string as an error and not emit it). It is particularly nasty when you run this gets passed through both tsc and webpack only to throw an error in the browser. At that point, it is so far downstream it is often difficult for people to trace things back to the original issue.

[sandersn]

Yes, that is what the 'Accepting PRs' label means.

[masaeedu]

@sandersn What's the process for adding new error messages? Just make something up?

[sandersn]

See Instructions for Contributing Code for full details, but here's a summary:

Create a test file in tests/cases/compiler/disallowOctalEscapes.ts that has some octal escapes.

gulp runtests --t=disallowOctalEscapes will make the test "fail" into tests/baselines/local, but the output will not have the error, so don't run gulp baseline-accept yet.

Add the error message in src/compiler/diagnosticMessages.json. I don't think we have a style guide for writing good errors, so base it on the existing examples.

Add code in the compiler to add the error. Rerun the test, check baselines/local/disallowOctalEscapes.error.txt to make sure the error is there, then run gulp baseline-accept.

[NaridaL]

I've started to read the source for parser.ts and scanner.ts to figure out how/where to implement this. So far I've figured out:

The scanner parses "\055" as "\0" + "55", i.e. a null character followed by "55", and "\55" as just "55", so either the scanner needs to be modified or whatever does the checking needs to do the same as emitter.ts and lookup the actual value in the original file source text (also duplicating the scanning effort with a regex for the check). I'm not sure what is preferable.

The actual check is dependent on whether we are in a strict context, so can't be done in scanner.ts directly, am I correct that it belongs in checker.ts? If so, some pointers as to where exactly it should be added would be appreciated.

[mhegazy]

The actual check is dependent on whether we are in a strict context, so can't be done in scanner.ts directly, am I correct that it belongs in checker.ts?

yes. we defer these checks to the binder/checker.

[aslatter]

Would folks be open to a PR which only updates the scanner to correctly interpret octal-escape syntax, but does not go so far as to add warnings about their use? I'm using the text field in the LiteralString nodes, and it would help me to have them be more correct.

Updating the scanner would be required for the warning, so it seems like a step in the right direction.

[mhegazy]

The issue is they are allowed in some contexts, i.e. ES3 and ES5 non-strict mode. so the scanner needs to allow all possible strings, the binder is where the error should be reported.

[aslatter]

@mhegazy Sorry for not being clear - I was more asking if a PR adding full support for octal escape sequences to the lexer would be accepted. Someone else can come in later and add an error to the binder if it makes sense.

[mhegazy]

how would you test the change then?

[aslatter]

I could add a test for #24209 and verify that x as "\1" is assignable to y as "\x01", or something like that. I don't actually care about that type, but I do care about the text field in the AST being correct.

[iansan5653]

Note that octals are disallowed completely (SyntaxError) in template literal strings, whether in strict mode or not. They're technically only deprecated in normal ES5 code. So it should definitely be an error in template literals, and possibly just a warning outside of them.

Also this may be obvious but '\0' is not an octal sequence while all other one, two, or three-digit forms are, ie '\00' and '\1'.

Here's a RegEx that includes all valid octals and excludes all invalid ones with the notable exception of \0: /\\[0-3]?[0-7]{1,2}/g. Try it.

[elibarzilay]

List of things to do for resolving this:

• parser.ts:

  • Not really needed, but there is a comment here that is no longer relevant and should be removed:
    // Octal literals are not allowed...

• scanner.ts:

  • At the end of scanEscapeSequence() should add:

    if (isDigit(ch)) tokenFlags |= TokenFlags.ContainsInvalidEscape;
    
    but...

• types.ts:

  • ContainsInvalidEscape = 1 << 11 is defined here but it's unused!
  • TemplateLiteralLikeFlags = ContainsInvalidEscape ⇒ this is fishy, probably should be its own value

• binder.ts:

  • See checkStrictModeNumericLiteral and make a similar checker for strings with the ContainsInvalidEscape flag.

One decision to make is whether to allow octal escapes in TS when not producing strict-mode code. It would probably be easier to forbid them anyway (especially since they're never allowed in template strings) but if not, then the scanner mode should also parse the octal escapes in case the output is not strict.

[reez12g]

@elibarzilay
Can I work on this issue?
Am I correct in understanding that I should work on the to-do list that you have written?
This is my first time contributing to Typescript and I'm looking forward to working on it!

[orta]

Yeah, just give it a shot and see how it goes 👍🏻

[reez12g]

Thank you! I'll try!

[reez12g]

@elibarzilay @orta
Can I consider a fix to disallow octal escaping even if it is not strict-mode?
Do you have any concerns about this?

(Is this a topic that should be discussed in PR rather than in this Issue?)

[orta]

I'm afraid that would be over-stepping, as they are allowed in those modes: #396 (comment)

[reez12g]

I see, too much.
Thanks for the quick reply!

One decision to make is whether to allow octal escapes in TS when not producing strict-mode code. It would probably be easier to forbid them anyway (especially since they're never allowed in template strings)

#396 (comment)
↑I was just trying to make sure about this.

So, is it the policy that octal escaping is allowed (or left allowed) when not in strict-mode?
I'm sorry if I misunderstood.

[reez12g]

@orta
#396 (comment)
If you could confirm this, it would be greatly appreciated!