Preserve string delimiter in type printing. #60729

dragomirtitian · 2024-12-10T17:08:36Z

Related to #60540

This PR shows what the impact of preserving string delimiters in type printing would be.

tests/baselines/reference/ambientErrors.types

src/compiler/checker.ts

jakebailey · 2024-12-13T19:19:23Z

We looked at this in the design meeting; the general consensus is that this is good for declaration files, but the fact that error messages are changing quote styles seems to be wrong; we should try and make diagnostic type printing normalize to " quotes because that's what our messages assume happen.

typescript-bot · 2024-12-18T17:35:15Z

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

src/compiler/checker.ts

jakebailey · 2024-12-18T20:17:48Z

tests/baselines/reference/aliasDoesNotDuplicateSignatures.symbols

@@ -8,7 +8,7 @@ declare namespace demoNS {
 >f : Symbol(f, Decl(demo.d.ts, 0, 26))
 }
 declare module 'demoModule' {
->'demoModule' : Symbol("demoModule", Decl(demo.d.ts, 2, 1))
+>'demoModule' : Symbol('demoModule', Decl(demo.d.ts, 2, 1))


I'm surprised to see symbol baselines change; were we inconsistent with these before?

Symbol printing is involved in type printing and error printing. I applied the 'preserve source code delimiters in everything except errors' to the symbol printing too. This does mean that the delimiters are now preserved in symbol baselines where they previously were not.

Were they always replaced before? Don't think so. Some error messages used symbol printing for the property names and we can see in error baselines changes that some of those kept the ' delimiter instead of having it replaced with "

Hm, I tried doing this in typeWriter.ts:

let symbolString = "Symbol(" + this.checker.symbolToString(symbol, node.parent, undefined, ts.SymbolFormatFlags.AllowAnyNodeKind | ts.SymbolFormatFlags.UseDoubleQuotesForStringLiteralType);

And it actually still changed a whole bunch of double quotes to single quotes.... Is there some place which is not respecting the flag?

With the latest changes, the above seems to actually work, though I don't think I actually think it matters; preserving stuff seems reasonable enough.

tests/baselines/reference/completionListForImportAttributes.baseline

tests/baselines/reference/assignmentCompatWithObjectMembersStringNumericNames.errors.txt

jakebailey

I read through all of the baselines; mostly seems correct, with some odd escaping changes, and I would like someone familiar with those displayParts to weigh in on those changes.

tests/baselines/reference/objectLiteralGettersAndSetters.symbols

tests/baselines/reference/objectTypeWithStringNamedPropertyOfIllegalCharacters.symbols

tests/baselines/reference/stringLiteralPropertyNameWithLineContinuation1.symbols

typescript-bot · 2024-12-20T14:56:56Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

src/compiler/checker.ts

jakebailey · 2025-01-06T20:00:21Z

tests/baselines/reference/aliasDoesNotDuplicateSignatures.symbols

@@ -8,7 +8,7 @@ declare namespace demoNS {
 >f : Symbol(f, Decl(demo.d.ts, 0, 26))
 }
 declare module 'demoModule' {
->'demoModule' : Symbol("demoModule", Decl(demo.d.ts, 2, 1))
+>'demoModule' : Symbol('demoModule', Decl(demo.d.ts, 2, 1))


Hm, I tried doing this in typeWriter.ts:

let symbolString = "Symbol(" + this.checker.symbolToString(symbol, node.parent, undefined, ts.SymbolFormatFlags.AllowAnyNodeKind | ts.SymbolFormatFlags.UseDoubleQuotesForStringLiteralType);

And it actually still changed a whole bunch of double quotes to single quotes.... Is there some place which is not respecting the flag?

jakebailey · 2025-01-07T22:06:11Z

The PR seems good to me, outside my comment above about symbol baselines and the weird quoting not changing where I expected it to.

dragomirtitian · 2025-01-08T18:05:38Z

The PR seems good to me, outside my comment above about symbol baselines and the weird quoting not changing where I expected it to.

I did not see places where the delimiter was replaced, I did see places where the delimiters were preserved as in source code.

This is because there are still cases where an identifier is created from whatever text was in the computed property.

// in symbolToExpression
if (canUsePropertyAccess(symbolName, languageVersion) || (index === 0 && firstChar === CharacterCodes.openBracket)) {
    const identifier = setEmitFlags(factory.createIdentifier(symbolName), EmitFlags.NoAsciiEscaping);
    if (typeParameterNodes) setIdentifierTypeArguments(identifier, factory.createNodeArray<TypeNode | TypeParameterDeclaration>(typeParameterNodes));
    identifier.symbol = symbol;

    return index > 0 ? factory.createPropertyAccessExpression(createExpressionFromSymbolChain(chain, index - 1), identifier) : identifier;
}

Because symbolToExpression returns an expression it can't return a raw computed property name (ie it can't return ['x']). This means that the identifier created will contain whatever string is used in the computed including delimiter present in source code.

I am working on changing this, but it's proving a bit trickier than I expected. Will push tomorrow a version that does not just reuse text if index === 0 && firstChar === CharacterCodes.openBracket and replaces delimiters in that case too.

jakebailey · 2025-01-13T18:53:45Z

tests/baselines/reference/declarationEmitLateBoundAssignments2.symbols

@@ -79,7 +79,7 @@ export function decl9() {}

 decl9["🤪"] = 0
 >decl9 : Symbol(decl9, Decl(declarationEmitLateBoundAssignments2.ts, 30, 25), Decl(declarationEmitLateBoundAssignments2.ts, 32, 26))
->"🤪" : Symbol(decl9["\uD83E\uDD2A"], Decl(declarationEmitLateBoundAssignments2.ts, 32, 26))
+>"🤪" : Symbol(decl9["🤪"], Decl(declarationEmitLateBoundAssignments2.ts, 32, 26))


jakebailey · 2025-01-13T18:54:36Z

tests/baselines/reference/aliasDoesNotDuplicateSignatures.symbols

@@ -8,7 +8,7 @@ declare namespace demoNS {
 >f : Symbol(f, Decl(demo.d.ts, 0, 26))
 }
 declare module 'demoModule' {
->'demoModule' : Symbol("demoModule", Decl(demo.d.ts, 2, 1))
+>'demoModule' : Symbol('demoModule', Decl(demo.d.ts, 2, 1))


With the latest changes, the above seems to actually work, though I don't think I actually think it matters; preserving stuff seems reasonable enough.

jakebailey · 2025-01-13T20:36:42Z

src/compiler/checker.ts

-                }
-            }
-            return symbolToExpression(symbol, context, meaning);
+            return symbolToExpression(symbol, context, meaning, !!(context.internalFlags & InternalNodeBuilderFlags.WriteComputedProps));


