Skip to content

class field initialization order#700

Merged
Perryvw merged 4 commits intomasterfrom
bugfix/field-init-order
Aug 12, 2019
Merged

class field initialization order#700
Perryvw merged 4 commits intomasterfrom
bugfix/field-init-order

Conversation

@tomblind
Copy link
Collaborator

fixes #697

This PR also addresses issues which arose with code like this:

class Test {
    constructor(public bar = true) {}
}
const result = (new Test(false)).bar;
console.log(result);

=>

Test = {}
Test.name = "Test"
Test.__index = Test
Test.prototype = {}
Test.prototype.__index = Test.prototype
Test.prototype.constructor = Test
function Test.new(...)
    local self = setmetatable({}, Test.prototype)
    self:____constructor(...)
    return self
end
function Test.prototype.____constructor(self, bar)
    self.bar = bar or true
    if bar == nil then
        bar = true
    end
end
local result = Test.new(false).bar
print(result)

There are 2 problems here:

  • bar will incorrectly be assign true
  • The default value assignment to bar is done twice

These are corrected by placing default assignments above field initializers and removing the or true assignment to constructor fields:

function Test.prototype.____constructor(self, bar)
    if bar == nil then
        bar = true
    end
    self.bar = bar
end

@tomblind tomblind changed the title Bugfix/field init order class field initialization order Aug 11, 2019
});

test("ClassConstructorPropertyInitiailizationOrder", () => {
const result = util.transpileAndExecute(
Copy link
Member

Choose a reason for hiding this comment

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

Could you use the new test builder here?

util.testFunction`
        class Test {
            public foo = this.bar;
            constructor(public bar: string) {}
        }
        return (new Test("baz")).foo;
    `.expectToMatchJsResult();

const superCall = body.shift();
if (superCall) {
bodyWithFieldInitializers.unshift(superCall);
bodyWithFieldInitializers.push(superCall);
Copy link
Member

Choose a reason for hiding this comment

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

This looks strange in combination with the comment on line 1256.

@Perryvw Perryvw merged commit c94e070 into master Aug 12, 2019
@Perryvw Perryvw deleted the bugfix/field-init-order branch August 12, 2019 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Class properties should be initialized after constructor parameter properties

2 participants