Get references and rename to include property in the search when short hand property assignment infers the property in the declaration#15850
Conversation
…nding references at short hand property location
… property type apart from the value Any other references to binding element symbols are value references
…d property assignement and search originated in non short hand assignment location
…he property type in the variable declaration and we arent already looking at the type
…ng assignment pattern This fixes incorrectly reported types of these properties and incorrect references
| getShorthandAssignmentValueSymbol(location: Node): Symbol; | ||
| getShorthandAssignmentValueSymbol(location: ShorthandPropertyAssignment): Symbol; | ||
| getExportSpecifierLocalTargetSymbol(location: ExportSpecifier): Symbol; | ||
| getPropertySymbolOfDestructuringAssignment(location: Identifier): Symbol; |
There was a problem hiding this comment.
This is a breaking change, can you document why it's removed and what people should do instead?
| // undefined from the final type. | ||
| if (strictNullChecks && | ||
| !(getFalsyFlags(checkExpression(objectAssignmentInitializer)) & TypeFlags.Undefined)) { | ||
| sourceType = getTypeWithFacts(sourceType, TypeFacts.NEUndefined); |
There was a problem hiding this comment.
Just return getTypeWithFacts(...) early.
| // In strict null checking mode, if a default value of a non-undefined type is specified, remove | ||
| // undefined from the final type. | ||
| if (strictNullChecks && | ||
| !(getFalsyFlags(checkExpression(objectAssignmentInitializer)) & TypeFlags.Undefined)) { |
There was a problem hiding this comment.
Is it safe if checkExpression is called from inside getTypeOfNode? Could that cause duplicate diagnostics?
|
|
||
| function isDeclarationNameOfPropertyFromDestructuringAssignmentTarget(node: Node): node is PropertyName { | ||
| return (isShorthandPropertyAssignment(node.parent) || isPropertyAssignment(node.parent)) && | ||
| isObjectLiteralExpression(node.parent.parent) && |
There was a problem hiding this comment.
Is this check necessary? Maybe we should add parent: ObjectLiteral to PropertyAssignment and ShorthandPropertyAssignment.
| } | ||
|
|
||
| export function isFromDestructuringAssignmentPatternTarget(node: Node): node is AssignmentPattern { | ||
| if (isAssignmentPattern(node)) { |
| >({} = a) : any | ||
| >{} = a : any | ||
| >{} : {} | ||
| >{} : any |
There was a problem hiding this comment.
This is fine since the {} isn't even an expression and doesn't properly have a type. Maybe we should exclude these from types baselines.
| >a3 : Symbol(a3, Decl(emptyAssignmentPatterns02_ES5.ts, 1, 20)) | ||
|
|
||
| ({} = { x, y, z } = a); | ||
| >x : Symbol(x, Decl(emptyAssignmentPatterns02_ES5.ts, 3, 7)) |
| { definition: "(property) I.property1: number", ranges: [r0, r1] }, | ||
| const [, , r2] = ranges; | ||
| verify.referenceGroups(ranges, [ | ||
| { definition: "(property) I.property1: number", ranges: ranges }, |
There was a problem hiding this comment.
This includes the same range twice. That's OK for referenceGroups, but if someone uses referenceEntries they won't see that the range is in two different groups, they'll just see that it appears twice. So we should either be sure to include it only once in findAllRefs, or referenceEntries should remember to deduplicate references.
(Nit: Speaking of shorthand property assignment, you can use just , ranges }.)
| verify.referenceGroups(r1, [ | ||
| { definition: "(property) a: any", ranges: [r0] }, | ||
| { definition: "(property) a: number", ranges: [r1] } | ||
| verify.referenceGroups(ranges, [ |
There was a problem hiding this comment.
This can be verify.singleReferenceGroup("(property) a: number").
| @@ -0,0 +1,10 @@ | |||
| /// <reference path='fourslash.ts' /> | |||
There was a problem hiding this comment.
Please add another test:
/// <reference path='fourslash.ts' />
////interface I1 {
//// [|x|]: number;
////}
////interface I2 {
//// [|x|]: number;
////}
////function f({ [|x|] }: I1): I2 {
//// return { [|x|] };
////}
const ranges = test.ranges();
const [r0, r1, r2, r3] = ranges;
verify.renameLocations(r0, [r0, r2]);
verify.renameLocations(r1, [r1, r3]);
verify.renameLocations(r2, [r0, r2, r3]);
verify.renameLocations(r3, [r1, r2, r3]);This isn't ideal since the renames will still introduce compile errors, but they did before anyway.
What we really need is a way to tell the host to change it to e.g. function f({ x: y }) { return { y }; , but that's an issue for another day.
|
Should this include goToDef and breakpoints tests, since those services were changed too? Or were the changes just to keep existing tests working? |
|
Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it. |
Previously we were adding short hand property symbol in search apart from value only if search location was that shorthand property assignment.
With this PR the symbol for rename and references includes the property symbol of short hand property (apart from value) assignment in the search if it indirectly ends up inferring property in the variable which doesn't have type annotation.
Fixes #15787 and #10791