[macro] fix redefinition of modules with defineType/defineModule (#12001)

* [macro] Disallow redefining module through Context's defineType/defineModule

* [macro] defineType / defineModule: allow redefining MSBad modules

Note: this hook should be moved somewhere else..

* [tests] small cleanup

* [tests] add test

* [tests] add more tests

* [std] add haxe.macro.CompilationServer.invalidateModule()

* Fix defineModule into curmod

* [tests] add test using CompilationServer.invalidateModule

* Default impl without server

* [macro] disallow invalidating loaded module

* CompilationServer.invalidate: raise error that macros can catch; add test

* Rework implementation

Allow redefining when not loaded, but invalidate previous cached module (if any)

* [tests] update tests

* Use more specific invalidation reasons

* [tests] update tests

* Let macros catch redefinition errors

* [tests] test catching errors

* [std] more information about catching errors

src/compiler/compilationCache.ml

@@ -234,15 +234,27 @@ class cache = object(self)
 			) cc#get_modules acc
 		) contexts []
 
+	method taint_module m_path reason =
+		Hashtbl.iter (fun _ cc ->
+			Hashtbl.iter (fun _ m ->
+				if m.m_path = m_path then m.m_extra.m_cache_state <- MSBad (Tainted reason)
+			) cc#get_modules;
+			Hashtbl.iter (fun _ mc ->
+				if mc.HxbData.mc_path = m_path then
+					mc.HxbData.mc_extra.m_cache_state <- match reason, mc.mc_extra.m_cache_state with
+					| CheckDisplayFile, (MSBad _ as state) -> state
+					| _ -> MSBad (Tainted reason)
+			) cc#get_hxb
+		) contexts
+
 	method taint_modules file_key reason =
 		Hashtbl.iter (fun _ cc ->
 			Hashtbl.iter (fun _ m ->
 				if Path.UniqueKey.lazy_key m.m_extra.m_file = file_key then m.m_extra.m_cache_state <- MSBad (Tainted reason)
 			) cc#get_modules;
-			let open HxbData in
 			Hashtbl.iter (fun _ mc ->
-				if Path.UniqueKey.lazy_key mc.mc_extra.m_file = file_key then
-					mc.mc_extra.m_cache_state <- match reason, mc.mc_extra.m_cache_state with
+				if Path.UniqueKey.lazy_key mc.HxbData.mc_extra.m_file = file_key then
+					mc.HxbData.mc_extra.m_cache_state <- match reason, mc.HxbData.mc_extra.m_cache_state with
 					| CheckDisplayFile, (MSBad _ as state) -> state
 					| _ -> MSBad (Tainted reason)
 			) cc#get_hxb

src/core/tPrinting.ml

@@ -637,8 +637,11 @@ module Printer = struct
 
 	let s_module_tainting_reason = function
 		| CheckDisplayFile -> "check_display_file"
+		| DefineType -> "define_type"
+		| DefineModule -> "define_module"
 		| ServerInvalidate -> "server/invalidate"
 		| ServerInvalidateFiles -> "server_invalidate_files"
+		| ServerInvalidateModule -> "server_invalidate_module"
 
 	let s_module_skip_reason reason =
 		let rec loop stack = function

src/core/tType.ml

@@ -32,8 +32,11 @@ type module_check_policy =
 
 type module_tainting_reason =
 	| CheckDisplayFile
+	| DefineType
+	| DefineModule
 	| ServerInvalidate
 	| ServerInvalidateFiles
+	| ServerInvalidateModule
 
 type module_skip_reason =
 	| DependencyDirty of path * module_skip_reason

src/macro/macroApi.ml

@@ -2323,6 +2323,18 @@ let macro_api ccom get_api =
 			(get_api()).add_module_check_policy filter policy (decode_bool recursive);
 			vnull
 		);
