Fix null coal assign (with less code duplication) (#11980)

* Fix null coal assign

* Can i extract this TBinop?

* Nope, so there is more tests

* start implementing the ??= stuff

* turn into real errors so we know what's happening

* adjust test for now

* implement AKField

* implement AKAccessor

* implement AKUsingAccessor

* implement AKResolve

* try something

* fix and adjust

---------

Co-authored-by: RblSb <[email protected]>

src/typing/operators.ml

@@ -650,42 +650,38 @@ let process_lhs_expr ctx name e_lhs =
 	let e = vr#get_expr name e_lhs in
 	e,vr
 
-let type_assign_op ctx op e1 e2 with_type p =
-	let field_rhs_by_name op name ev with_type =
+type 'a assign_op_api = {
+	akno_fallback : unit -> texpr;
+	type_rhs : texpr -> expr -> 'a;
+	to_texpr : value_reference -> 'a -> (texpr -> texpr) -> texpr;
+	generate : value_reference -> texpr -> texpr -> texpr;
+	assign : value_reference -> texpr -> 'a -> texpr;
+}
+
+let handle_assign_op ctx api e1 e2 with_type p =
+	let field_rhs_by_name name ev with_type =
 		let access_get = type_field_default_cfg ctx ev name p MGet with_type in
 		let e_get = acc_get ctx access_get in
-		e_get.etype,type_binop2 ctx op e_get e2 true WithType.value p
+		e_get,api.type_rhs e_get e2
 	in
-	let field_rhs op cf ev =
-		field_rhs_by_name op cf.cf_name ev (WithType.with_type cf.cf_type)
+	let field_rhs cf ev =
+		field_rhs_by_name cf.cf_name ev (WithType.with_type cf.cf_type)
 	in
-	let assign vr e r_rhs =
-		if BinopResult.needs_assign r_rhs then check_assign ctx e;
+	let set vr fa e_lhs r_rhs el =
 		let assign e_rhs =
