fix(396): (Dis)allow Octal and Bad Escape Sequences in String and (Tagged) Template Literals #51837
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Bad escape sequences are now preserved by returning the raw string instead of |
graphemecluster
changed the title
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at d1ba506. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the parallelized Definitely Typed test suite on this PR at d1ba506. You can monitor the build here. |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite (tsserver) on this PR at d1ba506. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite on this PR at d1ba506. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite on this PR at d1ba506. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite (tsserver) on this PR at d1ba506. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at d1ba506. You can monitor the build here. |
|
@RyanCavanaugh Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
|
@RyanCavanaugh Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @RyanCavanaugh, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@RyanCavanaugh Here they are:
CompilerComparison Report - main..51837
System
Hosts
Scenarios
TSServerComparison Report - main..51837
System
Hosts
Scenarios
StartupComparison Report - main..51837
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Everything looks good! |
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsbackstage/backstage
|
tests/baselines/reference/octalLiteralInStrictModeES3.errors.txt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I'm not sure if this PR is the right place to discuss this - but as we consider deprecating ES3 targets, and as I've reviewed more and more parsing issues in TypeScript that make decisions based on the language target, it feels like we should start reconsidering our approach.
Language target is really not the same as the parsing language version; and we're not always emitting code either. I wonder if there are opportunities to simplify.
|
@DanielRosenwasser The point is whether the code is in strict mode, not the target. We now have two options:
In either way I am going to root out Edit: I've implemented the first approach. Please give it a review. |
graphemecluster
commented
Dec 14, 2022
tests/cases/conformance/expressions/objectLiterals/objectLiteralErrors.ts
Show resolved
Hide resolved
graphemecluster
force-pushed
the
escape-sequence
branch
from
e8b6e50 to
0f211b7
Compare
|
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at 251e536. You can monitor the build here. Update: The results are in! |
|
Hey @gabritto, the results of running the DT tests are ready. Branch only errors:Package: vk-openapi |
|
That DT failure is weird, since the line before has |
|
Are parse errors |
|
Looks like not, eg Edit: So it's not a problem unique to this change; I think this PR is ready to go then. |
sandersn
approved these changes
Mar 14, 2023
sandersn
added a commit
to DefinitelyTyped/DefinitelyTyped
that referenced
this pull request
Mar 14, 2023
Old-style octal literals become syntax errors in an upcoming TS PR: microsoft/TypeScript#51837 But syntax errors can't be silenced with ts-expect-error, and the octal literal is not important to the vk-openapi test in question, so change it to use new octal syntax.
|
Fix the DT test's throwaway reference from 007 to 0o7: DefinitelyTyped/DefinitelyTyped#64741 |
sandersn
added a commit
to DefinitelyTyped/DefinitelyTyped
that referenced
this pull request
Mar 14, 2023
Old-style octal literals become syntax errors in an upcoming TS PR: microsoft/TypeScript#51837 But syntax errors can't be silenced with ts-expect-error, and the octal literal is not important to the vk-openapi test in question, so change it to use new octal syntax.
jakebailey
previously requested changes
Mar 17, 2023
jakebailey
approved these changes
Mar 17, 2023
|
@DanielRosenwasser Your review is the only red one; can you take another look to see if you see anything else? |
|
So, this looks good to me, but I'm still worried about people using our parser on previously-valid code and now getting parse errors. I'd be interested to test out some users of our public API like ts-loader or rollup-typescript-plugin to see what happens. |
DanielRosenwasser
approved these changes
Mar 24, 2023
|
Thank you @graphemecluster! |
DanielRosenwasser
added
the
Breaking Change
alex-sonar
pushed a commit
to SonarSource/SonarJS
that referenced
this pull request
Jun 7, 2023
This was referenced Jun 22, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#23801 makes bad escape sequences in tagged template allowable, but only in certain cases. #41030 aims to fix this but was closed.
In contrast, octal escape sequences in string literals (as in #396) should be disallowed.
Fully fixes #12700
Fixes #396
Fixes #39038
Fixes #39715
Fixes #42887
Fixes #39563 (#39698 attempts to fix this but was closed)
This PR currently disallow octal literals and escape sequences regardless of the target, strict mode or not as the Team is looking for simplicity (Refer to #396 (comment)).