Skip to content

Created a branded type for identifier-escaped strings #16915

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

Merged
merged 26 commits into from
Jul 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
042a78d
Created a branded type for escaped strings
weswigham Jun 30, 2017
ce4018c
Correctly double underscores WRT mapped types
weswigham Jul 3, 2017
342ec75
Add fourslash tests for other fixed issues
weswigham Jul 3, 2017
770010e
Merge branch 'master' into escaped-string-type
weswigham Jul 3, 2017
1f55366
use function call over cast
weswigham Jul 5, 2017
4411889
Update forEachEntry type accuracy
weswigham Jul 5, 2017
47f7fbd
Just use escaped names for ActiveLabel
weswigham Jul 5, 2017
7f22f39
Remove casts from getPropertyNameForPropertyNameNode
weswigham Jul 5, 2017
f88f21a
This pattern has occurred a few times, could use a helper function.
weswigham Jul 5, 2017
af30041
Remove duplicated helper
weswigham Jul 5, 2017
5d9ea3a
Remove unneeded check, use helper
weswigham Jul 5, 2017
352c13d
Identifiers list is no longer escaped strings
weswigham Jul 5, 2017
b371e7a
Extract repeated string-getting code into helper
weswigham Jul 5, 2017
ba0071e
Rename type and associated functions
weswigham Jul 5, 2017
e0c7dcc
Make getName() return UnderscoreEscapedString, add getUnescapedName()
weswigham Jul 6, 2017
50a8c04
Add list of internal symbol names to escaped string type to cut back …
weswigham Jul 6, 2017
3398d41
Remove outdated comments
weswigham Jul 6, 2017
a4bd2ab
Reassign interned values to nodes, just in case
weswigham Jul 6, 2017
0967927
Swap to string enum
weswigham Jul 6, 2017
e99b679
Add deprecated aliases to escapeIdentifier and unescapeIdentifier
weswigham Jul 6, 2017
3383d8a
Add temp var
weswigham Jul 6, 2017
5eca8a3
Remove unsafe casts
weswigham Jul 6, 2017
9424f14
Rename escaped string type as per @sandersn's suggestion, fix string …
weswigham Jul 6, 2017
23032d4
Reorganize double underscore tests
weswigham Jul 6, 2017
008532d
Remove jfreeman from TODO
weswigham Jul 6, 2017
5e6b69a
Remove unneeded parenthesis
weswigham Jul 6, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 54 additions & 53 deletions src/compiler/binder.ts

Large diffs are not rendered by default.

501 changes: 250 additions & 251 deletions src/compiler/checker.ts

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1047,7 +1047,7 @@ namespace ts {
errors.push(createDiagnosticForNodeInSourceFile(sourceFile, element.name, Diagnostics.String_literal_with_double_quotes_expected));
}