Is checking the context flags within symbolToExpression not sufficient?

symbolToExpression is invoked from several places (ex: serializeEntityName, nodeBuilder.symbolToExpression, createExpressionFromSymbolChain). In those places the expected return for symbolToExpression is always an expression. A computed property is not an expression. So in those cases it should always return an expression regardless of the flag. symbolToNode already return a generic Node so you can expect to get anything, including a computed property.

I mainly meant "does this have to be a parameter if we are already passing in context"

But I guess you're saying that other callers never want the computed prop name?

I do not like pushing this logic into symbolToExpression and overloading it with a boolean parameter - it ceases to be symbolToExpression at that point. It really aughta have an explicit Expression return, because the NodeBuilder reexposes it as such and we shouldn't be relying on our not-so-strict method assignment rules to ensure it stays that way. I can't see how this is actually required, either - this seems to be the only callsite that passes this flag dynamically; other callers which may or may not want to ensure expression-ness still swap between using symbolToNode and symbolToExpression as needed... I would rather the style preservation logic common to the two be factored into a separate function used by both symbolToNode and symbolToExpression and leave the layering in-place (so all the logic for building non-expressions continues to live in symbolToNode).

weswigham · 2025-01-30T21:59:13Z

src/compiler/checker.ts

-                }
-            }
-            return symbolToExpression(symbol, context, meaning);
+            return symbolToExpression(symbol, context, meaning, !!(context.internalFlags & InternalNodeBuilderFlags.WriteComputedProps));


I do not like pushing this logic into symbolToExpression and overloading it with a boolean parameter - it ceases to be symbolToExpression at that point. It really aughta have an explicit Expression return, because the NodeBuilder reexposes it as such and we shouldn't be relying on our not-so-strict method assignment rules to ensure it stays that way. I can't see how this is actually required, either - this seems to be the only callsite that passes this flag dynamically; other callers which may or may not want to ensure expression-ness still swap between using symbolToNode and symbolToExpression as needed... I would rather the style preservation logic common to the two be factored into a separate function used by both symbolToNode and symbolToExpression and leave the layering in-place (so all the logic for building non-expressions continues to live in symbolToNode).

weswigham · 2025-01-30T22:20:51Z

src/compiler/checker.ts

