Interfaces, Abstract Methods, and Virtual Overloading #862

willemneal · 2019-09-22T17:54:36Z

willemneal · 2019-09-22T17:55:08Z

@MaxGraey How do I mark this as a draft?

MaxGraey · 2019-09-22T19:18:56Z

I'm afraid it could only be done while setting/open PR at first time

willemneal · 2019-09-22T19:23:30Z

Ah okay thanks I'll check that out next time. I've been using gitkraken to open the PR's so I'll switch to the github interface.

src/ast.ts

willemneal · 2019-09-22T20:22:47Z

@MaxGraey and @dcodeIO My new commits are not being tested on travis; do you of any reason why? Have we made the switch to github Actions?

src/ast.ts

TODO: add better error messages

Still need to move logic into the interface methods.

Soon to add iterator

src/program.ts

dcodeIO · 2019-12-17T14:07:42Z

src/program.ts

@@ -3408,6 +3438,9 @@ export class Class extends TypedElement {
  /** Tests if a value of this class type is assignable to a target of the specified class type. */
  isAssignableTo(target: Class): bool {
    var current: Class | null = this;
+    if (target.kind == ElementKind.INTERFACE) {
+       return true;


This looks wrong?

This is because checking the interface requires resolving function prototypes. Which is why the check happens in the compiler method. This was an artifact as the condition in the compiler's method checks if the target is an interface so this never actually gets triggered. That is my least favorite part of this solution since it should happen in this method.

dcodeIO · 2019-12-17T14:08:27Z

src/program.ts

+  addImplementer(_class: Class): void {
+    this.implementers.add(_class);
+    if (this.base != null) {
+      (<Class>this.base).addImplementer(_class);


What if base is null, should that maybe hit an assertion?

A class on interface that doesn't extend anything has a base that is null. So it's the "base case" in the recursion. Should we instead make an Object class the default ?

src/program.ts

dcodeIO · 2019-12-17T14:09:35Z

src/program.ts

+    iDecl.typeParameters = cDecl.typeParameters;
+    iDecl.extendsType = cDecl.extendsType;
+    iDecl.implementsTypes = cDecl.implementsTypes;
+    iDecl.members = cDecl.members;


Seems like there's more to this than these properties, like the source range

dcodeIO · 2019-12-17T14:10:09Z

src/program.ts

-    typeArguments: Type[] = [],
-    base: Interface | null = null
+    typeArguments: Type[] | null = [],
+    base: Interface | Class | null = null // interface can extend classes in typescript


There are no union types when compiling this as AS

Yeah it should just be class.

src/program.ts

dcodeIO · 2019-12-17T14:14:22Z

src/util/collections.ts

+
+export function notNull<T>(t: T): bool {
+  return t != null;
+}


Not a fan of these, as it's trading convenience for the overhead of indirect calls. Would prefer to inline.

Yeah I never end up using these, did at first for the convenience but removed most used of iterators after first batch of feedback.

Yeah I never ended up using these after removing most of my use of iterations in my first iteration. I'll remove them.

src/compiler.ts

dcodeIO · 2019-12-17T14:17:52Z

src/compiler.ts

+    // this.error(
+    //   DiagnosticCode.Operation_not_supported,
+    //   declaration.range
+    // );


Is there missing something here?

Ah this could be used to merge multiple interface declarations.

dcodeIO · 2019-12-17T14:18:14Z

src/compiler.ts

+          DiagnosticCode.Type_0_is_not_assignable_to_type_1,
+          reportNode.range,
+          fromType.toString(),
+          toType.toString());


Indentation

src/compiler.ts

dcodeIO · 2019-12-17T14:20:11Z

src/compiler.ts

@@ -9217,12 +9280,394 @@ export class Compiler extends DiagnosticEmitter {
    return module.block(label, conditions, NativeType.I32);
  }

+  checkInterfaceImplementation(toType: Type, fromType: Type, reportNode: Node): bool {
+    const _interface = (<Interface>toType.classReference!);
+    const _class = fromType.classReference!;


Looks like these should be assert calls

dcodeIO · 2019-12-17T14:21:37Z

src/compiler.ts

+            DiagnosticCode.Type_0_is_not_assignable_to_type_1,
+            (<DeclaredElement>mem).declaration.range,
+            mem.name,
+            name);


Indentation; also looks like the error won't be very useful as it doesn't tell what's actually wrong?

I just went off the compiler errors I got from typescript. In fact this should result in a parsing error, so I'll look into that.

dcodeIO · 2019-12-17T14:21:52Z

src/compiler.ts

+            name);
+          continue;
+        }
+        // Error


Is there something missing here?

dcodeIO · 2019-12-17T14:23:50Z

src/compiler.ts

+          mem = (<Field>mem).prototype;
+          imem = (<Property>imem).prototype;
+        }
+        case ElementKind.FIELD_PROTOTYPE: {


Mixing prototypes and instances here seems odd, like if it's missing a distinction of static vs instance.

dcodeIO · 2019-12-17T14:24:19Z

src/compiler.ts

+        }
+        incorrectMember = false;
+    }
+      error = error || incorrectMember;// case where contiune skips the last check.


dcodeIO · 2019-12-17T14:24:26Z

src/compiler.ts

+          _class.identifierNode.range,
+            fromType.toString(),
+            toType.toString()
+            );


Indentation

dcodeIO · 2019-12-17T14:27:13Z

src/compiler.ts

+        relooper.addBranchForSwitch(first, returnBlock, classIds);
+      }
+
+      // finish relooper and prepare body of function


Wondering if the relooper will potentially build huge br_tables here, for instance when dealing with ids 1 to 1024 - do we get 1024 cases? Other code intentionally generates sequences of br_ifs so Binaryen can decide if it's worth to make a br_table or leave these as is depending on optimize and shrink levels.

src/compiler.ts

dcodeIO · 2019-12-17T14:45:59Z

Overall this appears somewhat rushed to me, with various minor problems, some oversights and missing comments to understand why it's built exactly this way. Don't get me wrong please, I appreciate your efforts, but I have a feeling that getting this right will be hard.

Fails interface-fail

MaxGraey reviewed Sep 22, 2019

View reviewed changes

src/ast.ts Outdated Show resolved Hide resolved

willemneal force-pushed the virtual branch 4 times, most recently from be5d99e to e918e48 Compare September 23, 2019 00:41

willemneal mentioned this pull request Sep 23, 2019

A mechanism for virtual overloads #693

Closed

dcodeIO reviewed Sep 23, 2019

View reviewed changes

src/ast.ts Outdated Show resolved Hide resolved

dcodeIO reviewed Sep 23, 2019

View reviewed changes

src/ast.ts Outdated Show resolved Hide resolved

willemneal and others added 16 commits October 1, 2019 18:38

temp

7ce5709

compiles

08f6dfe

Works but virtual table is hard coded

6b58579

Added relooper table and if cases

f704437

Fixed tests

3864d7e

Update arrayEquals

2904951

Test email again

e09ce14

Adding Email to Notice to pass test

e58342f

Add test for structurally interface

2a17582

Improve type checking

a3aef9c

TODO: add better error messages

Added error messages

99403b8

Reduce number of unreachable instructions

21c0227

remove file [skip ci]

e5d198c

Remove map and iterator usage

5fb758f

Type checking improved, but virtual table needs work

839aa13

Better type checking and virtual table works

099cb11

Still need to move logic into the interface methods.

willemneal force-pushed the virtual branch from a0eb70b to 099cb11 Compare October 1, 2019 22:39

Add from to portable definition

712d400

Soon to add iterator