const keyText = getTextOfPropertyName(element.name);
const keyText = unescapeLeadingUnderscores(getTextOfPropertyName(element.name));
const option = knownOptions ? knownOptions.get(keyText) : undefined;
if (extraKeyDiagnosticMessage && !option) {
errors.push(createDiagnosticForNodeInSourceFile(sourceFile, element.name, extraKeyDiagnosticMessage, keyText));
Expand Down
46 changes: 35 additions & 11 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,22 @@ namespace ts {
return new MapCtr<T>();
}

/** Create a new escaped identifier map. */
export function createUnderscoreEscapedMap<T>(): UnderscoreEscapedMap<T> {
return new MapCtr<T>() as UnderscoreEscapedMap<T>;
}

/* @internal */
export function createSymbolTable(symbols?: Symbol[]): SymbolTable {
const result = createMap<Symbol>() as SymbolTable;
if (symbols) {
for (const symbol of symbols) {
result.set(symbol.name, symbol);
}
}
return result;
}

export function createMapFromTemplate<T>(template?: MapLike<T>): Map<T> {
const map: Map<T> = new MapCtr<T>();

Expand Down Expand Up @@ -1000,11 +1016,13 @@ namespace ts {
* Calls `callback` for each entry in the map, returning the first truthy result.
* Use `map.forEach` instead for normal iteration.
*/
export function forEachEntry<T, U>(map: Map<T>, callback: (value: T, key: string) => U | undefined): U | undefined {
export function forEachEntry<T, U>(map: UnderscoreEscapedMap<T>, callback: (value: T, key: __String) => U | undefined): U | undefined;
export function forEachEntry<T, U>(map: Map<T>, callback: (value: T, key: string) => U | undefined): U | undefined;
export function forEachEntry<T, U>(map: UnderscoreEscapedMap<T> | Map<T>, callback: (value: T, key: (string & __String)) => U | undefined): U | undefined {
const iterator = map.entries();
for (let { value: pair, done } = iterator.next(); !done; { value: pair, done } = iterator.next()) {
const [key, value] = pair;
const result = callback(value, key);
const result = callback(value, key as (string & __String));
if (result) {
return result;
}
Expand All @@ -1013,10 +1031,12 @@ namespace ts {
}

/** `forEachEntry` for just keys. */
export function forEachKey<T>(map: Map<{}>, callback: (key: string) => T | undefined): T | undefined {
export function forEachKey<T>(map: UnderscoreEscapedMap<{}>, callback: (key: __String) => T | undefined): T | undefined;
export function forEachKey<T>(map: Map<{}>, callback: (key: string) => T | undefined): T | undefined;
export function forEachKey<T>(map: UnderscoreEscapedMap<{}> | Map<{}>, callback: (key: string & __String) => T | undefined): T | undefined {
const iterator = map.keys();
for (let { value: key, done } = iterator.next(); !done; { value: key, done } = iterator.next()) {
const result = callback(key);
const result = callback(key as string & __String);
if (result) {
return result;
}
Expand All @@ -1025,9 +1045,11 @@ namespace ts {
}

/** Copy entries from `source` to `target`. */
export function copyEntries<T>(source: Map<T>, target: Map<T>): void {
source.forEach((value, key) => {
target.set(key, value);
export function copyEntries<T>(source: UnderscoreEscapedMap<T>, target: UnderscoreEscapedMap<T>): void;
export function copyEntries<T>(source: Map<T>, target: Map<T>): void;
export function copyEntries<T, U extends UnderscoreEscapedMap<T> | Map<T>>(source: U, target: U): void {
(source as Map<T>).forEach((value, key) => {
(target as Map<T>).set(key, value);
});
}

Expand Down Expand Up @@ -1099,9 +1121,11 @@ namespace ts {
return arrayToMap<T, true>(array, makeKey, () => true);
}

export function cloneMap<T>(map: Map<T>) {
export function cloneMap(map: SymbolTable): SymbolTable;
export function cloneMap<T>(map: Map<T>): Map<T>;
export function cloneMap<T>(map: Map<T> | SymbolTable): Map<T> | SymbolTable {
const clone = createMap<T>();
copyEntries(map, clone);
copyEntries(map as Map<T>, clone);
return clone;
}

Expand Down Expand Up @@ -2272,13 +2296,13 @@ namespace ts {
getTokenConstructor(): new <TKind extends SyntaxKind>(kind: TKind, pos?: number, end?: number) => Token<TKind>;
getIdentifierConstructor(): new (kind: SyntaxKind.Identifier, pos?: number, end?: number) => Identifier;
getSourceFileConstructor(): new (kind: SyntaxKind.SourceFile, pos?: number, end?: number) => SourceFile;
getSymbolConstructor(): new (flags: SymbolFlags, name: string) => Symbol;
getSymbolConstructor(): new (flags: SymbolFlags, name: __String) => Symbol;
getTypeConstructor(): new (checker: TypeChecker, flags: TypeFlags) => Type;
getSignatureConstructor(): new (checker: TypeChecker) => Signature;
getSourceMapSourceConstructor(): new (fileName: string, text: string, skipTrivia?: (pos: number) => number) => SourceMapSource;
}

function Symbol(this: Symbol, flags: SymbolFlags, name: string) {
function Symbol(this: Symbol, flags: SymbolFlags, name: __String) {
this.flags = flags;
this.name = name;
this.declarations = undefined;
Expand Down
12 changes: 6 additions & 6 deletions src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2761,7 +2761,7 @@ namespace ts {
return generateName(node);
}
else if (isIdentifier(node) && (nodeIsSynthesized(node) || !node.parent)) {
return unescapeIdentifier(node.text);
return unescapeLeadingUnderscores(node.text);
}
else if (node.kind === SyntaxKind.StringLiteral && (<StringLiteral>node).textSourceNode) {
return getTextOfNode((<StringLiteral>node).textSourceNode, includeTrivia);
Expand Down Expand Up @@ -2818,13 +2818,13 @@ namespace ts {
// Auto, Loop, and Unique names are cached based on their unique
// autoGenerateId.
const autoGenerateId = name.autoGenerateId;
return autoGeneratedIdToGeneratedName[autoGenerateId] || (autoGeneratedIdToGeneratedName[autoGenerateId] = unescapeIdentifier(makeName(name)));
return autoGeneratedIdToGeneratedName[autoGenerateId] || (autoGeneratedIdToGeneratedName[autoGenerateId] = makeName(name));
}
}

function generateNameCached(node: Node) {
const nodeId = getNodeId(node);
return nodeIdToGeneratedName[nodeId] || (nodeIdToGeneratedName[nodeId] = unescapeIdentifier(generateNameForNode(node)));
return nodeIdToGeneratedName[nodeId] || (nodeIdToGeneratedName[nodeId] = generateNameForNode(node));
}

/**
Expand All @@ -2843,7 +2843,7 @@ namespace ts {
function isUniqueLocalName(name: string, container: Node): boolean {
for (let node = container; isNodeDescendantOf(node, container); node = node.nextContainer) {
if (node.locals) {
const local = node.locals.get(name);
const local = node.locals.get(escapeLeadingUnderscores(name));
// We conservatively include alias symbols to cover cases where they're emitted as locals
if (local && local.flags & (SymbolFlags.Value | SymbolFlags.ExportValue | SymbolFlags.Alias)) {
return false;
Expand Down Expand Up @@ -2918,7 +2918,7 @@ namespace ts {
function generateNameForImportOrExportDeclaration(node: ImportDeclaration | ExportDeclaration) {
const expr = getExternalModuleName(node);
const baseName = expr.kind === SyntaxKind.StringLiteral ?
escapeIdentifier(makeIdentifierFromModuleName((<LiteralExpression>expr).text)) : "module";
makeIdentifierFromModuleName((<LiteralExpression>expr).text) : "module";
return makeUniqueName(baseName);
}

Expand Down Expand Up @@ -2981,7 +2981,7 @@ namespace ts {
case GeneratedIdentifierKind.Loop:
return makeTempVariableName(TempFlags._i);
case GeneratedIdentifierKind.Unique:
return makeUniqueName(unescapeIdentifier(name.text));
return makeUniqueName(unescapeLeadingUnderscores(name.text));
}

Debug.fail("Unsupported GeneratedIdentifierKind.");
Expand Down
10 changes: 5 additions & 5 deletions src/compiler/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ namespace ts {
}

function createLiteralFromNode(sourceNode: StringLiteral | NumericLiteral | Identifier): StringLiteral {
const node = createStringLiteral(sourceNode.text);
const node = createStringLiteral(getTextOfIdentifierOrLiteral(sourceNode));
node.textSourceNode = sourceNode;
return node;
}
Expand All @@ -112,7 +112,7 @@ namespace ts {
export function createIdentifier(text: string, typeArguments: TypeNode[]): Identifier;
export function createIdentifier(text: string, typeArguments?: TypeNode[]): Identifier {
const node = <Identifier>createSynthesizedNode(SyntaxKind.Identifier);
node.text = escapeIdentifier(text);
node.text = escapeLeadingUnderscores(text);
node.originalKeywordKind = text ? stringToToken(text) : SyntaxKind.Unknown;
node.autoGenerateKind = GeneratedIdentifierKind.None;
node.autoGenerateId = 0;
Expand All @@ -124,7 +124,7 @@ namespace ts {

export function updateIdentifier(node: Identifier, typeArguments: NodeArray<TypeNode> | undefined): Identifier {
return node.typeArguments !== typeArguments
? updateNode(createIdentifier(node.text, typeArguments), node)
? updateNode(createIdentifier(unescapeLeadingUnderscores(node.text), typeArguments), node)
: node;
}

Expand Down Expand Up @@ -2643,12 +2643,12 @@ namespace ts {
function createJsxFactoryExpressionFromEntityName(jsxFactory: EntityName, parent: JsxOpeningLikeElement): Expression {
if (isQualifiedName(jsxFactory)) {
const left = createJsxFactoryExpressionFromEntityName(jsxFactory.left, parent);
const right = createIdentifier(jsxFactory.right.text);
const right = createIdentifier(unescapeLeadingUnderscores(jsxFactory.right.text));
right.text = jsxFactory.right.text;
return createPropertyAccess(left, right);
}
else {
return createReactNamespace(jsxFactory.text, parent);
return createReactNamespace(unescapeLeadingUnderscores(jsxFactory.text), parent);
}
}

Expand Down
28 changes: 15 additions & 13 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1178,12 +1178,11 @@ namespace ts {
}

const result = createNode(kind, scanner.getStartPos());
(<Identifier>result).text = "";
(<Identifier>result).text = "" as __String;
return finishNode(result);
}

function internIdentifier(text: string): string {
text = escapeIdentifier(text);
let identifier = identifiers.get(text);
if (identifier === undefined) {
identifiers.set(text, identifier = text);
Expand All @@ -1203,7 +1202,7 @@ namespace ts {
if (token() !== SyntaxKind.Identifier) {
node.originalKeywordKind = token();
}
node.text = internIdentifier(scanner.getTokenValue());
node.text = escapeLeadingUnderscores(internIdentifier(scanner.getTokenValue()));
Copy link
Member

Choose a reason for hiding this comment

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

internIdentifier used to escape before interning. Now the code interns, then escapes. Is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is correct for how we use the identifiers map the internIdentifier function uses (namely, looking for collisions in generated identifiers).

nextToken();
return finishNode(node);
}
Expand All @@ -1227,7 +1226,9 @@ namespace ts {

function parsePropertyNameWorker(allowComputedPropertyNames: boolean): PropertyName {
if (token() === SyntaxKind.StringLiteral || token() === SyntaxKind.NumericLiteral) {
return <StringLiteral | NumericLiteral>parseLiteralNode(/*internName*/ true);
const node = <StringLiteral | NumericLiteral>parseLiteralNode();
node.text = internIdentifier(node.text);
return node;
}
if (allowComputedPropertyNames && token() === SyntaxKind.OpenBracketToken) {
return parseComputedPropertyName();
Expand Down Expand Up @@ -2049,26 +2050,26 @@ namespace ts {
return finishNode(span);
}

function parseLiteralNode(internName?: boolean): LiteralExpression {
return <LiteralExpression>parseLiteralLikeNode(token(), internName);
function parseLiteralNode(): LiteralExpression {
return <LiteralExpression>parseLiteralLikeNode(token());
}

function parseTemplateHead(): TemplateHead {
const fragment = parseLiteralLikeNode(token(), /*internName*/ false);
const fragment = parseLiteralLikeNode(token());
Debug.assert(fragment.kind === SyntaxKind.TemplateHead, "Template head has wrong token kind");
return <TemplateHead>fragment;
}

function parseTemplateMiddleOrTemplateTail(): TemplateMiddle | TemplateTail {
const fragment = parseLiteralLikeNode(token(), /*internName*/ false);
const fragment = parseLiteralLikeNode(token());
Debug.assert(fragment.kind === SyntaxKind.TemplateMiddle || fragment.kind === SyntaxKind.TemplateTail, "Template fragment has wrong token kind");
return <TemplateMiddle | TemplateTail>fragment;
}

function parseLiteralLikeNode(kind: SyntaxKind, internName: boolean): LiteralLikeNode {
function parseLiteralLikeNode(kind: SyntaxKind): LiteralLikeNode {
const node = <LiteralExpression>createNode(kind);
const text = scanner.getTokenValue();
node.text = internName ? internIdentifier(text) : text;
node.text = text;

if (scanner.hasExtendedUnicodeEscape()) {
node.hasExtendedUnicodeEscape = true;
Expand Down Expand Up @@ -5624,7 +5625,8 @@ namespace ts {
node.flags |= NodeFlags.GlobalAugmentation;
}
else {
node.name = <StringLiteral>parseLiteralNode(/*internName*/ true);
node.name = <StringLiteral>parseLiteralNode();
node.name.text = internIdentifier(node.name.text);
}

if (token() === SyntaxKind.OpenBraceToken) {
Expand Down Expand Up @@ -5768,7 +5770,7 @@ namespace ts {
function parseModuleSpecifier(): Expression {
if (token() === SyntaxKind.StringLiteral) {
const result = parseLiteralNode();
internIdentifier((<LiteralExpression>result).text);
result.text = internIdentifier(result.text);
return result;
}
else {
Expand Down Expand Up @@ -6990,7 +6992,7 @@ namespace ts {
const pos = scanner.getTokenPos();
const end = scanner.getTextPos();
const result = <Identifier>createNode(SyntaxKind.Identifier, pos);
result.text = content.substring(pos, end);
result.text = escapeLeadingUnderscores(content.substring(pos, end));
finishNode(result, end);

nextJSDocToken();
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ namespace ts {
let commonSourceDirectory: string;
let diagnosticsProducingTypeChecker: TypeChecker;
let noDiagnosticsTypeChecker: TypeChecker;
let classifiableNames: Map<string>;
let classifiableNames: UnderscoreEscapedMap<__String>;
let modifiedFilePaths: Path[] | undefined;

const cachedSemanticDiagnosticsForFile: DiagnosticCache = {};
Expand Down Expand Up @@ -580,7 +580,7 @@ namespace ts {
if (!classifiableNames) {
// Initialize a checker so that all our files are bound.
getTypeChecker();
classifiableNames = createMap<string>();
classifiableNames = createUnderscoreEscapedMap<__String>();

for (const sourceFile of files) {
copyEntries(sourceFile.classifiableNames, classifiableNames);
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/transformers/destructuring.ts
Original file line number Diff line number Diff line change
Expand Up @@ -411,11 +411,11 @@ namespace ts {
}
else if (isStringOrNumericLiteral(propertyName)) {
const argumentExpression = getSynthesizedClone(propertyName);
argumentExpression.text = unescapeIdentifier(argumentExpression.text);
argumentExpression.text = argumentExpression.text;
return createElementAccess(value, argumentExpression);
}
else {
const name = createIdentifier(unescapeIdentifier(propertyName.text));
const name = createIdentifier(unescapeLeadingUnderscores(propertyName.text));
return createPropertyAccess(value, name);
}
}
Expand Down
12 changes: 6 additions & 6 deletions src/compiler/transformers/es2015.ts
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ namespace ts {
// - break/continue is non-labeled and located in non-converted loop/switch statement
const jump = node.kind === SyntaxKind.BreakStatement ? Jump.Break : Jump.Continue;
const canUseBreakOrContinue =
(node.label && convertedLoopState.labels && convertedLoopState.labels.get(node.label.text)) ||
(node.label && convertedLoopState.labels && convertedLoopState.labels.get(unescapeLeadingUnderscores(node.label.text))) ||
(!node.label && (convertedLoopState.allowedNonLabeledJumps & jump));

if (!canUseBreakOrContinue) {
Expand All @@ -680,11 +680,11 @@ namespace ts {
else {
if (node.kind === SyntaxKind.BreakStatement) {
labelMarker = `break-${node.label.text}`;
setLabeledJump(convertedLoopState, /*isBreak*/ true, node.label.text, labelMarker);
setLabeledJump(convertedLoopState, /*isBreak*/ true, unescapeLeadingUnderscores(node.label.text), labelMarker);
}
else {
labelMarker = `continue-${node.label.text}`;
setLabeledJump(convertedLoopState, /*isBreak*/ false, node.label.text, labelMarker);
setLabeledJump(convertedLoopState, /*isBreak*/ false, unescapeLeadingUnderscores(node.label.text), labelMarker);
}
}
let returnExpression: Expression = createLiteral(labelMarker);
Expand Down Expand Up @@ -2236,11 +2236,11 @@ namespace ts {
}

function recordLabel(node: LabeledStatement) {
convertedLoopState.labels.set(node.label.text, node.label.text);
convertedLoopState.labels.set(unescapeLeadingUnderscores(node.label.text), unescapeLeadingUnderscores(node.label.text));
}

function resetLabel(node: LabeledStatement) {
convertedLoopState.labels.set(node.label.text, undefined);
convertedLoopState.labels.set(unescapeLeadingUnderscores(node.label.text), undefined);
}

function visitLabeledStatement(node: LabeledStatement): VisitResult<Statement> {
Expand Down Expand Up @@ -3053,7 +3053,7 @@ namespace ts {
else {
loopParameters.push(createParameter(/*decorators*/ undefined, /*modifiers*/ undefined, /*dotDotDotToken*/ undefined, name));
if (resolver.getNodeCheckFlags(decl) & NodeCheckFlags.NeedsLoopOutParameter) {
const outParamName = createUniqueName("out_" + unescapeIdentifier(name.text));
const outParamName = createUniqueName("out_" + unescapeLeadingUnderscores(name.text));
loopOutParameters.push({ originalName: name, outParamName });
}
}
Expand Down
Loading