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 4 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
105 changes: 53 additions & 52 deletions src/compiler/binder.ts

Large diffs are not rendered by default.

491 changes: 243 additions & 248 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 = unescapeIdentifier(getTextOfPropertyName(element.name));
const option = knownOptions ? knownOptions.get(keyText) : undefined;
if (extraKeyDiagnosticMessage && !option) {
errors.push(createDiagnosticForNodeInSourceFile(sourceFile, element.name, extraKeyDiagnosticMessage, keyText));
Expand Down
41 changes: 30 additions & 11 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ namespace ts {
return new MapCtr<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 +1011,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: EscapedIdentifierMap<T>, callback: (value: T, key: string) => U | undefined): U | undefined;
Copy link

Choose a reason for hiding this comment

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

key: EscapedIdentifier

export function forEachEntry<T, U>(map: Map<T>, callback: (value: T, key: string) => U | undefined): U | undefined;
export function forEachEntry<T, U>(map: EscapedIdentifierMap<T> | Map<T>, callback: (value: T, key: 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);
if (result) {
return result;
}
Expand All @@ -1013,10 +1026,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: EscapedIdentifierMap<{}>, callback: (key: EscapedIdentifier) => T | undefined): T | undefined;
export function forEachKey<T>(map: Map<{}>, callback: (key: string) => T | undefined): T | undefined;
export function forEachKey<T>(map: EscapedIdentifierMap<{}> | Map<{}>, callback: (key: string & EscapedIdentifier) => 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 & EscapedIdentifier);
if (result) {
return result;
}
Expand All @@ -1025,9 +1040,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: EscapedIdentifierMap<T>, target: EscapedIdentifierMap<T>): void;
export function copyEntries<T>(source: Map<T>, target: Map<T>): void;
export function copyEntries<T, U extends EscapedIdentifierMap<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 +1116,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 +2291,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: EscapedIdentifier) => 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: EscapedIdentifier) {
this.flags = flags;
this.name = name;
this.declarations = undefined;
Expand Down
6 changes: 3 additions & 3 deletions src/compiler/declarationEmitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ namespace ts {
let resultHasExternalModuleIndicator: boolean;
let currentText: string;
let currentLineMap: number[];
let currentIdentifiers: Map<string>;
let currentIdentifiers: EscapedIdentifierMap<EscapedIdentifier>;
let isCurrentFileExternalModule: boolean;
let reportedDeclarationError = false;
let errorNameNode: DeclarationName;
Expand Down Expand Up @@ -608,14 +608,14 @@ namespace ts {
// Note that export default is only allowed at most once in a module, so we
// do not need to keep track of created temp names.
function getExportTempVariableName(baseName: string): string {
if (!currentIdentifiers.has(baseName)) {
if (!currentIdentifiers.has(escapeIdentifier(baseName))) {
return baseName;
}
let count = 0;
while (true) {
count++;
const name = baseName + "_" + count;
if (!currentIdentifiers.has(name)) {
if (!currentIdentifiers.has(escapeIdentifier(name))) {
return name;
}
}
Expand Down
10 changes: 5 additions & 5 deletions src/compiler/emitter.ts
Original file line number Diff line number Diff line change
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 @@ -2833,7 +2833,7 @@ namespace ts {
*/
function isUniqueName(name: string): boolean {
return !(hasGlobalName && hasGlobalName(name))
&& !currentSourceFile.identifiers.has(name)
&& !currentSourceFile.identifiers.has(escapeIdentifier(name))
&& !generatedNames.has(name);
}

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(escapeIdentifier(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
8 changes: 4 additions & 4 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(sourceNode.kind === SyntaxKind.Identifier ? unescapeIdentifier(sourceNode.text) : sourceNode.text);
node.textSourceNode = sourceNode;
return node;
}
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(unescapeIdentifier(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(unescapeIdentifier(jsxFactory.right.text));
right.text = jsxFactory.right.text;
return createPropertyAccess(left, right);
}
else {
return createReactNamespace(jsxFactory.text, parent);
return createReactNamespace(unescapeIdentifier(jsxFactory.text), parent);
}
}

Expand Down
33 changes: 18 additions & 15 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,7 @@ namespace ts {
let currentToken: SyntaxKind;
let sourceText: string;
let nodeCount: number;
let identifiers: Map<string>;
let identifiers: EscapedIdentifierMap<EscapedIdentifier>;
let identifierCount: number;

let parsingContext: ParsingContext;
Expand Down Expand Up @@ -681,7 +681,7 @@ namespace ts {

parseDiagnostics = [];
parsingContext = 0;
identifiers = createMap<string>();
identifiers = createMap<EscapedIdentifier>() as EscapedIdentifierMap<EscapedIdentifier>;
identifierCount = 0;
nodeCount = 0;

Expand Down Expand Up @@ -1178,12 +1178,12 @@ namespace ts {
}

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

function internIdentifier(text: string): string {
text = escapeIdentifier(text);
function internIdentifier(input: string): EscapedIdentifier {
const text = escapeIdentifier(input);
let identifier = identifiers.get(text);
if (identifier === undefined) {
identifiers.set(text, identifier = text);
Expand Down Expand Up @@ -1227,7 +1227,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(); // Interning makes the string literal be escaped - we do not want that
Copy link

@ghost ghost Jul 5, 2017

Choose a reason for hiding this comment

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

We probably want these to be interned (reused) strings though. I don't see any reason why identifiers needs to contain escaped names anyway -- if it is just for interning, we should be able to store plain strings.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

internIdentifier(node.text); // We do, however, want to intern the text for the purposes of the identifier list
return node;
}
if (allowComputedPropertyNames && token() === SyntaxKind.OpenBracketToken) {
return parseComputedPropertyName();
Expand Down Expand Up @@ -2049,26 +2051,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 @@ -4098,7 +4100,7 @@ namespace ts {
indexedAccess.argumentExpression = allowInAnd(parseExpression);
if (indexedAccess.argumentExpression.kind === SyntaxKind.StringLiteral || indexedAccess.argumentExpression.kind === SyntaxKind.NumericLiteral) {
const literal = <LiteralExpression>indexedAccess.argumentExpression;
literal.text = internIdentifier(literal.text);
internIdentifier(literal.text);
Copy link

Choose a reason for hiding this comment

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

Same, don't use this as a statement

}
}

Expand Down Expand Up @@ -5624,7 +5626,8 @@ namespace ts {
node.flags |= NodeFlags.GlobalAugmentation;
}
else {
node.name = <StringLiteral>parseLiteralNode(/*internName*/ true);
node.name = <StringLiteral>parseLiteralNode(); // Interning as an identifier causes it to be escaped - we do not want that
internIdentifier(node.name.text); // We do, however, want to intern the identifier for completions
}

if (token() === SyntaxKind.OpenBraceToken) {
Expand Down Expand Up @@ -6990,7 +6993,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 = escapeIdentifier(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: EscapedIdentifierMap<EscapedIdentifier>;
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 = createMap<string>() as EscapedIdentifierMap<EscapedIdentifier>;

for (const sourceFile of files) {
copyEntries(sourceFile.classifiableNames, classifiableNames);
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/transformers/destructuring.ts
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ namespace ts {
}
else if (isStringOrNumericLiteral(propertyName)) {
const argumentExpression = getSynthesizedClone(propertyName);
argumentExpression.text = unescapeIdentifier(argumentExpression.text);
argumentExpression.text = argumentExpression.text;
return createElementAccess(value, argumentExpression);
}
else {
Expand Down
10 changes: 5 additions & 5 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(unescapeIdentifier(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, unescapeIdentifier(node.label.text), labelMarker);
}
else {
labelMarker = `continue-${node.label.text}`;
setLabeledJump(convertedLoopState, /*isBreak*/ false, node.label.text, labelMarker);
setLabeledJump(convertedLoopState, /*isBreak*/ false, unescapeIdentifier(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(unescapeIdentifier(node.label.text), unescapeIdentifier(node.label.text));
}

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

function visitLabeledStatement(node: LabeledStatement): VisitResult<Statement> {
Expand Down
Loading