Give mapped type properties a synthetic declaration name#18023
Conversation
| return "(Anonymous function)"; | ||
| } | ||
| } | ||
| if (hasProperty(symbol, "syntheticName")) { |
There was a problem hiding this comment.
I would rather see if ((symbol as TransientSymbol).syntheticName), because hasProperty isn't type-checked.
There was a problem hiding this comment.
(symbol as TransientSymbol).syntheticName is falsy for "", so it would have to be typeof (symbol as TransientSymbol).syntheticName === "string"
| export type Timespans = | ||
| '1 hour' | '4 hours' | '12 hours' | '1 day' | '3 days' | '1 week' | '1 month'; | ||
| const TIMESPANS: {[k in Timespans]: number} = { | ||
| '1 hour': 1 * 60 * 60 * 1000, |
There was a problem hiding this comment.
I'd rather see the smallest test possible, so long as it failed before this PR and succeeds after.
| prop.syntheticOrigin = propertySymbol; | ||
| prop.declarations = propertySymbol.declarations; | ||
| } | ||
| else if (!isIdentifierText((<StringLiteralType>t).value, compilerOptions.target)) { |
There was a problem hiding this comment.
Nit: Would be prettier to access value once at the top of this block
sandersn
left a comment
There was a problem hiding this comment.
I'm not sure this is the correct way to fix the bug.
| } | ||
| } | ||
| if (hasProperty(symbol, "syntheticName")) { | ||
| return (symbol as TransientSymbol).syntheticName; |
There was a problem hiding this comment.
can you use symbol.syntheticOrigin.name? (or escapedName, not sure which). Even if it doesn't exist, creating a synthetic symbol for syntheticOrigin might make more sense than creating a standalone name.
There was a problem hiding this comment.
No - the symbol will have no syntheticOrigin, as it has no source type to draw a named declaration from.
| function addMemberForKeyType(t: Type, propertySymbol?: Symbol) { | ||
| function addMemberForKeyType(t: Type, propertySymbolOrIndex?: Symbol | number) { | ||
| let propertySymbol: Symbol; | ||
| // forEachType offloads to forEach, which calls with a numeric second argument |
There was a problem hiding this comment.
I can't comment on line 5724 because of github's limitations, but I would just change that to foreachType(iterationType, (p,i) => addMemberForKeyType(p)) and remove this whole changed section.
There was a problem hiding this comment.
We usually avoid inline lambdas (for things which could get called a lot), from what I've been told, since they don't get optimized as well as a named function. Could be false, I dunno.
| prop.declarations = propertySymbol.declarations; | ||
| } | ||
| else if (!isIdentifierText((<StringLiteralType>t).value, compilerOptions.target)) { | ||
| prop.syntheticName = `"${escapeString((<StringLiteralType>t).value, CharacterCodes.doubleQuote)}"`; |
There was a problem hiding this comment.
why use escapeString instead of escapeLeadingUnderscores as in propName?
There was a problem hiding this comment.
What is propName in the buggy case, anyway? Why not change propName in this case to be the version that's called with escapeString, because then line 5748 (const prop = createSymbol(...) would do this name lookup that we need for free.
There was a problem hiding this comment.
escapeString escapes the specified quotemark, to make the string suitable for use within quotes. escapeLeadingUnderscores converts a string to a __String by adding extra leading underscore, if needed. Totally different.
|
@Andy-MS These symbols have no synthetic origin, since a union of string literal types does point to any |
75c34d0 to
1215a99
Compare
| function addMemberForKeyType(t: Type, propertySymbol?: Symbol) { | ||
| function addMemberForKeyType(t: Type, propertySymbolOrIndex?: Symbol | number) { | ||
| let propertySymbol: Symbol; | ||
| // forEachType offloads to forEach, which calls with a numeric second argument |
Which escapes them appropriately if need be, as is suitable for printing.
Fixes #18012