[server] don't leave modules in unknown cache state when a check raises

src/compiler/server/serverCache.ml

@@ -257,8 +257,10 @@ let check_module sctx com m_path m_extra p =
 					(* This should not happen because any MSUnknown module is supposed to have the current m_checked. *)
 					die "" __LOC__
 				| MSGood ->
-					(* Otherwise, run the checks *)
+					(* Otherwise, run the checks. Register the module before checking it so that its MSUnknown
+					   state is reverted even if the check raises. *)
 					m_extra.m_cache_state <- MSUnknown;
+					unknown_state_modules := m_extra :: !unknown_state_modules;
 					check ()
 			in
 			(* Update the module now. It will use this dirty status for the remainder of this compilation. *)
@@ -268,28 +270,40 @@ let check_module sctx com m_path m_extra p =
 				m_extra.m_cache_state <- MSBad reason;
 			| None ->
 				(* We cannot update if we're clean because at this point it might just be an assumption.
-				   Instead We add the module to a list which is updated at the end of handling this subgraph. *)
-				unknown_state_modules := m_extra :: !unknown_state_modules;
+				   The module stays in unknown_state_modules, which is updated at the end of handling this subgraph. *)
+				()
 			end;
 			dirty
 		end
 	in
-	let state = check m_path m_extra in
-	begin match state with
-	| None ->
-		(* If the entire subgraph is clean, we can set all modules to good state *)
-		List.iter (fun m_extra -> m_extra.m_cache_state <- MSGood) !unknown_state_modules;
-	| Some _ ->
-		(* Otherwise, unknown state module may or may not be dirty. We didn't check everything eagerly, so we have
-		   to make sure that the module is checked again if it appears in a different check. This is achieved by
-		   setting m_checked to a lower value and assuming Good state again. *)
+	(* Unknown state modules may or may not be dirty. We didn't check everything eagerly, so we have to make sure
+	   that they are checked again if they appear in a different check. This is achieved by setting m_checked to a
+	   lower value and assuming Good state again. *)
+	let revert_unknown_state_modules () =
 		List.iter (fun m_extra -> match m_extra.m_cache_state with
 			| MSUnknown ->
 				m_extra.m_checked <- start_mark - 1;
 				m_extra.m_cache_state <- MSGood;
 			| MSGood | MSBad _ ->
 				()
 		) !unknown_state_modules
+	in
+	let state = try
+		check m_path m_extra
+	with e ->
+		(* Leaving modules in MSUnknown state would violate the m_checked invariant of any later check. *)
+		revert_unknown_state_modules();
+		raise e
+	in
+	begin match state with
+	| None ->
+		(* If the entire subgraph is clean, we can set all modules to good state *)
+		List.iter (fun m_extra -> match m_extra.m_cache_state with
+			| MSUnknown -> m_extra.m_cache_state <- MSGood
+			| MSGood | MSBad _ -> ()
+		) !unknown_state_modules;
+	| Some _ ->
+		revert_unknown_state_modules()
 	end;
 	state
 

tests/server/src/cases/ServerTests.hx

@@ -768,4 +768,29 @@ class ServerTests extends TestCase {
 		}
 		// Hover may also succeed in some configurations — that's fine too.
 	}
+
+	function testErrorWhileCheckingModules() {
+		vfs.putContent("Util.hx", "class Util {\n\tpublic static function foo() return 1;\n}");
+		// import.hx modules are kept in the cache by reference, so a stale cache state survives the request
+		vfs.putContent("import.hx", "import Util;");
+		vfs.putContent("Main.hx", "class Main {\n\tstatic function main() {\n\t\ttrace(Util.foo());\n\t}\n}");
+		var args = [
+			"-main", "Main", "--macro",
+			"server.setModuleCheckPolicy(['Util'], [CheckFileContentModification], false)", "--no-output", "-js", "no.js"
+		];
+		runHaxe(args);
+		assertSuccess();
+		// The check policy makes the module check parse Util.hx, which now fails
+		vfs.putContent("Util.hx", "class Util {\n\tpublic static function foo() return 1;\n\t@#$%\n}");
+		vfs.touch("Util.hx");
+		runHaxe(args);
+		assertErrorMessage("Invalid character '#'");
+		// The error must not leave any module in an unknown cache state
+		runHaxe(args);
+		assertErrorMessage("Invalid character '#'");
+		vfs.putContent("Util.hx", "class Util {\n\tpublic static function foo() return 2;\n}");
+		vfs.touch("Util.hx");
+		runHaxe(args);
+		assertSuccess();
+	}
 }

tests/server/src/utils/Vfs.hx

@@ -31,6 +31,16 @@ class Vfs {
 		Fs.writeFileSync(path.toString(), content);
 	}
 
+	/**
+		Bumps the modification time of `path` into the future. The server compares file times with
+		second granularity, so a rewrite within the same second is invisible to it otherwise.
+	**/
+	public function touch(path:String) {
+		var path = getPhysicalPath(path).toString();
+		var time = Date.fromTime(Date.now().getTime() + 2000);
+		Fs.utimesSync(path, time, time);
+	}
+
 	public function getContent(path:String):String {
 		var path = getPhysicalPath(path);
 		FileSystem.createDirectory(path.dir);