+                            stringLiteralName.symbol = symbol;
+                            expression = stringLiteralName;
+                        }
+                        else if (("" + +symbolName) === symbolName && !hasEnumOrSymbolName) {


You probably want to use isValidNumberString to avoid handling a positive or negative Infinity literal type (which you can get by overflowing) like a number here.

weswigham · 2025-01-30T22:27:58Z

tests/baselines/reference/ambientShorthandFindAllRefs.baseline.jsonc

@@ -106,7 +106,7 @@
     },
     {
      "text": "\"jquery\"",
-      "kind": "stringLiteral"


I think this means that writeSymbol call added into the emitter might change how we colorize this quickinfo because of the now attached symbol data in VS (in vscode the quickinfo colorization isn't based on these kinds anymore). Specifically, in the default theme, I think the string's going to be a blue color instead of string color with this? Which seems maybe subjective but less good? I dunno if it matters, and don't actually know if that's still true for VS.

weswigham · 2025-01-30T22:51:10Z

tests/baselines/reference/numericEnumMappedType.symbols

@@ -49,7 +49,7 @@ const e2: E2 = E2.ONE;

 b1[1] = "a";
 >b1 : Symbol(b1, Decl(numericEnumMappedType.ts, 8, 5))
->1 : Symbol(1)


...wat? So all enum reverse mapping symbol names are a computed name of the enum member now? Is that...good...?

Preserve string delimiter in type printing.

2906dea

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 10, 2024

dragomirtitian mentioned this pull request Dec 10, 2024

Extract types from object literals instead of using the type checker. #60540

Open

jakebailey reviewed Dec 10, 2024

View reviewed changes

tests/baselines/reference/ambientErrors.types Show resolved Hide resolved

rbuckton reviewed Dec 13, 2024

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

dragomirtitian added 3 commits December 16, 2024 18:32

Only preserve string delimiters if we print for declaration files.

275f6b9

Preserve original quotes in all except errors.

638ceea

Preserve symbol name string delimiters

b8d575a

robpalme reviewed Dec 18, 2024

View reviewed changes

src/compiler/checker.ts Show resolved Hide resolved

jakebailey reviewed Dec 18, 2024

View reviewed changes

tests/baselines/reference/completionListForImportAttributes.baseline Show resolved Hide resolved

jakebailey reviewed Dec 18, 2024

View reviewed changes

tests/baselines/reference/assignmentCompatWithObjectMembersStringNumericNames.errors.txt Show resolved Hide resolved

jakebailey reviewed Dec 18, 2024

View reviewed changes

Improve preservation of strings in symbol printing.

1f2c52c

dragomirtitian marked this pull request as ready for review December 20, 2024 14:56

jakebailey reviewed Jan 6, 2025

View reviewed changes

dragomirtitian added 3 commits January 9, 2025 17:13

Improve symbol printing.

b983d7b

Improve respect of string delimiter preferences.

78f7c22

Revereted to using [] for missing names.

569b7ce

jakebailey reviewed Jan 13, 2025

View reviewed changes

jakebailey requested a review from weswigham January 13, 2025 20:35

jakebailey reviewed Jan 29, 2025

View reviewed changes

weswigham reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve string delimiter in type printing. #60729

Preserve string delimiter in type printing. #60729

dragomirtitian commented Dec 10, 2024

jakebailey commented Dec 13, 2024

typescript-bot commented Dec 18, 2024

jakebailey Dec 18, 2024

dragomirtitian Dec 18, 2024

jakebailey Jan 6, 2025

jakebailey Jan 13, 2025

jakebailey left a comment

typescript-bot commented Dec 20, 2024

jakebailey Jan 6, 2025

jakebailey commented Jan 7, 2025

dragomirtitian commented Jan 8, 2025

jakebailey Jan 13, 2025

jakebailey Jan 13, 2025

jakebailey Jan 13, 2025

dragomirtitian Jan 30, 2025

jakebailey Jan 30, 2025

jakebailey Jan 30, 2025

weswigham Jan 30, 2025

weswigham Jan 30, 2025

weswigham Jan 30, 2025

weswigham Jan 30, 2025

weswigham Jan 30, 2025

Preserve string delimiter in type printing. #60729

Are you sure you want to change the base?

Preserve string delimiter in type printing. #60729

Conversation

dragomirtitian commented Dec 10, 2024

jakebailey commented Dec 13, 2024

typescript-bot commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey left a comment

Choose a reason for hiding this comment

typescript-bot commented Dec 20, 2024

Choose a reason for hiding this comment

jakebailey commented Jan 7, 2025

dragomirtitian commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment