Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for Lift Template Literal Restriction #23801

Merged
merged 34 commits into from
Feb 5, 2020

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented May 1, 2018

Fixes #12700

@@ -428,6 +429,16 @@ namespace ts {
return ch >= CharacterCodes._0 && ch <= CharacterCodes._7;
}

/* @internal */
export function isHexDigit(ch: number): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither of these are used outside of scanner.ts, so just don't export them.

@@ -27,7 +27,8 @@ namespace ts {
getTokenFlags(): TokenFlags;
reScanGreaterToken(): SyntaxKind;
reScanSlashToken(): SyntaxKind;
reScanTemplateToken(): SyntaxKind;
reScanTemplateToken(isTaggedTemplate?: boolean): SyntaxKind;
reScanTemplateHead(): SyntaxKind;
Copy link
Member

@DanielRosenwasser DanielRosenwasser May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhegazy, thoughts on just having reScanTemplateToken operate on template heads as well? The implementation could have a Debug.assert like so:

if (token !== SyntaxKind.TemplateHead) {
    Debug.assert(isTaggedTemplate, "'reScanTemplateToken' should only be called with a template head when in a tagged template.");
}
else {
    Debug.assert(token === SyntaxKind.CloseBraceToken, "'reScanTemplateToken' should only be called on a '}' or TemplateHead.");
}

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked too closely at the logic yet, but the general mechanism between the parser and the scanner looks close to what I would've done, so that's good!

I think what we also need is tests for es5 and es2015. One thing we need to discuss is whether the es2015 target needs the downleveled version.

@@ -1606,16 +1607,22 @@ namespace ts {
export interface TemplateHead extends LiteralLikeNode {
kind: SyntaxKind.TemplateHead;
parent?: TemplateExpression;
/* @internal */
notEscapeFlags?: TokenFlags;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider just calling these templateFlags.

@@ -1593,6 +1593,7 @@ namespace ts {
BinarySpecifier = 1 << 7, // e.g. `0b0110010000000000`
OctalSpecifier = 1 << 8, // e.g. `0o777`
ContainsSeparator = 1 << 9, // e.g. `0b1100_0101`
NotEscape = 1 << 10, // e.g. `\uhello`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContainsInvalidEscape

@Kingwl
Copy link
Contributor Author

Kingwl commented May 2, 2018

One thing we need to discuss is whether the es2015 target needs the downleveled version.

In my memory, there are several issues related to escape sequences (es2015) that can be fixed together

@Kingwl
Copy link
Contributor Author

Kingwl commented May 2, 2018

wow, seems ci is upgrade😄

@@ -2104,17 +2108,24 @@ namespace ts {
return allowIdentifierNames ? parseIdentifierName() : parseIdentifier();
}

function parseTemplateExpression(): TemplateExpression {
function parseNoSubstitutionTemplate(isTaggedTemplate?: boolean) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no public API, just make this parameter required.

@@ -0,0 +1,47 @@
tests/cases/conformance/es2018/invalidTaggedTemplateEscapeSequences.ts(5,18): error TS1125: Hexadecimal digit expected.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to figure out exactly how to prevent the errors from the initial scan...

@Kingwl
Copy link
Contributor Author

Kingwl commented May 7, 2018

should this move to es2018 transform?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 7, 2018

should this move to es2018 transform?

I would say yes, but only run conditionally. So you only use the ES5-style transformed syntax when an "invalid escape" template string is used, but leave it alone otherwise in the ES2018 transform.

@DanielRosenwasser
Copy link
Member

Also, I'm not sure if you want to share that logic between each transform, or to just do the ES2015 transform in ES2018. @rbuckton might have a better idea here.

@rbuckton
Copy link
Member

rbuckton commented May 9, 2018

Also, I'm not sure if you want to share that logic between each transform, or to just do the ES2015 transform in ES2018.

I think the best approach would be to do something similar to what we do for destructuring and pull the logic for template literal down-leveling out of both places. You can then have the esnext.ts transform call this separate API for the tagged templates that need it, and the es2015.ts transform can call it for all tagged templates.

@rbuckton
Copy link
Member

rbuckton commented May 9, 2018

should this move to es2018 transform?

Not the ES2018 transform, since this is not part of ES2018. Rather, this should go in the esnext.ts transform.

@Kingwl
Copy link
Contributor Author

Kingwl commented May 10, 2018

is that mean i should add a new transformer, eg: taggedTemplate.ts and export the parse function, call the function in es2015 or esnext transformer?

@mhegazy
Copy link
Contributor

mhegazy commented May 23, 2018

is that mean i should add a new transformer, eg: taggedTemplate.ts and export the parse function, call the function in es2015 or esnext transformer?

we already have esnext transform. i would put it there..

@Kingwl Kingwl force-pushed the Lift-Template-Literal-Restriction branch from e92b6bc to 2f49205 Compare May 30, 2018 09:50
@Kingwl
Copy link
Contributor Author

Kingwl commented Jun 6, 2018

@Kingwl Kingwl mentioned this pull request Jun 6, 2018
3 tasks
@DanielRosenwasser
Copy link
Member

Sorry, I didn't have full context as I was reviewing this. Please ignore the last batch of comments.

@Kingwl
Copy link
Contributor Author

Kingwl commented Jun 8, 2018

@DanielRosenwasser there are a other commit that i miss
the commit split the transform logic and call them in different transformter
that commit is in my another pc, i'll push it after i finish my work 😢

@Kingwl Kingwl force-pushed the Lift-Template-Literal-Restriction branch from 2f49205 to e92b6bc Compare June 8, 2018 15:00
@Kingwl
Copy link
Contributor Author

Kingwl commented Jun 8, 2018

ahhh, Sure enough it committed in my another pc

@mhegazy
Copy link
Contributor

mhegazy commented Jun 8, 2018

@rbuckton can you take a look.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 13, 2020

Heya @Kingwl, I've started to run the tarball bundle task on this PR at 569b35e. You can monitor the build here. It should now contribute to this PR's status checks.

@sandersn sandersn added this to Curio in Pall Mall Jan 28, 2020
@sandersn sandersn moved this from Curio to Gallery in Pall Mall Jan 28, 2020
@sandersn sandersn added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Milestone Bug PRs that fix a bug with a specific milestone labels Jan 31, 2020
@sandersn sandersn added this to Needs review in PR Backlog Feb 5, 2020
@sandersn sandersn moved this from Needs review to Needs merge in PR Backlog Feb 5, 2020
@sandersn
Copy link
Member

sandersn commented Feb 5, 2020

One last CI run and I will merge this.

@sandersn sandersn removed this from Gallery in Pall Mall Feb 5, 2020
@sandersn
Copy link
Member

sandersn commented Feb 5, 2020

Oh no, it failed on the lint rules we just enabled! I pushed a new commit.

@Kingwl
Copy link
Contributor Author

Kingwl commented Feb 5, 2020

What is the meaning of For Milestone Bug ... :XD

@sandersn sandersn merged commit 70399e1 into microsoft:master Feb 5, 2020
PR Backlog automation moved this from Needs merge to Done Feb 5, 2020
@sandersn
Copy link
Member

sandersn commented Feb 5, 2020

@Kingwl the PR is intended to fix a bug that has been accepted into a milestone. That contrasts with "For Backlog Bug", which are PRs intended to fix a backlog bug. This PR technically fixes a backlog bug, but it's an Ecmascript conformance bug, so I think it should have been put in a milestone long ago.

@Kingwl Kingwl deleted the Lift-Template-Literal-Restriction branch February 6, 2020 14:47
@Kingwl
Copy link
Contributor Author

Kingwl commented Feb 6, 2020

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 6, 2020

Heya @Kingwl, I've started to run the tarball bundle task on this PR at f8b9bbb. You can monitor the build here. It should now contribute to this PR's status checks.

elibarzilay added a commit to elibarzilay/TypeScript that referenced this pull request May 15, 2020
This problem was introduced in 70399e1 (from PR microsoft#23801), which added
a `visitTaggedTemplateExpression` case for `TaggedTemplateExpression`,
before that, it would fallback to the default of `visitNode`.  So re-add
that happen in `processTaggedTemplateExpression`.

Since it doesn't hurt, I left a `Debug.checkDefined(property.name)`
instead of `!`-ing it.

Fixes microsoft#38558.
elibarzilay added a commit to elibarzilay/TypeScript that referenced this pull request May 15, 2020
This problem was introduced in 70399e1 (from PR microsoft#23801), which added
a `visitTaggedTemplateExpression` case for `TaggedTemplateExpression`,
before that, it would fallback to the default of `visitNode`.  So re-add
that happen in `processTaggedTemplateExpression`.

Since it doesn't hurt, I left a `Debug.checkDefined(property.name)`
instead of `!`-ing it.

Fixes microsoft#38558.
elibarzilay added a commit that referenced this pull request May 15, 2020
This problem was introduced in 70399e1 (from PR #23801), which added
a `visitTaggedTemplateExpression` case for `TaggedTemplateExpression`,
before that, it would fallback to the default of `visitNode`.  So re-add
that happen in `processTaggedTemplateExpression`.

Since it doesn't hurt, I left a `Debug.checkDefined(property.name)`
instead of `!`-ing it.

Fixes #38558.
@Kingwl Kingwl mentioned this pull request Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Lift Template Literal Restriction on Backslashes
7 participants