GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Implement calls to 'super()' by dcodeIO · Pull Request #445 · AssemblyScript/assemblyscript

Conversation

@dcodeIO

@dcodeIO dcodeIO commented Jan 30, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Passes a simple test and implements quite a few related checks. Still possible that not every single edge case is covered, but having this now and fixing any bugs as we go appears reasonable, considering that class support is still in the works anyway.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@dcodeIO
Implement calls to 'super()'
6e3c348
@MaxGraey

MaxGraey commented Jan 30, 2019

Copy link
Copy Markdown
Member

It seems this fixing also #375, #245, #199 and #187.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO
check that super() is the first statement in constructors of derived …
a749af9
…classes
MaxGraey
MaxGraey reviewed Jan 30, 2019
View reviewed changes
Comment thread src/compiler.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

dcodeIO added 2 commits January 30, 2019 18:14
@dcodeIO
also check that super calls are the first statement within their cons…
92ac406
…tructor when compiling them
@dcodeIO
fix
c5fc6c6
@dcodeIO
dcodeIO force-pushed the call-super branch from 76a5710 to c5fc6c6 Compare January 30, 2019 17:18
@MaxGraey

MaxGraey commented Jan 30, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Hmm, I'm not sure about first statement call rule. It make sense only if this or super keyword using before super call. But should be valid for this case:

constructor(a: i32) {
   var arg = a + 1;
   super(arg);
}
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO

dcodeIO commented Jan 30, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Yeah, there are the ALLOCATES and CONDITIONALLY_ALLOCATES flags in flows for that. Will probably need a CALLS_SUPER one as well then to do it this way. TS also has

    "A 'super' call must be the first statement in the constructor when a class contains initialized properties or has parameter properties.": {
        "category": "Error",
        "code": 2376
    },

Maybe we could use

    "'super' must be called before accessing 'this' in the constructor of a derived class.": {
        "category": "Error",
        "code": 17009
    },
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO
add diagnostic for when trying to call super outside of a ctor
8b065bf
@MaxGraey

MaxGraey commented Jan 30, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Maybe we could use

Yeah

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dcodeIO added 2 commits January 30, 2019 19:17
@dcodeIO
utilize flow to check for super conditions
891d654
@dcodeIO
fix
2af8c22
@dcodeIO

dcodeIO commented Jan 30, 2019

Copy link
Copy Markdown
Member Author

Seems

constructor(a: i32) {
   var arg = a + 1;
   super(arg);
}

is rejected by TS, but now legalized in AS with the flow changes above. Still needs a check when doing super.something that super has already been called.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dcodeIO added 2 commits January 30, 2019 19:32
@dcodeIO
check that super has been called before accessing it otherwise
50f2d6a
@dcodeIO
validate super syntax in parser
f2f03fc
@MaxGraey

MaxGraey commented Jan 30, 2019

Copy link
Copy Markdown
Member

Hmm, really? That's interesting...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MaxGraey

MaxGraey commented Jan 30, 2019

Copy link
Copy Markdown
Member

Yeah, but PR for fixing this in TS already exists: microsoft/TypeScript#29374

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO

dcodeIO commented Jan 30, 2019

Copy link
Copy Markdown
Member Author

Yeah, I remember that this limitation annoyed me in the past. Especially given the dynamic nature of JavaScript this seems like an arbitrary restriction (not directly related to AS, where such a restriction might actually make sense).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MaxGraey

MaxGraey commented Jan 30, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

As I know this legal in any OOP typed languages may be except C#. It seems typescript just missing this ability. But this should be fix soon

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO
minor cleanup
94fbbc7
MaxGraey
MaxGraey reviewed Jan 30, 2019
View reviewed changes
Comment thread src/program.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO

dcodeIO commented Jan 30, 2019

Copy link
Copy Markdown
Member Author

One thing that's still particularly off is that derived classes currently inherit their super class's fields, and thus initialize super fields in each derived constructor, which is odd. Instead, each constructor should initialize its own class's fields right after bubbling up the entire prototype chain, calling all the overloaded constructors sequentially. One reason why this doesn't yet work is that classes that don't have an explicit constructor also don't have a wasm function for it (their field initialization is instead inlined), which we should change by generating actual constructor functions for them that we can call.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MaxGraey

MaxGraey commented Jan 30, 2019

Copy link
Copy Markdown
Member

Makes sense. But it look like not simple. May be better leave this for other PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO
remove tinkering
ef57350
@dcodeIO

dcodeIO commented Jan 31, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Sure, makes sense to leave that for a separate PR. One example what's not possible until then:

class A {
  a: i32 = 1;
  constructor() {
    assert(this.a == 1); // == 0, is uninitialized, and remains so
  }
}

class B extends A {
  a: i32 = 3;
  constructor() {
    super();
  }
}

new B();

This is currently prevented by rejecting duplicate field names.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO

dcodeIO commented Jan 31, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Similarly:

class A {
  a: i32 = 1;
  constructor() {}
}

class B extends A {
  b: i32 = 2;
}

var b = new B();
assert(b.b == 2); // == 0, uninitialized due to A#constructor being used, with no B#constructor

Not directly related to super(), but part of the same problem.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO
more tests
5883ec9
@dcodeIO

dcodeIO commented Jan 31, 2019

Copy link
Copy Markdown
Member Author

Merging, continuing in a separate PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcodeIO
dcodeIO merged commit 75328f3 into master Jan 31, 2019
@dcodeIO dcodeIO mentioned this pull request Jan 31, 2019
Rework constructor handling #446
Merged
@robrohan

robrohan commented Feb 5, 2019

Copy link
Copy Markdown

New to the assemblyscript project, and coming here because I hit this bug: #245 Does this mean that OO stuff in general isn't supported by assembly script at the moment?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MaxGraey

MaxGraey commented Feb 5, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@robrohan Basic OO supports except virtual methods. Issue which you mentioned is not yet fixed. If you add some params to basic constructor it would work

robrohan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MaxGraey MaxGraey mentioned this pull request Feb 7, 2019
The concrete class achieve the base class field value was incorrect. #199
Closed
@dcodeIO
dcodeIO deleted the call-super branch March 7, 2019 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MaxGraey MaxGraey MaxGraey left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@dcodeIO @MaxGraey @robrohan
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.