Skip to content

Comments

Fix class transpile issue with child class constructor not inherriting parent params#1390

Merged
TwitchBronBron merged 12 commits intomasterfrom
fix-child-class-constructor-params
Jan 13, 2025
Merged

Fix class transpile issue with child class constructor not inherriting parent params#1390
TwitchBronBron merged 12 commits intomasterfrom
fix-child-class-constructor-params

Conversation

@TwitchBronBron
Copy link
Member

@TwitchBronBron TwitchBronBron commented Jan 6, 2025

When a child class extends a super class, but does not include its own new function, and the parent class has parameters, we are missing those parameters in the child class's factory.

  • unit test
  • fix the bug

@TwitchBronBron TwitchBronBron added help wanted Extra attention is needed good first issue Good for newcomers labels Jan 6, 2025
Comment on lines 2183 to 2184
// @todo: somehow, ancestors can have a list where the last element is null.
// this is exposed by the test named "extending namespaced class transpiles properly".
Copy link
Collaborator

@luis-j-soares luis-j-soares Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's likely to be a bug in getAncestors, specifically with this part:

const namespace = this.findAncestor<NamespaceStatement>(isNamespaceStatement);
stmt = state.file.getClassFileLink(
    stmt.parentClassName.getName(ParseMode.BrighterScript),
    namespace?.getName(ParseMode.BrighterScript)
)?.item;
ancestors.push(stmt);

In some cases (namely the test mentioned in the comment), stmt is null and yet it still gets pushed to the result array. I'll try and figure out where exactly it's becoming null.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the problem is:

this.findAncestor<NamespaceStatement>(isNamespaceStatement)

Which should actually be:

stmt.findAncestor<NamespaceStatement>(isNamespaceStatement)

@TwitchBronBron is it okay if I make that change in this PR? Or would you rather have a separate one for this fix?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fine, go ahead and fix it in this PR.

@TwitchBronBron TwitchBronBron merged commit ccb4649 into master Jan 13, 2025
6 checks passed
@TwitchBronBron TwitchBronBron deleted the fix-child-class-constructor-params branch January 13, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

good first issue Good for newcomers help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants