-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Extend addPropertyToElementList to differentiate get/set accessors from properties
#54935
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
base: main
Are you sure you want to change the base?
Changes from all commits
3e250e6
1957a47
111b16f
cfa77f3
fad8bcc
6a1e4e7
faf9843
79b71a7
706e1c9
7ad27c5
6610b5b
616c614
c442e29
af6febf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7165,17 +7165,50 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
| } | ||
| } | ||
|
|
||
| const modifiers = isReadonlySymbol(propertySymbol) ? [factory.createToken(SyntaxKind.ReadonlyKeyword)] : undefined; | ||
| if (modifiers) { | ||
| context.approximateLength += 9; | ||
| if (propertySymbol.flags & SymbolFlags.Accessor) { | ||
| if (propertySymbol.flags & SymbolFlags.GetAccessor) { | ||
| const getAccessorDecl = find(propertySymbol.declarations, decl => decl.kind === SyntaxKind.GetAccessor) as GetAccessorDeclaration; | ||
| const getAccessorSignature = factory.createGetAccessorDeclaration( | ||
| /*modifiers*/ undefined, | ||
| propertyName, | ||
| [], | ||
| propertyTypeNode, | ||
| /*body*/ undefined); | ||
| setCommentRange(getAccessorSignature, getAccessorDecl); | ||
| typeElements.push(getAccessorSignature); | ||
| } | ||
| if (propertySymbol.flags & SymbolFlags.SetAccessor) { | ||
| const setAccessorDecl = find(propertySymbol.declarations, decl => decl.kind === SyntaxKind.SetAccessor) as SetAccessorDeclaration; | ||
| const parameterName = setAccessorDecl?.parameters?.length > 0 ? setAccessorDecl.parameters[0].name : "arg"; | ||
| const writePropertyTypeNode = serializeTypeForDeclaration(context, getNonMissingWriteTypeOfSymbol(propertySymbol), propertySymbol, saveEnclosingDeclaration); | ||
| const setAccessorSignature = factory.createSetAccessorDeclaration( | ||
| /*modifiers*/ undefined, | ||
| propertyName, | ||
| [factory.createParameterDeclaration( | ||
| /*modifiers*/ undefined, | ||
| /*dotDotDotToken*/ undefined, | ||
| parameterName, | ||
| /*questionToken*/ undefined, | ||
| writePropertyTypeNode | ||
|
||
| )], | ||
| /*body*/ undefined); | ||
| setCommentRange(setAccessorSignature, setAccessorDecl); | ||
| typeElements.push(setAccessorSignature); | ||
| } | ||
| } | ||
| const propertySignature = factory.createPropertySignature( | ||
| modifiers, | ||
| propertyName, | ||
| optionalToken, | ||
| propertyTypeNode); | ||
| else { | ||
| const modifiers = isReadonlySymbol(propertySymbol) ? [factory.createToken(SyntaxKind.ReadonlyKeyword)] : undefined; | ||
| if (modifiers) { | ||
| context.approximateLength += 9; | ||
| } | ||
| const propertySignature = factory.createPropertySignature( | ||
| modifiers, | ||
| propertyName, | ||
| optionalToken, | ||
| propertyTypeNode); | ||
|
|
||
| typeElements.push(preserveCommentsOn(propertySignature)); | ||
| typeElements.push(preserveCommentsOn(propertySignature)); | ||
| } | ||
|
|
||
| function preserveCommentsOn<T extends Node>(node: T) { | ||
| if (some(propertySymbol.declarations, d => d.kind === SyntaxKind.JSDocPropertyTag)) { | ||
|
|
@@ -11536,6 +11569,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
| return removeMissingType(getTypeOfSymbol(symbol), !!(symbol.flags & SymbolFlags.Optional)); | ||
| } | ||
|
|
||
| function getNonMissingWriteTypeOfSymbol(symbol: Symbol) { | ||
| return removeMissingType(getWriteTypeOfSymbol(symbol), !!(symbol.flags & SymbolFlags.Optional)); | ||
| } | ||
|
|
||
| function isReferenceToType(type: Type, target: Type) { | ||
| return type !== undefined | ||
| && target !== undefined | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than reusing the
propertyTypeNode, we'd need to at least (deep) clone it - potentially putting the same node in the tree at two places can have funky results sometimes. But, better yet, would be fixing the differing get/set type bug while we're here! You can get theseter type by usinggetWriteTypeOfSymbolon thepropertySymbol, which you can then feed throughserializeTypeForDeclaration, same as is done for the reading type. You might need to duplicate thegetNonMissingTypeOfSymbolinto agetNonMissingWriteTypeOfSymbolto handle themissingtype (the specialundefined-like type we represent non-existent properties with in some compiler settings) correctly, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, @weswigham. I should be able to implement it this weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done, and I think the updated baselines demonstrate it's working. I wasn't sure if any of the
createElidedInformationPlaceholderorpropertyIsReverseMappedstuff applies to the setter type. If the answer is "yes" (or even "maybe"), let me know and I'll dig deeper. Thanks for the ongoing review!