Skip to content

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

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.

Sign up for GitHub

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

Merged
TwitchBronBron merged 12 commits into from
Jan 13, 2025
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
11626af
Add tests
TwitchBronBron Jan 6, 2025
e94e807
Fix test
luis-j-soares Jan 6, 2025
9f829f2
Make sure the builder works
luis-j-soares Jan 6, 2025
6f1cb12
Make sure the class function works too
luis-j-soares Jan 6, 2025
60ccc63
Fix bug with nested classes which don't contain "new"
luis-j-soares Jan 6, 2025
cee7507
Fix some test cases which expected the bug to happen
luis-j-soares Jan 6, 2025
780363a
Fix crash, update test case
luis-j-soares Jan 6, 2025
9bd7f77
Adjusted the test to account for mid-level ctor change
luis-j-soares Jan 6, 2025
ce8c570
Fix lint issues
luis-j-soares Jan 7, 2025
c71189b
Fix issue with missing ancestors when crossing different namespaces
luis-j-soares Jan 10, 2025
c3cfb4d
Merge branch 'master' into fix-child-class-constructor-params
TwitchBronBron Jan 13, 2025
667749a
Update src/parser/Statement.ts
TwitchBronBron Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 84 additions & 13 deletions src/files/BrsFile.Class.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ describe('BrsFile BrighterScript classes', () => {
`, undefined, 'source/main.bs');
});

it('works for simple class', () => {
it('works for simple class', () => {
testTranspile(`
class Duck
end class
Expand All @@ -470,6 +470,77 @@ describe('BrsFile BrighterScript classes', () => {
`, undefined, 'source/main.bs');
});

it('inherits the parameters of the last known constructor', () => {
testTranspile(`
class Animal
sub new(p1)
end sub
end class
class Bird extends Animal
end class
class Duck extends Bird
sub new(p1, p2)
super(p1)
m.p2 = p2
end sub
end class
class BabyDuck extends Duck
end class
`, `
function __Animal_builder()
instance = {}
instance.new = sub(p1)
end sub
return instance
end function
function Animal(p1)
instance = __Animal_builder()
instance.new(p1)
return instance
end function
function __Bird_builder()
instance = __Animal_builder()
instance.super0_new = instance.new
instance.new = sub(p1)
m.super0_new(p1)
end sub
return instance
end function
function Bird(p1)
instance = __Bird_builder()
instance.new(p1)
return instance
end function
function __Duck_builder()
instance = __Bird_builder()
instance.super1_new = instance.new
instance.new = sub(p1, p2)
m.super1_new(p1)
m.p2 = p2
end sub
return instance
end function
function Duck(p1, p2)
instance = __Duck_builder()
instance.new(p1, p2)
return instance
end function
function __BabyDuck_builder()
instance = __Duck_builder()
instance.super2_new = instance.new
instance.new = sub(p1, p2)
m.super2_new(p1, p2)
end sub
return instance
end function
function BabyDuck(p1, p2)
instance = __BabyDuck_builder()
instance.new(p1, p2)
return instance
end function
`);
});

it('registers the constructor and properly handles its parameters', () => {
testTranspile(`
class Duck
Expand Down Expand Up @@ -569,8 +640,8 @@ describe('BrsFile BrighterScript classes', () => {
function __Duck_builder()
instance = __Creature_builder()
instance.super0_new = instance.new
instance.new = sub()
m.super0_new()
instance.new = sub(name as string)
m.super0_new(name)
end sub
instance.super0_sayHello = instance.sayHello
instance.sayHello = function(text)
Expand All @@ -581,9 +652,9 @@ describe('BrsFile BrighterScript classes', () => {
end function
return instance
end function
function Duck()
function Duck(name as string)
instance = __Duck_builder()
instance.new()
instance.new(name)
return instance
end function
`, 'trim', 'source/main.bs'
Expand Down Expand Up @@ -774,8 +845,8 @@ describe('BrsFile BrighterScript classes', () => {
function __Duck_builder()
instance = __Animal_builder()
instance.super0_new = instance.new
instance.new = sub()
m.super0_new()
instance.new = sub(name as string)
m.super0_new(name)
end sub
instance.super0_move = instance.move
instance.move = sub(distanceInMeters as integer)
Expand All @@ -784,16 +855,16 @@ describe('BrsFile BrighterScript classes', () => {
end sub
return instance
end function
function Duck()
function Duck(name as string)
instance = __Duck_builder()
instance.new()
instance.new(name)
return instance
end function
function __BabyDuck_builder()
instance = __Duck_builder()
instance.super1_new = instance.new
instance.new = sub()
m.super1_new()
instance.new = sub(name as string)
m.super1_new(name)
end sub
instance.super1_move = instance.move
instance.move = sub(distanceInMeters as integer)
Expand All @@ -802,9 +873,9 @@ describe('BrsFile BrighterScript classes', () => {
end sub
return instance
end function
function BabyDuck()
function BabyDuck(name as string)
instance = __BabyDuck_builder()
instance.new()
instance.new(name)
return instance
end function

Expand Down
65 changes: 58 additions & 7 deletions src/parser/Statement.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable no-bitwise */
import type { Token, Identifier } from '../lexer/Token';
import { CompoundAssignmentOperators, TokenKind } from '../lexer/TokenKind';
import type { BinaryExpression, NamespacedVariableNameExpression, FunctionExpression, FunctionParameterExpression, LiteralExpression } from './Expression';
import { type BinaryExpression, type NamespacedVariableNameExpression, FunctionExpression, type FunctionParameterExpression, type LiteralExpression } from './Expression';
import { CallExpression, VariableExpression } from './Expression';
import { util } from '../util';
import type { Range } from 'vscode-languageserver';
Expand All @@ -11,7 +11,7 @@
import { InternalWalkMode, walk, createVisitor, WalkMode, walkArray } from '../astUtils/visitors';
import { isCallExpression, isCommentStatement, isEnumMemberStatement, isExpression, isExpressionStatement, isFieldStatement, isFunctionExpression, isFunctionStatement, isIfStatement, isInterfaceFieldStatement, isInterfaceMethodStatement, isInvalidType, isLiteralExpression, isMethodStatement, isNamespaceStatement, isTypedefProvider, isUnaryExpression, isVoidType } from '../astUtils/reflection';
import type { TranspileResult, TypedefProvider } from '../interfaces';
import { createInvalidLiteral, createMethodStatement, createToken } from '../astUtils/creators';
import { createIdentifier, createInvalidLiteral, createMethodStatement, createToken } from '../astUtils/creators';
import { DynamicType } from '../types/DynamicType';
import type { BscType } from '../types/BscType';
import type { TranspileState } from './TranspileState';
Expand Down Expand Up @@ -2173,6 +2173,23 @@
}) as MethodStatement;
}

/**
* Return the parameters for the first constructor function for this class
* @param ancestors The list of ancestors for this class
* @returns The parameters for the first constructor function for this class
*/
private getConstructorParams(ancestors: ClassStatement[]) {
for (let ancestor of ancestors) {
// @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 •
edited
Loading

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

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

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

@TwitchBronBron TwitchBronBron Jan 10, 2025

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.

if (ancestor) {
const ctor = ancestor.getConstructorFunction();
if (ctor) return ctor.func.parameters;

Check failure on line 2187 in src/parser/Statement.ts

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest) 

Expected { after 'if' condition
}
}
return [];
}

/**
* Determine if the specified field was declared in one of the ancestor classes
*/
Expand Down Expand Up @@ -2226,10 +2243,38 @@
let body = this.body;
//inject an empty "new" method if missing
if (!this.getConstructorFunction()) {
body = [
createMethodStatement('new', TokenKind.Sub),
...this.body
];
if (ancestors.length == 0) {

Check failure on line 2246 in src/parser/Statement.ts

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest) 

Expected '===' and instead saw '=='
body = [
createMethodStatement('new', TokenKind.Sub),
...this.body
];
} else {
const params = this.getConstructorParams(ancestors);
const call = new ExpressionStatement(
new CallExpression(
new VariableExpression(createToken(TokenKind.Identifier, 'super')),
createToken(TokenKind.LeftParen),
createToken(TokenKind.RightParen),
params.map(x => new VariableExpression(x.name))
)
);
body = [
new MethodStatement(
[],
createIdentifier('new'),
new FunctionExpression(
params.map(x => x.clone()),
new Block([call]),
createToken(TokenKind.Sub),
createToken(TokenKind.EndSub),
createToken(TokenKind.LeftParen),
createToken(TokenKind.RightParen)
),
null
),
...this.body
];
}
}

for (let statement of body) {
Expand Down Expand Up @@ -2289,8 +2334,14 @@
*/
private getTranspiledClassFunction(state: BrsTranspileState) {
let result = [] as TranspileResult;

const constructorFunction = this.getConstructorFunction();
const constructorParams = constructorFunction ? constructorFunction.func.parameters : [];
let constructorParams = [];
if (constructorFunction) {
constructorParams = constructorFunction.func.parameters;
} else {
constructorParams = this.getConstructorParams(this.getAncestors(state));
}

result.push(
state.sourceNode(this.classKeyword, 'function'),
Expand Down
Loading