[inliner] change type of abstract `this` expression to preserve it (closes #2236) (closes #3713)

optimizer.ml

@@ -285,10 +285,14 @@ let rec type_inline ctx cf f ethis params tret config p ?(self_calling_closure=f
 			if v.v_type != t_dynamic && follow e.etype == t_dynamic then (local v).i_write <- true;
 			(match e.eexpr, opt with
 			| TConst TNull , Some c -> mk (TConst c) v.v_type e.epos
-			(* we have to check for abstract casts here because we can't do that later. However, we have to skip the check for the
-			   first argument of abstract implementation functions. *)
-			(* actually we don't because unify_call_args takes care of that anyway *)
-			(* | _ when not (first && Meta.has Meta.Impl cf.cf_meta && cf.cf_name <> "_new") -> (!cast_or_unify_ref) ctx (map_type v.v_type) e e.epos *)
+			(*
+				This is really weird and should be reviewed again. The problem is that we cannot insert a TCast here because
+				the abstract `this` value could be written to, which is not possible if it is wrapped in a cast.
+
+				The original problem here is that we do not generate a temporary variable and thus mute the type of the
+				`this` variable, which leads to unification errors down the line. See issues #2236 and #3713.
+			*)
+			| _ when first && (Meta.has Meta.Impl cf.cf_meta) -> {e with etype = v.v_type}
 			| _ -> e) :: loop pl al false
 		| [], (v,opt) :: al ->
 			(mk (TConst (match opt with None -> TNull | Some c -> c)) v.v_type p) :: loop [] al false

tests/optimization/run.hxml

@@ -14,4 +14,5 @@
 --macro Macro.register('Test')
 --macro Macro.register('TestJs')
 --macro Macro.register('TestLocalDce')
+--macro Macro.register('issues')
 -dce std

tests/optimization/src/Macro.hx

@@ -15,8 +15,20 @@ class Macro {
 		if (classes.length == 0) {
 			Context.onAfterGenerate(run);
 		}
-		Context.getType(className);
-		classes.push(className);
+		if (className.charAt(0).toLowerCase() == className.charAt(0)) {
+			var dir = sys.FileSystem.readDirectory("src/" +className.replace(".", "/"));
+			for (file in dir) {
+				if (file.endsWith(".hx")) {
+					var name = className + "." + file.substr(0, -3);
+					Context.getType(name);
+					classes.push(name);
+
+				}
+			}
+		} else {
+			Context.getType(className);
+			classes.push(className);
+		}
 	}
 
 	static function run() {

tests/optimization/src/issues/Issue2236.hx

@@ -0,0 +1,50 @@
+package issues;
+
+private class ArrayIterator<T> {
+    var a : Array<T>;
+    var pos : Int;
+    public inline function new(a) {
+        this.a = a;
+        this.pos = 0;
+    }
+    public inline function hasNext() {
+        return pos < a.length;
+    }
+    public inline function next() {
+        return a[pos++];
+    }
+}
+
+private abstract ArrayRead<T>(Array<T>) {
+
+    public inline function new(a) {
+        this = a;
+    }
+
+    function toArray() : Array<T> {
+        return this;
+    }
+
+    public inline function iterator() : ArrayIterator<T> {
+        return new ArrayIterator(this);
+    }
+
+}
+
+class Issue2236 {
+	@:js('
+		var a = [0];
+		var _g_a = a;
+		var _g_pos = 0;
+		while(_g_pos < _g_a.length) {
+			var x = _g_a[_g_pos++];
+			x;
+		}
+	')
+	static function test() {
+		var a = new ArrayRead([0]);
+		for( x in a ) {
+			x;
+		}
+	}
+}

tests/optimization/src/issues/Issue3713.hx

@@ -0,0 +1,29 @@
+package issues;
+
+private class A<T> {
+    public function new() {}
+}
+
+private abstract B<T>(A<T>) {
+    public function new() this = new A();
+    public inline function f():C<T> return new C(this);
+}
+
+private class C<T> {
+	var x:Int;
+    public inline function new(d:A<T>) {
+		x = 1;
+	}
+}
+
+class Issue3713 {
+	@:js('
+		var b = issues._Issue3713.B_Impl_._new();
+		var c_x = 1;
+	')
+	@:analyzer(no_const_propagation, no_local_dce, no_check_has_effect)
+	static function test() {
+        var b = new B<Int>();
+        var c = b.f();
+	}
+}

tests/unit/src/unit/issues/Issue2236.hx

@@ -0,0 +1,41 @@
+package unit.issues;
+
+private class ArrayIterator<T> {
+    var a : Array<T>;
+    var pos : Int;
+    public inline function new(a) {
+        this.a = a;
+        this.pos = 0;
+    }
+    public inline function hasNext() {
+        return pos < a.length;
+    }
+    public inline function next() {
+        return a[pos++];
+    }
+}
+
+private abstract ArrayRead<T>(Array<T>) {
+
+    public inline function new(a) {
+        this = a;
+    }
+
+    function toArray() : Array<T> {
+        return this;
+    }
+
+    public inline function iterator() : ArrayIterator<T> {
+        return new ArrayIterator(this);
+    }
+
+}
+
+class Issue2236 extends Test {
+	function test() {
+		var a = new ArrayRead([0]);
+		for( x in a ) {
+
+		}
+	}
+}

tests/unit/src/unit/issues/Issue3713.hx

@@ -0,0 +1,21 @@
+package unit.issues;
+
+private class A<T> {
+    public function new() {}
+}
+
+private abstract B<T>(A<T>) {
+    public inline function new() this = new A();
+    public inline function f():C<T> return new C(this);
+}
+
+private class C<T> {
+    public inline function new(d:A<T>) {}
+}
+
+class Issue3713 extends Test {
+	function test() {
+        var b = new B<Int>();
+        var c = b.f();
+	}
+}