Fix find all references of inherited constructor#30514
Conversation
| verify.referenceGroups(aCtr, [ | ||
| { definition: "class A", ranges: [aCtr, aNew] }, | ||
| { definition: "class B", ranges: [cSuper, bNew]}, | ||
| { definition: "class D", ranges: [dNew]}]); |
There was a problem hiding this comment.
Can you please add tests for finding constructors for all other classes as well. Also tests for constructor invocation locations as well
There was a problem hiding this comment.
I tried to do this but find all references only understands that we are looking for a constructor reference if we call it on a constructor declaration. So if we call find all references on a constructor invocation like new B(), it will return references to the class B. I'm not sure if this is the expected behavior and don't know how to write those tests. Should I open an issue about this? @sandersn @sheetalkamat
There was a problem hiding this comment.
You might as well open an issue, although this seems like something that might have an existing issue.
src/services/findAllReferences.ts
Outdated
| } | ||
|
|
||
| function findInheritedConstructorReferences(classDeclaration: ClassLikeDeclaration, state: State): void { | ||
| if (hasOwnConstructor(classDeclaration)) return undefined; |
There was a problem hiding this comment.
Don't you need to look if this class is extending another class and look for its constructor even though this doesn't have constructor?
There was a problem hiding this comment.
I think this line quits early in exactly the case that find-all-refs is called on B below:
class A { constructor() { } }
class B extends A { constructor () { } }That's because B does have its own constructor.
There was a problem hiding this comment.
Yes. The reasoning is that this function will only be called when we are looking for a class' (A) constructor and we find a reference that is like class B extends A. So I think it is enough to check if B doesn't have is own constructor declared.
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
Looks good, just need the tests @sheetalkamat suggested.
src/services/findAllReferences.ts
Outdated
| } | ||
|
|
||
| function findInheritedConstructorReferences(classDeclaration: ClassLikeDeclaration, state: State): void { | ||
| if (hasOwnConstructor(classDeclaration)) return undefined; |
There was a problem hiding this comment.
I think this line quits early in exactly the case that find-all-refs is called on B below:
class A { constructor() { } }
class B extends A { constructor () { } }That's because B does have its own constructor.
3 participants
Fixes #30216