Run some filters in diagnostics (HaxeFoundation#11220)

* let's see how much breaks * [tests] enable diagnostics tests for 11177 and 11184 * [tests] Update test for 5306 * Don't cache/run filters for find reference/implementation requests (HaxeFoundation#11226) * Only run filters and save cache on diagnostics, not usage requests * [tests] Update test for 11184 * disable test * add VUsedByTyper to avoid bad unused local errors * revert @:compilerGenerated change --------- Co-authored-by: Rudy Ges <[email protected]>
0b1kn00b · Jan 25, 2024 · 6354c3c · 6354c3c
1 parent 04d280a
commit 6354c3c
Show file tree

Hide file tree

Showing 19 changed files with 135 additions and 74 deletions.
diff --git a/src/compiler/compiler.ml b/src/compiler/compiler.ml
@@ -323,10 +323,10 @@ let finalize_typing ctx tctx =
 	com.modules <- modules;
 	t()
 
-let filter ctx tctx =
+let filter ctx tctx before_destruction =
 	let t = Timer.timer ["filters"] in
 	DeprecationCheck.run ctx.com;
-	Filters.run tctx ctx.com.main;
+	run_or_diagnose ctx Filters.run tctx ctx.com.main before_destruction;
 	t()
 
 let compile ctx actx callbacks =
@@ -363,8 +363,12 @@ let compile ctx actx callbacks =
 		let (tctx,display_file_dot_path) = do_type ctx mctx actx display_file_dot_path macro_cache_enabled in
 		DisplayProcessing.handle_display_after_typing ctx tctx display_file_dot_path;
 		finalize_typing ctx tctx;
-		DisplayProcessing.handle_display_after_finalization ctx tctx display_file_dot_path;
-		filter ctx tctx;
+		if is_diagnostics com then
+			filter ctx tctx (fun () -> DisplayProcessing.handle_display_after_finalization ctx tctx display_file_dot_path)
+		else begin
+			DisplayProcessing.handle_display_after_finalization ctx tctx display_file_dot_path;
+			filter ctx tctx (fun () -> ());
+		end;
 		if ctx.has_error then raise Abort;
 		Generate.check_auxiliary_output com actx;
 		enter_stage com CGenerationStart;

diff --git a/src/context/display/diagnostics.ml b/src/context/display/diagnostics.ml
@@ -16,7 +16,7 @@ let find_unused_variables com e =
 	let vars = Hashtbl.create 0 in
 	let pmin_map = Hashtbl.create 0 in
 	let rec loop e = match e.eexpr with
-		| TVar({v_kind = VUser _} as v,eo) when v.v_name <> "_" ->
+		| TVar({v_kind = VUser _} as v,eo) when v.v_name <> "_" && not (has_var_flag v VUsedByTyper) ->
 			Hashtbl.add pmin_map e.epos.pmin v;
 			let p = match eo with
 				| None -> e.epos
@@ -44,18 +44,18 @@ let check_other_things com e =
 	let pointless_compound s p =
 		add_diagnostics_message com (Printf.sprintf "This %s has no effect, but some of its sub-expressions do" s) p DKCompilerMessage Warning;
 	in
-	let rec compound compiler_generated s el p =
+	let rec compound s el p =
 		let old = !had_effect in
 		had_effect := false;
-		List.iter (loop true compiler_generated) el;
-		if not !had_effect then no_effect p else if not compiler_generated then pointless_compound s p;
+		List.iter (loop true) el;
+		if not !had_effect then no_effect p else pointless_compound s p;
 		had_effect := old;
-	and loop in_value compiler_generated e = match e.eexpr with
+	and loop in_value e = match e.eexpr with
 		| TBlock el ->
 			let rec loop2 el = match el with
 				| [] -> ()
-				| [e] -> loop in_value compiler_generated e
-				| e :: el -> loop false compiler_generated e; loop2 el
+				| [e] -> loop in_value e
+				| e :: el -> loop false e; loop2 el
 			in
 			loop2 el
 		| TMeta((Meta.Extern,_,_),_) ->
@@ -67,29 +67,27 @@ let check_other_things com e =
 			()
 		| TField (_, fa) when PurityState.is_explicitly_impure fa -> ()
 		| TFunction tf ->
-			loop false compiler_generated tf.tf_expr
-		| TCall({eexpr = TField(e1,fa)},el) when not in_value && PurityState.is_pure_field_access fa -> compound compiler_generated "call" el e.epos
+			loop false tf.tf_expr
+		| TCall({eexpr = TField(e1,fa)},el) when not in_value && PurityState.is_pure_field_access fa -> compound "call" el e.epos
 		| TNew _ | TCall _ | TBinop ((Ast.OpAssignOp _ | Ast.OpAssign),_,_) | TUnop ((Ast.Increment | Ast.Decrement),_,_)
 		| TReturn _ | TBreak | TContinue | TThrow _ | TCast (_,Some _)
 		| TIf _ | TTry _ | TSwitch _ | TWhile _ | TFor _ ->
 			had_effect := true;
-			Type.iter (loop true compiler_generated) e
-		| TMeta((Meta.CompilerGenerated,_,_),e1) ->
-			loop in_value true e1
+			Type.iter (loop true) e
 		| TParenthesis e1 | TMeta(_,e1) ->
-			loop in_value compiler_generated e1
+			loop in_value e1
 		| TArray _ | TCast (_,None) | TBinop _ | TUnop _
 		| TField _ | TArrayDecl _ | TObjectDecl _ when in_value ->
-			Type.iter (loop true compiler_generated) e;
-		| TArray(e1,e2) -> compound compiler_generated "array access" [e1;e2] e.epos
-		| TCast(e1,None) -> compound compiler_generated "cast" [e1] e.epos
-		| TBinop(op,e1,e2) -> compound compiler_generated (Printf.sprintf "'%s' operator" (s_binop op)) [e1;e2] e.epos
-		| TUnop(op,_,e1) -> compound compiler_generated (Printf.sprintf "'%s' operator" (s_unop op)) [e1] e.epos
-		| TField(e1,_) -> compound compiler_generated "field access" [e1] e.epos
-		| TArrayDecl el -> compound compiler_generated "array declaration" el e.epos
-		| TObjectDecl fl -> compound compiler_generated "object declaration" (List.map snd fl) e.epos
+			Type.iter (loop true) e;
+		| TArray(e1,e2) -> compound "array access" [e1;e2] e.epos
+		| TCast(e1,None) -> compound "cast" [e1] e.epos
+		| TBinop(op,e1,e2) -> compound (Printf.sprintf "'%s' operator" (s_binop op)) [e1;e2] e.epos
+		| TUnop(op,_,e1) -> compound (Printf.sprintf "'%s' operator" (s_unop op)) [e1] e.epos
+		| TField(e1,_) -> compound "field access" [e1] e.epos
+		| TArrayDecl el -> compound "array declaration" el e.epos
+		| TObjectDecl fl -> compound "object declaration" (List.map snd fl) e.epos
 	in
-	loop true false e
+	loop true e
 
 let prepare_field dctx dectx com cf = match cf.cf_expr with
 	| None -> ()
@@ -177,9 +175,6 @@ let prepare com =
 	dctx.unresolved_identifiers <- com.display_information.unresolved_identifiers;
 	dctx
 
-let secure_generated_code ctx e =
-	if is_diagnostics ctx then mk (TMeta((Meta.CompilerGenerated,[],e.epos),e)) e.etype e.epos else e
-
 let print com =
 	let dctx = prepare com in
 	Json.string_of_json (DiagnosticsPrinter.json_of_diagnostics com dctx)

diff --git a/src/core/displayTypes.ml b/src/core/displayTypes.ml
@@ -233,7 +233,6 @@ module DisplayMode = struct
 		| DMDefault | DMDefinition | DMTypeDefinition | DMPackage | DMHover | DMSignature -> settings
 		| DMUsage _ | DMImplementation -> { settings with
 				dms_full_typing = true;
-				dms_populate_cache = !ServerConfig.populate_cache_from_display;
 				dms_force_macro_typing = true;
 				dms_display_file_policy = DFPAlso;
 				dms_exit_during_typing = false

diff --git a/src/core/tType.ml b/src/core/tType.ml
@@ -467,11 +467,12 @@ let flag_tclass_field_names = [
 type flag_tvar =
 	| VCaptured
 	| VFinal
-	| VUsed (* used by the analyzer *)
+	| VAnalyzed
 	| VAssigned
 	| VCaught
 	| VStatic
+	| VUsedByTyper (* Set if the typer looked up this variable *)
 
 let flag_tvar_names = [
-	"VCaptured";"VFinal";"VUsed";"VAssigned";"VCaught";"VStatic"
+	"VCaptured";"VFinal";"VAnalyzed";"VAssigned";"VCaught";"VStatic";"VUsedByTyper"
 ]
diff --git a/src/filters/filters.ml b/src/filters/filters.ml
@@ -912,7 +912,7 @@ let save_class_state com t =
 			a.a_meta <- List.filter (fun (m,_,_) -> m <> Meta.ValueUsed) a.a_meta
 		)
 
-let run tctx main =
+let run tctx main before_destruction =
 	let com = tctx.com in
 	let detail_times = (try int_of_string (Common.defined_value_safe com ~default:"0" Define.FilterTimes) with _ -> 0) in
 	let new_types = List.filter (fun t ->
@@ -1022,4 +1022,5 @@ let run tctx main =
 	with_timer detail_times "callbacks" None (fun () ->
 		com.callbacks#run com.error_ext com.callbacks#get_after_save;
 	);
+	before_destruction();
 	destruction tctx detail_times main locals
diff --git a/src/optimization/analyzer.ml b/src/optimization/analyzer.ml
@@ -661,22 +661,22 @@ module LocalDce = struct
 
 	let apply ctx =
 		let is_used v =
-			has_var_flag v VUsed
+			has_var_flag v VAnalyzed
 		in
 		let keep v =
 			is_used v || ((match v.v_kind with VUser _ | VInlined -> true | _ -> false) && not ctx.config.local_dce) || ExtType.has_reference_semantics v.v_type || has_var_flag v VCaptured || Meta.has Meta.This v.v_meta
 		in
 		let rec use v =
 			if not (is_used v) then begin
-				add_var_flag v VUsed;
+				add_var_flag v VAnalyzed;
 				(try expr (get_var_value ctx.graph v) with Not_found -> ());
 				begin match Ssa.get_reaching_def ctx.graph v with
 					| None ->
 						(* We don't want to fully recurse for the origin variable because we don't care about its
 						   reaching definition (issue #10972). Simply marking it as being used should be sufficient. *)
 						let v' = get_var_origin ctx.graph v in
 						if not (is_used v') then
-							add_var_flag v' VUsed
+							add_var_flag v' VAnalyzed
 					| Some v ->
 						use v;
 				end

diff --git a/src/optimization/inline.ml b/src/optimization/inline.ml
@@ -589,7 +589,6 @@ class inline_state ctx ethis params cf f p = object(self)
 				mk (TBlock (DynArray.to_list el)) tret e.epos
 		in
 		let e = inline_metadata e cf.cf_meta in
-		let e = Diagnostics.secure_generated_code ctx.com e in
 		if has_params then begin
 			let mt = map_type cf.cf_type in
 			let unify_func () = unify_raise mt (TFun (tl,tret)) p in

diff --git a/src/optimization/optimizer.ml b/src/optimization/optimizer.ml
@@ -224,8 +224,6 @@ let reduce_expr com e =
 			) l with
 			| [] -> ec
 			| l -> { e with eexpr = TBlock (List.rev (ec :: l)) })
-	| TMeta ((Meta.CompilerGenerated,_,_),ec) ->
-		{ ec with epos = e.epos }
 	| TParenthesis ec ->
 		{ ec with epos = e.epos }
 	| TTry (e,[]) ->

diff --git a/src/typing/callUnification.ml b/src/typing/callUnification.ml
@@ -503,7 +503,6 @@ object(self)
 			!ethis_f();
 			raise exc
 		in
-		let e = Diagnostics.secure_generated_code ctx.com e in
 		ctx.com.error_ext <- old;
 		!ethis_f();
 		e

diff --git a/src/typing/operators.ml b/src/typing/operators.ml
@@ -88,16 +88,14 @@ module BinopResult = struct
 		| BinopSpecial(_,needs_assign) -> needs_assign
 end
 
-let rec check_assign ctx e =
+let check_assign ctx e =
 	match e.eexpr with
 	| TLocal v when has_var_flag v VFinal && not (Common.ignore_error ctx.com) ->
 		raise_typing_error "Cannot assign to final" e.epos
 	| TLocal {v_extra = None} | TArray _ | TField _ | TIdent _ ->
 		()
 	| TConst TThis | TTypeExpr _ when ctx.untyped ->
 		()
-	| TMeta ((Meta.CompilerGenerated,_,_),e1) ->
-		check_assign ctx e1
 	| _ ->
 		if not (Common.ignore_error ctx.com) then
 			invalid_assign e.epos

diff --git a/src/typing/typer.ml b/src/typing/typer.ml
@@ -422,6 +422,7 @@ let rec type_ident_raise ctx i p mode with_type =
 	| _ ->
 	try
 		let v = PMap.find i ctx.locals in
+		add_var_flag v VUsedByTyper;
 		(match v.v_extra with
 		| Some ve ->
 			let (params,e) = (ve.v_params,ve.v_expr) in
@@ -1062,7 +1063,7 @@ and type_new ctx (path,p_path) el with_type force_inline p =
 		raise_typing_error (s_type (print_context()) t ^ " cannot be constructed") p
 	end with Error ({ err_message = No_constructor _ } as err) when ctx.com.display.dms_kind <> DMNone ->
 		display_error_ext ctx.com err;
-		Diagnostics.secure_generated_code ctx.com (mk (TConst TNull) t p)
+		mk (TConst TNull) t p
 
 and type_try ctx e1 catches with_type p =
 	let e1 = type_expr ctx (Expr.ensure_block e1) with_type in
@@ -1744,7 +1745,7 @@ and type_call_builtin ctx e el mode with_type p =
 				s
 		in
 		warning ctx WInfo s e1.epos;
-		Diagnostics.secure_generated_code ctx.com e1
+		e1
 	| (EField(e,"match",efk_todo),p), [epat] ->
 		let et = type_expr ctx e WithType.value in
 		let rec has_enum_match t = match follow t with

diff --git a/src/typing/typerBase.ml b/src/typing/typerBase.ml
@@ -155,9 +155,11 @@ let get_this ctx p =
 	| FunMemberClassLocal | FunMemberAbstractLocal ->
 		let v = match ctx.vthis with
 			| None ->
-				let v = if ctx.curfun = FunMemberAbstractLocal then
-					PMap.find "this" ctx.locals
-				else
+				let v = if ctx.curfun = FunMemberAbstractLocal then begin
+					let v = PMap.find "this" ctx.locals in
+					add_var_flag v VUsedByTyper;
+					v
+				end else
 					add_local ctx VGenerated (Printf.sprintf "%sthis" gen_local_prefix) ctx.tthis p
 				in
 				ctx.vthis <- Some v;

diff --git a/tests/display/src/cases/Issue11203.hx → .../display/src/cases/Issue11203.hx.disabled b/tests/display/src/cases/Issue11203.hx → .../display/src/cases/Issue11203.hx.disabled
diff --git a/tests/display/src/cases/Issue5306.hx b/tests/display/src/cases/Issue5306.hx
@@ -7,8 +7,8 @@ class Issue5306 extends DisplayTestCase {
 		class Main {
 			static function main() {
 				var ib:Array<Int>;
-				ib[0] = 0; ib[1] = 1; ib[2]
-				{-5-}trace{-6-}("test");
+				{-5-}ib{-6-}[0] = 0; ib[1] = 1; ib[2]
+				{-7-}trace{-8-}("test");
 			}
 		}
 	**/
@@ -22,7 +22,7 @@ class Issue5306 extends DisplayTestCase {
 			// },
 			{
 				kind: DKParserError,
-				range: diagnosticsRange(pos(5), pos(6)),
+				range: diagnosticsRange(pos(7), pos(8)),
 				severity: Error,
 				code: null,
 				relatedInformation: [],
@@ -35,6 +35,14 @@ class Issue5306 extends DisplayTestCase {
 				code: null,
 				relatedInformation: [],
 				args: "Type not found : InvalidType"
+			},
+			{
+				kind: DKCompilerError,
+				range: diagnosticsRange(pos(5), pos(6)),
+				severity: Error,
+				code: null,
+				relatedInformation: [],
+				args: "Local variable ib used without being initialized"
 			}
 		];
 		arrayEq(expected, diagnostics());

diff --git a/tests/server/src/cases/issues/Issue11177.hx b/tests/server/src/cases/issues/Issue11177.hx
@@ -1,19 +1,18 @@
 package cases.issues;
 
 class Issue11177 extends TestCase {
-	// Disabled for now until #11177 is actually fixed, likely by #11220
-	// function test(_) {
-	// 	vfs.putContent("Main.hx", getTemplate("issues/Issue11177/Main.hx"));
-	// 	vfs.putContent("Buttons.hx", getTemplate("issues/Issue11177/Buttons.hx"));
-	// 	vfs.putContent("KeyCode.hx", getTemplate("issues/Issue11177/KeyCode.hx"));
-	// 	var args = ["-main", "Main", "--interp"];
-	// 	runHaxe(args.concat(["--display", "Buttons.hx@0@diagnostics"]));
-	// 	vfs.putContent("Main.hx", getTemplate("issues/Issue11177/Main2.hx"));
-	// 	runHaxeJson([], ServerMethods.Invalidate, {file: new FsPath("Main.hx")});
-	// 	runHaxe(args);
-	// 	runHaxe(args.concat(["--display", "Buttons.hx@0@diagnostics"]));
-	// 	Assert.isTrue(lastResult.stderr.length == 2);
-	// }
+	function test(_) {
+		vfs.putContent("Main.hx", getTemplate("issues/Issue11177/Main.hx"));
+		vfs.putContent("Buttons.hx", getTemplate("issues/Issue11177/Buttons.hx"));
+		vfs.putContent("KeyCode.hx", getTemplate("issues/Issue11177/KeyCode.hx"));
+		var args = ["-main", "Main", "--interp"];
+		runHaxe(args.concat(["--display", "Buttons.hx@0@diagnostics"]));
+		vfs.putContent("Main.hx", getTemplate("issues/Issue11177/Main2.hx"));
+		runHaxeJson([], ServerMethods.Invalidate, {file: new FsPath("Main.hx")});
+		runHaxe(args);
+		runHaxe(args.concat(["--display", "Buttons.hx@0@diagnostics"]));
+		Assert.isTrue(lastResult.stderr.length == 2);
+	}
 
 	function testWithoutCacheFromDisplay(_) {
 		vfs.putContent("Main.hx", getTemplate("issues/Issue11177/Main.hx"));

diff --git a/tests/server/src/cases/issues/Issue11184.hx b/tests/server/src/cases/issues/Issue11184.hx
@@ -1,16 +1,19 @@
 package cases.issues;
 
 class Issue11184 extends TestCase {
-	// Disabled for now until #11184 is actually fixed, likely by #11220
-	// function test(_) {
-	// 	vfs.putContent("Main.hx", getTemplate("issues/Issue11184/Main.hx"));
-	// 	var args = ["-main", "Main", "-js", "bin/test.js"];
-	// 	runHaxe(args.concat(["--display", "Main.hx@0@diagnostics"]));
-	// 	runHaxe(args);
-	// 	Assert.isTrue(hasErrorMessage("Cannot use Void as value"));
-	// 	runHaxe(args);
-	// 	Assert.isTrue(hasErrorMessage("Cannot use Void as value"));
-	// }
+	function testDiagnostics(_) {
+		vfs.putContent("Main.hx", getTemplate("issues/Issue11184/Main.hx"));
+		var args = ["-main", "Main", "-js", "bin/test.js"];
+
+		runHaxe(args.concat(["--display", "Main.hx@0@diagnostics"]));
+		final diagnostics = haxe.Json.parse(lastResult.stderr)[0].diagnostics;
+		Assert.equals(diagnostics[0].args, "Cannot use Void as value");
+
+		runHaxe(args);
+		Assert.isTrue(hasErrorMessage("Cannot use Void as value"));
+		runHaxe(args);
+		Assert.isTrue(hasErrorMessage("Cannot use Void as value"));
+	}
 
 	function testWithoutCacheFromDisplay(_) {
 		vfs.putContent("Main.hx", getTemplate("issues/Issue11184/Main.hx"));

diff --git a/tests/server/src/cases/issues/Issue11203.hx b/tests/server/src/cases/issues/Issue11203.hx
@@ -0,0 +1,23 @@
+package cases.issues;
+
+class Issue11203 extends TestCase {
+	function testClass(_) {
+		vfs.putContent("Main.hx", getTemplate("issues/Issue11203/MainClass.hx"));
+		var args = ["Main", "--interp"];
+		runHaxe(args);
+		runHaxe(args.concat(["--display", "Main.hx@0@diagnostics"]));
+
+		var diag = parseDiagnostics();
+		Assert.isTrue(diag.length == 0);
+	}
+
+	function testAbstract(_) {
+		vfs.putContent("Main.hx", getTemplate("issues/Issue11203/MainAbstract.hx"));
+		var args = ["Main", "--interp"];
+		runHaxe(args);
+		runHaxe(args.concat(["--display", "Main.hx@0@diagnostics"]));
+
+		var diag = parseDiagnostics();
+		Assert.isTrue(diag.length == 0);
+	}
+}