+		"server_invalidate_module", vfun1 (fun p ->
+			let mpath = parse_path (decode_string p) in
+			let com = ccom() in
+			(try
+				ignore(com.module_lut#find mpath);
+				let msg = "Cannot invalidate loaded module " ^ (s_type_path mpath) in
+				let pos = get_api_call_pos() in
+				compiler_error (Error.make_error (Custom msg) pos)
+			with Not_found ->
+				com.cs#taint_module mpath ServerInvalidateModule);
+			vnull
+		);
 		"server_invalidate_files", vfun1 (fun a ->
 			let com = ccom() in
 			let cs = com.cs in

src/typing/macroContext.ml

@@ -464,13 +464,21 @@ let make_macro_api ctx mctx p =
 				| _ -> false
 			in
 			let add is_macro ctx =
-				let mdep = Option.map_default (fun s -> TypeloadModule.load_module ~origin:MDepFromMacro ctx (parse_path s) pos) ctx.m.curmod mdep in
-				let mnew = TypeloadModule.type_module ctx.com ctx.g ~dont_check_path:(has_native_meta) mpath (ctx.com.file_keys#generate_virtual mpath ctx.com.compilation_step) [tdef,pos] pos in
-				mnew.m_extra.m_kind <- if is_macro then MMacro else MFake;
-				add_dependency mnew mdep MDepFromMacro;
-				add_dependency mdep mnew MDepFromMacroDefine;
-				ctx.com.module_nonexistent_lut#clear;
-			in
+				try
+					let m = ctx.com.module_lut#find mpath in
+					let pos = { pfile = (Path.UniqueKey.lazy_path m.m_extra.m_file); pmin = 0; pmax = 0 } in
+					Interp.compiler_error (make_error ~sub:[
+						make_error ~depth:1 (Custom "Previously defined here") pos
+					] (Custom (Printf.sprintf "Cannot redefine module %s" (s_type_path mpath))) p);
+				with Not_found ->
+					ctx.com.cs#taint_module mpath DefineType;
+					let mdep = Option.map_default (fun s -> TypeloadModule.load_module ~origin:MDepFromMacro ctx (parse_path s) pos) ctx.m.curmod mdep in
+					let mnew = TypeloadModule.type_module ctx.com ctx.g ~dont_check_path:(has_native_meta) mpath (ctx.com.file_keys#generate_virtual mpath ctx.com.compilation_step) [tdef,pos] pos in
+					mnew.m_extra.m_kind <- if is_macro then MMacro else MFake;
+					add_dependency mnew mdep MDepFromMacro;
+					add_dependency mdep mnew MDepFromMacroDefine;
+					ctx.com.module_nonexistent_lut#clear;
+				in
 			add false ctx;
 			(* if we are adding a class which has a macro field, we also have to add it to the macro context (issue #1497) *)
 			if not ctx.com.is_macro_context then match tdef with
@@ -496,12 +504,13 @@ let make_macro_api ctx mctx p =
 				let m = ctx.com.module_lut#find mpath in
 				if m != ctx.m.curmod then begin
 					let pos = { pfile = (Path.UniqueKey.lazy_path m.m_extra.m_file); pmin = 0; pmax = 0 } in
-					raise_typing_error_ext (make_error ~sub:[
+					Interp.compiler_error (make_error ~sub:[
 						make_error ~depth:1 (Custom "Previously defined here") pos
 					] (Custom (Printf.sprintf "Cannot redefine module %s" (s_type_path mpath))) p);
 				end else
-					ignore(TypeloadModule.type_types_into_module ctx.com ctx.g m types pos)
+					ignore(TypeloadModule.type_types_into_module ctx.com ctx.g ctx.m.curmod types pos)
 			with Not_found ->
+				ctx.com.cs#taint_module mpath DefineModule;
 				let mnew = TypeloadModule.type_module ctx.com ctx.g mpath (ctx.com.file_keys#generate_virtual mpath ctx.com.compilation_step) types pos in
 				mnew.m_extra.m_kind <- MFake;
 				add_dependency mnew ctx.m.curmod MDepFromMacro;

std/haxe/macro/CompilationServer.hx

@@ -69,6 +69,17 @@ class CompilationServer {
 		});
 	}
 
+	/**
+		Invalidates a module, removing it from the cache.
+
+		If the module has already been loaded in current context, a
+		`haxe.macro.Expr.Error` compiler error will be raised which can be
+		caught using `try ... catch`.
+	**/
+	static public function invalidateModule(path:String) {
+		@:privateAccess Compiler.load("server_invalidate_module", 1)(path);
+	}
+
 	/**
 		Invalidates all files given in `filePaths`, removing them from the cache.
 	**/

std/haxe/macro/Context.hx

@@ -663,6 +663,10 @@ class Context {
 	/**
 		Defines a new type from `TypeDefinition` `t`.
 
+		If a matching module has already been loaded in current context, a
+		`haxe.macro.Expr.Error` compiler error will be raised which can be
+		caught using `try ... catch`.
+
 		If `moduleDependency` is given and is not `null`, it should contain
 		a module path that will be used as a dependency for the newly defined module
 		instead of the current module.
@@ -695,6 +699,10 @@ class Context {
 		Defines a new module as `modulePath` with several `TypeDefinition`
 		`types`. This is analogous to defining a .hx file.
 
+		If a matching module has already been loaded in current context, a
+		`haxe.macro.Expr.Error` compiler error will be raised which can be
+		caught using `try ... catch`.
+
 		The individual `types` can reference each other and any identifier
 		respects the `imports` and `usings` as usual, expect that imports are
 		not allowed to have `.*` wildcards or `as s` shorthands.

tests/server/src/cases/issues/Issue12001.hx

@@ -0,0 +1,129 @@
+package cases.issues;
+
+import utest.Async;
+
+class Issue12001 extends TestCase {
+	function testDefineType(_) {
+		vfs.putContent("Macro.hx", getTemplate("issues/Issue12001/Macro.hx"));
+		vfs.putContent("Empty.hx", getTemplate("Empty.hx"));
+		var args = ["-main", "Empty", "--macro", "Macro.defineType()"];
+		runHaxe(args);
+		assertSuccess();
+
+		// Nothing is loading Foo, so no redefinition issue
+		runHaxe(args);
+		assertSuccess();
+	}
+
+	function testRedefineTypeCatchError(_) {
+		vfs.putContent("Macro.hx", getTemplate("issues/Issue12001/Macro.hx"));
+		vfs.putContent("Empty.hx", getTemplate("Empty.hx"));
+		var args = ["-main", "Empty", "--macro", "Macro.redefineTypeCatchError()"];
+		runHaxe(args);
+		assertSuccess();
+
+		runHaxe(args);
+		assertSuccess();
+		assertHasPrint("Macro.hx:56: TInst(Foobar,[])");
+		assertHasPrint("Macro.hx:69: Cannot redefine module Foobar");
+	}
+
+	@:async
+	@:timeout(3000)
+	function testRedefineType(async:Async) {
+		vfs.putContent("Macro.hx", getTemplate("issues/Issue12001/Macro.hx"));
+		vfs.putContent("Main.hx", getTemplate("issues/Issue12001/Main.hx"));
+		var args = ["-main", "Main", "--interp", "--macro", "Macro.defineType()"];
+		var i = 0;
+		function test() {
+			// Was failing with nightlies (HxbFailure)
+			runHaxe(args, () -> {
+				assertSuccess();
+				assertHasPrint("Foo.test() = " + i);
+				if (++i >= 5) async.done();
+				else test();
+			});
+		}
+		test();
+	}
+
+	function testDefineModule(_) {
+		vfs.putContent("Macro.hx", getTemplate("issues/Issue12001/Macro.hx"));
+		vfs.putContent("Empty.hx", getTemplate("Empty.hx"));
+		var args = ["-main", "Empty", "--macro", "Macro.defineModule()"];
+		runHaxe(args);
+		assertSuccess();
+
+		// Nothing is loading Bar, so no redefinition issue
+		runHaxe(args);
+		assertSuccess();
+	}
+
+	function testRedefineModuleCatchError(_) {
+		vfs.putContent("Macro.hx", getTemplate("issues/Issue12001/Macro.hx"));
+		vfs.putContent("Empty.hx", getTemplate("Empty.hx"));
+		var args = ["-main", "Empty", "--macro", "Macro.redefineModuleCatchError()"];
+		runHaxe(args);
+		assertSuccess();
+
+		runHaxe(args);
+		assertSuccess();
+		assertHasPrint("Macro.hx:77: TInst(Foobaz,[])");
+		assertHasPrint("Macro.hx:90: Cannot redefine module Foobaz");
+	}
+
+	@:async
+	@:timeout(3000)
+	function testRedefineModule(async:Async) {
+		vfs.putContent("Macro.hx", getTemplate("issues/Issue12001/Macro.hx"));
+		vfs.putContent("Main.hx", getTemplate("issues/Issue12001/Main1.hx"));
+		var args = ["-main", "Main", "--interp", "--macro", "Macro.defineModule()"];
+		var i = 0;
+		function test() {
+			// Was failing with nightlies (HxbFailure)
+			runHaxe(args, () -> {
+				assertSuccess();
+				assertHasPrint("Bar.test() = " + i);
+				if (++i >= 5) async.done();
+				else test();
+			});
+		}
+		test();
+	}
+
+	@:async
+	@:timeout(3000)
+	function testRedefineAfterTyping(async:Async) {
+		vfs.putContent("Macro.hx", getTemplate("issues/Issue12001/Macro.hx"));
+		vfs.putContent("Empty.hx", getTemplate("Empty.hx"));
+		var args = ["-main", "Empty", "--interp", "--macro", "Macro.hookRedefine()"];
+		var i = 0;
+		function test() {
+			runHaxe(args, () -> {
+				assertSuccess();
+				// Newest version is being included
+				assertHasPrint("Baz.test() = " + i);
+				if (++i >= 5) async.done();
+				else test();
+			});
+		}
+		test();
+	}
+
+	function testInvalidateError(_) {
+		vfs.putContent("Macro.hx", getTemplate("issues/Issue12001/Macro1.hx"));
+		vfs.putContent("Empty.hx", getTemplate("Empty.hx"));
+		var args = ["-main", "Empty", "--interp", "--macro", "Macro.hookInvalidateError()"];
+		runHaxe(args);
+		assertErrorMessage("Cannot invalidate loaded module Empty");
+	}
+
+	function testInvalidateCaughtError(_) {
+		vfs.putContent("Macro.hx", getTemplate("issues/Issue12001/Macro1.hx"));
+		vfs.putContent("Empty.hx", getTemplate("Empty.hx"));
+		var args = ["-main", "Empty", "--interp", "--macro", "Macro.hookInvalidateCatch()"];
+		runHaxe(args);
+		assertSuccess();
+		assertHasPrint("Cannot invalidate loaded module Empty");
+	}
+}

tests/server/test/templates/csSafeTypeBuilding/Macro.macro.hx

@@ -11,23 +11,19 @@ class Macro {
 
 	@:persistent static var generated = new Map<String, Bool>();
 
-	#if config.getType
-	static function isAlive(name:String):Bool {
+	static function isAlive(name:String, ct:ComplexType, pos:Position):Bool {
 		// Null check is just there to make it a one liner
 		// Basically returning true if no exception is caught
+		#if config.getType
 		return try Context.getType(name) != null
 			catch(s:String) {
 				if (s != 'Type not found \'$name\'') throw s;
 				false;
 			};
-	}
-	#else
-	static function isAlive(ct:ComplexType, pos:Position):Bool {
-		// Null check is just there to make it a one liner
-		// Basically returning true if no exception is caught
+		#else
 		return try Context.resolveType(ct, pos) != null catch(e) false;
+		#end
 	}
-	#end
 
 	public static function buildFoo() {
 		var from = '[${Context.getLocalModule()}] ';
@@ -41,11 +37,7 @@ class Macro {
 				var ct = TPath({pack: [], name: key});
 
 				if (generated.exists(key)) {
-					#if config.getType
-					if (isAlive(key)) {
-					#else
-					if (isAlive(ct, pos)) {
-					#end
+					if (isAlive(key, ct, pos)) {
 						print('Reusing previously generated type for $key.');
 						return ct;
 					}

tests/server/test/templates/issues/Issue12001/Macro.hx

@@ -0,0 +1,93 @@
+import haxe.macro.Context;
+import haxe.macro.Expr.Error;
+
+@:persistent var i = 0;
+function defineType() {
+	Context.onAfterInitMacros(() -> {
+		Context.defineType({
+			pos: Context.currentPos(),
+			pack: [],
+			name: "Foo",
+			kind: TDClass(null, null, false, false, false),
+			fields: (macro class Foo {
+				public static function test() Sys.println("Foo.test() = " + $v{i++});
+			}).fields
+		});
+	});
+}
+
+@:persistent var j = 0;
+function defineModule() {
+	Context.onAfterInitMacros(() -> {
+		Context.defineModule("Bar", [{
+			pos: Context.currentPos(),
+			pack: [],
+			name: "Bar",
+			kind: TDClass(null, null, false, false, false),
+			fields: (macro class Bar {
+				public static function test() Sys.println("Bar.test() = " + $v{j++});
+			}).fields
+		}]);
+	});
+}
+
+@:persistent var k = 0;
+function hookRedefine() {
+	var generated = false;
+	Context.onAfterTyping((_) -> {
+		if (generated) return;
+		generated = true;
+
+		Context.defineModule("Baz", [{
+			pos: Context.currentPos(),
+			pack: [],
+			name: "Baz",
+			kind: TDClass(null, null, false, false, false),
+			fields: (macro class Baz {
+				public static function __init__() Sys.println("Baz.test() = " + $v{k++});
+			}).fields
+		}]);
+	});
+}
+
+@:persistent var l = 0;
+function redefineTypeCatchError() {
+	Context.onAfterInitMacros(() -> {
+		if (l > 0) trace(Context.getType("Foobar"));
+
+		try {
+			l++;
+			Context.defineType({
+				pos: Context.currentPos(),
+				pack: [],
+				name: "Foobar",
+				kind: TDClass(null, null, false, false, false),
+				fields: []
+			});
+		} catch (e:Error) {
+			if (l == 0) throw e;
+			trace(e.message);
+		}
+	});
+}
+
+@:persistent var m = 0;
+function redefineModuleCatchError() {
+	Context.onAfterInitMacros(() -> {
+		if (m > 0) trace(Context.getType("Foobaz"));
+
+		try {
+			m++;
+			Context.defineModule("Foobaz", [{
+				pos: Context.currentPos(),
+				pack: [],
+				name: "Foobaz",
+				kind: TDClass(null, null, false, false, false),
+				fields: []
+			}]);
+		} catch (e:Error) {
+			if (m == 0) throw e;
+			trace(e.message);
+		}
+	});
+}

tests/server/test/templates/issues/Issue12001/Macro1.hx

@@ -0,0 +1,19 @@
+import haxe.macro.CompilationServer;
+import haxe.macro.Context;
+import haxe.macro.Expr.Error;
+
+function hookInvalidateError() {
+	Context.onAfterTyping((_) -> {
+		CompilationServer.invalidateModule("Empty");
+	});
+}
+
+function hookInvalidateCatch() {
+	Context.onAfterTyping((_) -> {
+		try {
+			CompilationServer.invalidateModule("Empty");
+		} catch (e:Error) {
+			Sys.println(e.message);
+		}
+	});
+}

tests/server/test/templates/issues/Issue12001/Main.hx

@@ -0,0 +1,3 @@
+function main() {
+	Foo.test();
+}

tests/server/test/templates/issues/Issue12001/Main1.hx

@@ -0,0 +1,3 @@
+function main() {
+	Bar.test();
+}