Fix isPropertyDeclaredInAncestorClass function
#2376
Conversation
| return false | ||
| } | ||
| classType := c.getDeclaredTypeOfSymbol(prop.Parent) | ||
| for { |
There was a problem hiding this comment.
The loop here isn't necessary since getPropertyOfType on the base class will get any inherited property (and not just a property in the immediate base class as the loop seems to imply).
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix issue #2284 by refactoring the isPropertyDeclaredInAncestorClass function to use a more concise implementation. However, the refactored code introduces a critical regression by only checking the immediate parent class instead of traversing the entire inheritance hierarchy.
Key changes:
- Simplified the
isPropertyDeclaredInAncestorClassfunction logic - Added test case
checkInheritedProperty.tsto verify property initialization order checking - Added corresponding baseline files for the test
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
testdata/tests/cases/compiler/checkInheritedProperty.ts |
New test case checking that property 'b' used before initialization produces an error when extending a factory constructor |
testdata/baselines/reference/compiler/checkInheritedProperty.types |
Expected type information baseline for the new test |
testdata/baselines/reference/compiler/checkInheritedProperty.symbols |
Expected symbol information baseline for the new test |
testdata/baselines/reference/compiler/checkInheritedProperty.errors.txt |
Expected error baseline showing TS2729 error for property used before initialization |
internal/checker/checker.go |
Refactored isPropertyDeclaredInAncestorClass to a more concise form, but removed the loop that checks the full inheritance chain |
| if prop.Parent.Flags&ast.SymbolFlagsClass != 0 { | ||
| if baseTypes := c.getBaseTypes(c.getDeclaredTypeOfSymbol(prop.Parent)); len(baseTypes) != 0 { | ||
| superProperty := c.getPropertyOfType(baseTypes[0], prop.Name) | ||
| return superProperty != nil && superProperty.ValueDeclaration != nil |
There was a problem hiding this comment.
The refactored implementation only checks the immediate base type and no longer traverses the entire inheritance hierarchy. The original code correctly looped through all ancestor classes to check if the property was declared in any ancestor. This change will cause incorrect behavior when a property is declared in a grandparent or higher ancestor class, as those declarations will not be detected.
The loop should be restored to properly check all ancestors in the inheritance chain, not just the immediate parent.
There was a problem hiding this comment.
Wrong. getPropertyOfType on the immediate base type checks all inherited properties, not just those from the immediate base. Thus, the loop is unnecessary and actually causes a panic when a base type is an intersection.
jakebailey
left a comment
jakebailey
left a comment
There was a problem hiding this comment.
@camc314 this should allow you to revert oxc-project/tsgolint#504
|
nice, thank you 🙏 |
|
@camc314 When you get a chance, can you verify that this has fixed the issue for you? |
|
Yep, this has fixed the panic, thank you! |
Fixes #2284.