-			let e_rhs = AbstractCast.cast_or_unify ctx e.etype e_rhs p in
-			match e_rhs.eexpr with
-			| TBinop(op',e1',e2') when op = op' && Texpr.equal e e1' ->
-				mk (TBinop(OpAssignOp op',e1',e2')) e.etype p
-			| _ ->
-				mk (TBinop(OpAssign,e,e_rhs)) e.etype p
-		in
-		let e = BinopResult.to_texpr vr r_rhs assign in
-		vr#to_texpr e
-	in
-	let set vr fa t_lhs r_rhs el =
-		let assign e_rhs =
-			let e_rhs = AbstractCast.cast_or_unify ctx t_lhs e_rhs p in
+			let e_rhs = AbstractCast.cast_or_unify ctx e_lhs.etype e_rhs p in
 			let dispatcher = new call_dispatcher ctx (MSet (Some e2)) with_type p in
 			dispatcher#accessor_call fa (el @ [e_rhs]) [];
 		in
-		let e = BinopResult.to_texpr vr r_rhs assign in
-		vr#to_texpr e
+		let e = api.to_texpr vr r_rhs assign in
+		api.generate vr e_lhs e
 	in
 	let rec loop acc = match acc with
 		| AKNo(_,p) ->
 			(* try abstract operator overloading *)
 			begin try
-				type_non_assign_op ctx op e1 e2 true true with_type p
+				api.akno_fallback();
+				(* type_non_assign_op ctx op e1 e2 true true with_type p *)
 			with Not_found ->
 				raise_typing_error "This expression cannot be accessed for writing" p
 			end
@@ -695,23 +691,23 @@ let type_assign_op ctx op e1 e2 with_type p =
 			raise_typing_error "Invalid operation" p
 		| AKExpr e ->
 			let e,vr = process_lhs_expr ctx "lhs" e in
-			let e_rhs = type_binop2 ctx op e e2 true WithType.value p in
-			assign vr e e_rhs
+			let e_rhs = api.type_rhs e e2 in
+			api.assign vr e e_rhs
 		| AKField fa ->
 			let vr = new value_reference ctx in
 			let ef = vr#get_expr_part "fh" fa.fa_on in
-			let _,e_rhs = field_rhs op fa.fa_field ef in
+			let _,e_rhs = field_rhs fa.fa_field ef in
 			let e_lhs = FieldAccess.get_field_expr {fa with fa_on = ef} FWrite in
-			assign vr e_lhs e_rhs
+			api.assign vr e_lhs e_rhs
 		| AKAccessor fa ->
 			let vr = new value_reference ctx in
 			let ef = vr#get_expr_part "fh" fa.fa_on in
-			let t_lhs,e_rhs = field_rhs op fa.fa_field ef in
-			set vr {fa with fa_on = ef} t_lhs e_rhs []
+			let e_lhs,e_rhs = field_rhs fa.fa_field ef in
+			set vr {fa with fa_on = ef} e_lhs e_rhs []
 		| AKUsingAccessor sea ->
 			let fa = sea.se_access in
 			let ef,vr = process_lhs_expr ctx "fh" sea.se_this in
-			let t_lhs,e_rhs = field_rhs op fa.fa_field ef in
+			let t_lhs,e_rhs = field_rhs fa.fa_field ef in
 			set vr sea.se_access t_lhs e_rhs [ef]
 		| AKAccess(a,tl,c,ebase,ekey) ->
 			let cf_get,tf_get,r_get,ekey = AbstractCast.find_array_read_access ctx a tl ekey p in
@@ -724,16 +720,16 @@ let type_assign_op ctx op e1 e2 with_type p =
 			in
 			let ebase = maybe_bind_to_temp "base" ebase in
 			let ekey = maybe_bind_to_temp "key" ekey in
-			let eget = mk_array_get_call ctx (cf_get,tf_get,r_get,ekey) c ebase p in
-			let eget = type_binop2 ctx op eget e2 true WithType.value p in
-			let eget = BinopResult.to_texpr vr eget (fun e -> e) in
+			let eread = mk_array_get_call ctx (cf_get,tf_get,r_get,ekey) c ebase p in
+			let eget = api.type_rhs eread e2 in
+			let eget = api.to_texpr vr eget (fun e -> e) in
 			unify ctx eget.etype r_get p;
 			let cf_set,tf_set,r_set,ekey,eget = AbstractCast.find_array_write_access ctx a tl ekey eget p in
 			let et = type_module_type ctx (TClassDecl c) p in
 			let e = match cf_set.cf_expr,cf_get.cf_expr with
-				| None,None ->
+				(* | None,None ->
 					let ea = mk (TArray(ebase,ekey)) r_get p in
-					mk (TBinop(OpAssignOp op,ea,type_expr ctx e2 (WithType.with_type r_get))) r_set p
+					mk (TBinop(OpAssignOp op,ea,type_expr ctx e2 (WithType.with_type r_get))) r_set p *)
 				| Some _,Some _ ->
 					let ef_set = mk (TField(et,(FStatic(c,cf_set)))) tf_set p in
 					let el = [make_call ctx ef_set [ebase;ekey;eget] r_set p] in
@@ -745,20 +741,89 @@ let type_assign_op ctx op e1 e2 with_type p =
 					raise_typing_error "Invalid array access getter/setter combination" p
 			in
 			save();
-			vr#to_texpr	e
+			api.generate vr eread e
 		| AKResolve(sea,name) ->
 			let e,vr = process_lhs_expr ctx "fh" sea.se_this in
-			let t_lhs,r_rhs = field_rhs_by_name op name e WithType.value in
+			let e_lhs,r_rhs = field_rhs_by_name name e WithType.value in
 			let assign e_rhs =
 				let e_name = Texpr.Builder.make_string ctx.t name null_pos in
 				(new call_dispatcher ctx (MCall [e2]) with_type p)#field_call sea.se_access [sea.se_this;e_name;e_rhs] []
 			in
-			let e = BinopResult.to_texpr vr r_rhs assign in
-			vr#to_texpr e
+			let e = api.to_texpr vr r_rhs assign in
+			api.generate vr e_lhs e
 	in
 	let with_type = with_type_or_value with_type in
 	loop (!type_access_ref ctx (fst e1) (snd e1) (MSet (Some e2)) with_type)
 
+let type_assign_op ctx op e1 e2 with_type p =
+	let api = {
+		akno_fallback = (fun () ->
+			type_non_assign_op ctx op e1 e2 true true with_type p
+		);
+		type_rhs = (fun e_lhs e2 ->
+			type_binop2 ctx op e_lhs e2 true WithType.value p
+		);
+		to_texpr = (fun vr br assign ->
+			BinopResult.to_texpr vr br assign
+		);
+		generate = (fun vr e_lhs e ->
+			vr#to_texpr e
+		);
+		assign = (fun vr e_lhs r_rhs ->
+			let assign e_rhs =
+				if BinopResult.needs_assign r_rhs then check_assign ctx e_lhs;
+				let e_rhs = AbstractCast.cast_or_unify ctx e_lhs.etype e_rhs p in
+				match e_rhs.eexpr with
+				| TBinop(op',e1',e2') when op = op' && Texpr.equal e_lhs e1' ->
+					mk (TBinop(OpAssignOp op',e1',e2')) e_lhs.etype p
+				| _ ->
+					mk (TBinop(OpAssign,e_lhs,e_rhs)) e_lhs.etype p
+			in
+			let e = BinopResult.to_texpr vr r_rhs assign in
+			vr#to_texpr e
+		)
+	} in
+	handle_assign_op ctx api e1 e2 with_type p
+
+let type_op_null_coal_assign ctx e1 e2 with_type p =
+	let hack = ref (fun e -> e) in
+	let gen vr e1 t2 e_assign =
+		let e1,eelse,tif = match with_type with
+			| WithType.NoValue ->
+				e1,None,ctx.t.tvoid
+			| _ ->
+				let e1 = vr#as_var "tmp" e1 in
+				(* The t2 is here so that `anything ??= 2` doesn't become Null<T> *)
+				e1,Some e1,t2
+		in
+		let e_null = Texpr.Builder.make_null e1.etype e1.epos in
+		let e_null = Texpr.Builder.binop OpEq e1 e_null ctx.t.tbool e1.epos in
+		let e = mk (TIf(e_null,e_assign,eelse)) tif e1.epos in
+		vr#to_texpr e
+	in
+	let api = {
+		akno_fallback = (fun () ->
+			raise Not_found
+		);
+		type_rhs = (fun e_lhs e2 ->
+			type_expr ctx e2 (WithType.WithType(e_lhs.etype,None))
+		);
+		to_texpr = (fun vr e assign ->
+			hack := assign;
+			e
+		);
+		generate = (fun vr e_lhs e ->
+			gen vr e_lhs e.etype (!hack e)
+		);
+		assign = (fun vr e_lhs e_rhs ->
+			let assign e_rhs =
+				let e_rhs = AbstractCast.cast_or_unify ctx e_lhs.etype e_rhs p in
+				mk (TBinop(OpAssign,e_lhs,e_rhs)) e_lhs.etype p
+			in
+			gen vr e_lhs e_rhs.etype (assign e_rhs)
+		)
+	} in
+	handle_assign_op ctx api e1 e2 with_type p
 
 let type_binop ctx op e1 e2 is_assign_op with_type p =
 	match op with

src/typing/typer.ml

@@ -1878,9 +1878,7 @@ and type_expr ?(mode=MGet) ctx (e,p) (with_type:WithType.t) =
 		let e_if = mk (TIf(e_cond,cast e1,Some e2)) iftype p in
 		vr#to_texpr e_if
 	| EBinop (OpAssignOp OpNullCoal,e1,e2) ->
-		let e_cond = EBinop(OpNotEq,e1,(EConst(Ident "null"), p)) in
-		let e_if = EIf ((e_cond, p),e1,Some e2) in
-		type_assign ctx e1 (e_if, p) with_type p
+		type_op_null_coal_assign ctx e1 e2 with_type p
 	| EBinop (op,e1,e2) ->
 		type_binop ctx op e1 e2 false with_type p
 	| EBlock [] when (match with_type with

src/typing/typerBase.ml

@@ -269,7 +269,7 @@ let rec s_access_kind acc =
 	| AKUsingField sea -> Printf.sprintf "AKUsingField(%s)" (s_static_extension_access sea)
 	| AKUsingAccessor sea -> Printf.sprintf "AKUsingAccessor(%s)" (s_static_extension_access sea)
 	| AKAccess(a,tl,c,e1,e2) -> Printf.sprintf "AKAccess(%s, [%s], %s, %s, %s)" (s_type_path a.a_path) (String.concat ", " (List.map st tl)) (s_type_path c.cl_path) (se e1) (se e2)
-	| AKResolve(_) -> ""
+	| AKResolve(sea,name) -> Printf.sprintf "AKResolve(%s, %s)" (s_static_extension_access sea) name
 
 and s_safe_nav_access sn =
 	let st = s_type (print_context()) in

tests/misc/projects/Issue10845/compile-fail.hxml.stderr

@@ -1,6 +1,6 @@
 Main.hx:21: characters 3-10 : Cannot modify abstract value of final field
 Main.hx:22: characters 3-10 : Cannot modify abstract value of final local
-Main.hx:24: characters 3-8 : Cannot modify abstract value of final field
+Main.hx:24: characters 3-13 : Cannot modify abstract value of final field
 Main.hx:25: characters 3-13 : Cannot modify abstract value of final local
 Main.hx:29: characters 3-8 : This expression cannot be accessed for writing
 Main.hx:30: characters 3-8 : Cannot assign to final

tests/optimization/src/issues/Issue11931.hx

@@ -0,0 +1,17 @@
+package issues;
+
+class Issue11931 {
+	@:js('
+		var arr = [];
+		var tmp = arr[0];
+		issues_Issue11931.use(tmp == null ? arr[0] = [] : tmp);
+	')
+	static function test() {
+		var arr = [];
+		var e = arr[0] ??= [];
+		use(e);
+	}
+
+	@:pure(false)
+	static function use(v:Array<Int>) {}
+}

tests/unit/src/unit/TestNullCoalescing.hx

@@ -4,6 +4,90 @@ private class A {}
 private class B extends A {}
 private class C extends A {}
 
+private class NullCoalClass {
+	@:isVar public var field(get, set):String;
+
+	public var getCounter = 0;
+	public var setCounter = 0;
+
+	public function new() {}
+
+	public function get_field() {
+		getCounter++;
+		return field;
+	}
+
+	public function set_field(v:String) {
+		setCounter++;
+		return field = v;
+	}
+}
+
+private typedef NullCoalAbstractData = {
+	var field:String;
+	var getCounter:Int;
+	var setCounter:Int;
+}
+
+private abstract NullCoalAbstract(NullCoalAbstractData) {
+	public var field(get, set):String;
+
+	public function new() {
+		this = {
+			field: null,
+			getCounter: 0,
+			setCounter: 0
+		}
+	}
+
+	public function getGetCounter() {
+		return this.getCounter;
+	}
+
+	public function getSetCounter() {
+		return this.setCounter;
+	}
+
+	public function get_field() {
+		this.getCounter++;
+		return this.field;
+	}
+
+	public function set_field(v:String) {
+		this.setCounter++;
+		return this.field = v;
+	}
+}
+
+private abstract NullCoalAbstractResolve(NullCoalAbstractData) {
+	public function new() {
+		this = {
+			field: null,
+			getCounter: 0,
+			setCounter: 0
+		}
+	}
+
+	public function getGetCounter() {
+		return this.getCounter;
+	}
+
+	public function getSetCounter() {
+		return this.setCounter;
+	}
+
+	@:op(a.b) public function readResolve(field:String) {
+		this.getCounter++;
+		return Reflect.field(this, field);
+	}
+
+	@:op(a.b) public function writeResolve<T>(field:String, v:T) {
+		this.setCounter++;
+		Reflect.setField(this, field, v);
+		return Reflect.field(this, field);
+	}
+}
+
 @:nullSafety(StrictThreaded)
 class TestNullCoalescing extends Test {
 	final nullInt:Null<Int> = null;
@@ -19,6 +103,7 @@ class TestNullCoalescing extends Test {
 	}
 
 	function test() {
+		count = 0;
 		eq(true, 0 != 1 ?? 2);
 		var a = call() ?? "default";
 		eq(count, 1);
@@ -53,6 +138,7 @@ class TestNullCoalescing extends Test {
 		eq(nullInt ?? 2, 2);
 		eq(nullInt ?? (2 : Null<Int>) ?? 3 + 100, 2);
 		eq(nullInt ?? nullInt ?? 3, 3);
+		f(HelperMacros.isNullable(nullInt ?? nullInt ?? 3));
 
 		final i:Null<Int> = 1;
 		final arr:Array<Int> = [i ?? 2];
@@ -73,29 +159,6 @@ class TestNullCoalescing extends Test {
 		final di3:Null<Dynamic> = 2;
 		eq(di ?? di2 ?? di3, 2);
 
-		var a:Null<Int> = null;
-		a ??= 5;
-		eq(a, 5);
-		var a:Null<Int> = null;
-		eq(a ??= 5, 5);
-		eq(a, 5);
-		var a = "default";
-		eq(a ??= "5", "default");
-
-		count = 0;
-		var a = call();
-		eq(count, 1);
-		a ??= call();
-		eq(count, 1);
-
-		var a:Null<String> = null;
-		final b = a ??= call();
-		final c = a ??= call();
-		eq(count, 2);
-		eq(a, "_");
-		eq(b, "_");
-		eq(c, "_");
-
 		final a:Null<Int> = ({} : Dynamic).x;
 		eq(a ?? 2, 2);
 
@@ -143,4 +206,161 @@ class TestNullCoalescing extends Test {
 		f(HelperMacros.isNullable(notNullF));
 		f(HelperMacros.isNullable(notNullF2));
 	}
+
+	function testAssignOp() {
+		count = 0;
+		var a:Null<Int> = null;
+		a ??= 5;
+		eq(a, 5);
+		t(HelperMacros.isNullable(a ??= null));
+		f(HelperMacros.isNullable(a ??= 5));
+		var a:Null<Int> = null;
+		eq(a ??= 5, 5);
+		eq(a, 5);
+		var a = "default";
+		eq(a ??= "5", "default");
+
+		count = 0;
+		var a = call();
+		eq(count, 1);
+		a ??= call();
+		eq(count, 1);
+
+		var a:Null<String> = null;
+		final b = a ??= call();
+		final c = a ??= call();
+		eq(count, 2);
+		eq(a, "_");
+		eq(b, "_");
+		eq(c, "_");
+
+		final map:Map<String, Array<Int>> = [];
+		var array1 = [];
+		map["foo"] ??= array1;
+		eq(map["foo"], array1);
+		map["foo"] ??= [];
+		eq(map["foo"], array1);
+
+		// test typing
+		#if !macro
+		var a = mut() ?? mut();
+		eq(2, getMut());
+		resetMut();
+
+		var a:Null<Int> = 0;
+		mutAssignLeft(a) ??= mut() ?? mut();
+		eq(3, getMut());
+		resetMut();
+
+		var a:Null<Int> = 0;
+		final b = a ??= mut();
+		eq(1, getMut());
+		resetMut();
+
+		var a:Null<Int> = 0;
+		mutAssignLeft(a) ??= 1;
+		eq(1, getMut());
+		resetMut();
+
+		// field
+		var obj = getObj();
+		obj.field ??= "value";
+		eq("value", obj.field ?? "fail");
+
+		var value = obj.field ??= "value2";
+		eq("value", obj.field ?? "fail");
+		eq("value", value);
+
+		mutAssignLeft(obj.field) ??= "not value";
+		eq(1, getMut());
+		eq("value", obj.field ?? "fail");
+		resetMut();
+
+		// accessor
+		var obj = new NullCoalClass();
+		obj.field ??= "value";
+		eq(1, obj.getCounter);
+		eq(1, obj.setCounter);
+		eq("value", obj.field ?? "fail");
+
+		var value = obj.field ??= "value2";
+		eq(3, obj.getCounter);
+		eq(1, obj.setCounter);
+		eq("value", obj.field ?? "fail");
+		eq("value", value);
+
+		mutAssignLeft(obj.field) ??= "not value";
+		eq(5, obj.getCounter);
+		eq(1, obj.setCounter);
+		eq(1, getMut());
+		eq("value", obj.field ?? "fail");
+		resetMut();
+
+		// static extension accessor
+		var obj = new NullCoalAbstract();
+		obj.field ??= "value";
+		eq(1, obj.getGetCounter());
+		eq(1, obj.getSetCounter());
+		eq("value", obj.field ?? "fail");
+
+		var value = obj.field ??= "value2";
+		eq(3, obj.getGetCounter());
+		eq(1, obj.getSetCounter());
+		eq("value", obj.field ?? "fail");
+		eq("value", value);
+
+		mutAssignLeft(obj.field) ??= "not value";
+		eq(5, obj.getGetCounter());
+		eq(1, obj.getSetCounter());
+		eq(1, getMut());
+		eq("value", obj.field ?? "fail");
+		resetMut();
+
+		// resolve
+		var obj = new NullCoalAbstractResolve();
+		obj.field ??= "value";
+		eq(1, obj.getGetCounter());
+		eq(1, obj.getSetCounter());
+		eq("value", obj.field ?? "fail");
+
+		var value = obj.field ??= "value2";
+		eq(3, obj.getGetCounter());
+		eq(1, obj.getSetCounter());
+		eq("value", obj.field ?? "fail");
+		eq("value", value);
+
+		// TODO: this fails at the moment with some "not enough arguments error"
+		// mutAssignLeft(obj.field) ??= "not value";
+		// eq(5, obj.getGetCounter());
+		// eq(1, obj.getSetCounter());
+		// eq(1, getMut());
+		// eq("value", obj.field ?? "fail");
+		// resetMut();
+		#end
+	}
+
+	static var mutI = 0;
+
+	static function getObj<T>():{field:Null<T>} {
+		return {field: null}
+	}
+
+	static macro function mut() {
+		mutI++;
+		return macro mutI;
+	}
+
+	static macro function getMut() {
+		return macro $v{mutI};
+	}
+
+	static macro function resetMut() {
+		mutI = 0;
+		return macro $v{mutI};
+	}
+
+	static macro function mutAssignLeft(e:haxe.macro.Expr) {
+		mutI++;
+		return e;
+	}
 }