rework `rename_local_vars` (closes #3344, see #4805)

ast.ml

@@ -24,6 +24,7 @@ type pos = {
 }
 
 module IntMap = Map.Make(struct type t = int let compare a b = a - b end)
+module StringMap = Map.Make(struct type t = string let compare = String.compare end)
 
 module Meta = struct
 	type strict_meta =

common.ml

@@ -74,12 +74,6 @@ type platform_config = {
 	pf_static : bool;
 	(** has access to the "sys" package *)
 	pf_sys : bool;
-	(** local variables are block-scoped *)
-	pf_locals_scope : bool;
-	(** captured local variables are scoped *)
-	pf_captured_scope : bool;
-	(** generated locals must be absolutely unique wrt the current function *)
-	pf_unique_locals : bool;
 	(** captured variables handling (see before) *)
 	pf_capture_policy : capture_policy;
 	(** when calling a method with optional args, do we replace the missing args with "null" constants *)
@@ -546,9 +540,6 @@ let default_config =
 	{
 		pf_static = true;
 		pf_sys = true;
-		pf_locals_scope = true;
-		pf_captured_scope = true;
-		pf_unique_locals = false;
 		pf_capture_policy = CPNone;
 		pf_pad_nulls = false;
 		pf_add_final_return = false;
@@ -567,9 +558,6 @@ let get_config com =
 		{
 			pf_static = false;
 			pf_sys = false;
-			pf_locals_scope = false;
-			pf_captured_scope = false;
-			pf_unique_locals = false;
 			pf_capture_policy = CPLoopVars;
 			pf_pad_nulls = false;
 			pf_add_final_return = false;
@@ -582,9 +570,6 @@ let get_config com =
 		{
 			pf_static = false;
 			pf_sys = true;
-			pf_locals_scope = true;
-			pf_captured_scope = true;
-			pf_unique_locals = false;
 			pf_capture_policy = CPNone;
 			pf_pad_nulls = true;
 			pf_add_final_return = false;
@@ -597,9 +582,6 @@ let get_config com =
 		{
 			pf_static = true;
 			pf_sys = false;
-			pf_locals_scope = false;
-			pf_captured_scope = true;
-			pf_unique_locals = true;
 			pf_capture_policy = CPLoopVars;
 			pf_pad_nulls = false;
 			pf_add_final_return = true;
@@ -612,9 +594,6 @@ let get_config com =
 		{
 			pf_static = true;
 			pf_sys = false;
-			pf_locals_scope = true;
-			pf_captured_scope = true; (* handled by genSwf9 *)
-			pf_unique_locals = false;
 			pf_capture_policy = CPLoopVars;
 			pf_pad_nulls = false;
 			pf_add_final_return = false;
@@ -627,9 +606,6 @@ let get_config com =
 		{
 			pf_static = false;
 			pf_sys = true;
-			pf_locals_scope = false; (* some duplicate work is done in genPhp *)
-			pf_captured_scope = false;
-			pf_unique_locals = false;
 			pf_capture_policy = CPNone;
 			pf_pad_nulls = true;
 			pf_add_final_return = false;
@@ -642,9 +618,6 @@ let get_config com =
 		{
 			pf_static = true;
 			pf_sys = true;
-			pf_locals_scope = true;
-			pf_captured_scope = true;
-			pf_unique_locals = false;
 			pf_capture_policy = CPWrapRef;
 			pf_pad_nulls = true;
 			pf_add_final_return = true;
@@ -657,9 +630,6 @@ let get_config com =
 		{
 			pf_static = true;
 			pf_sys = true;
-			pf_locals_scope = false;
-			pf_captured_scope = true;
-			pf_unique_locals = true;
 			pf_capture_policy = CPWrapRef;
 			pf_pad_nulls = true;
 			pf_add_final_return = false;
@@ -672,9 +642,6 @@ let get_config com =
 		{
 			pf_static = true;
 			pf_sys = true;
-			pf_locals_scope = false;
-			pf_captured_scope = true;
-			pf_unique_locals = false;
 			pf_capture_policy = CPWrapRef;
 			pf_pad_nulls = true;
 			pf_add_final_return = false;
@@ -687,9 +654,6 @@ let get_config com =
 		{
 			pf_static = false;
 			pf_sys = true;
-			pf_locals_scope = false;
-			pf_captured_scope = false;
-			pf_unique_locals = false;
 			pf_capture_policy = CPLoopVars;
 			pf_pad_nulls = false;
 			pf_add_final_return = false;

filters.ml

@@ -522,71 +522,17 @@ let captured_vars com e =
 (* RENAME LOCAL VARS *)
 
 let rename_local_vars ctx e =
-	let cfg = ctx.com.config in
-	let all_scope = (not cfg.pf_captured_scope) || (not cfg.pf_locals_scope) in
-	let vars = ref PMap.empty in
-	let all_vars = ref PMap.empty in
-	let vtemp = alloc_var "~" t_dynamic in
-	let rebuild_vars = ref false in
-	let rebuild m =
-		PMap.fold (fun v acc -> PMap.add v.v_name v acc) m PMap.empty
+	let vars = ref [] in
+	let declare v =
+		vars := v :: !vars
 	in
-	let save() =
-		let old = !vars in
-		if cfg.pf_unique_locals || not cfg.pf_locals_scope then (fun() -> ()) else (fun() -> vars := if !rebuild_vars then rebuild old else old)
+	let reserved = ref StringMap.empty in
+	let reserve name =
+		reserved := StringMap.add name true !reserved
 	in
-	let count = ref 1 in
-	let rename vars v =
-		while PMap.mem (v.v_name ^ string_of_int !count) vars do
-			incr count;
-		done;
-		begin match ctx.com.platform with
-			| Cpp ->
-				if not (Meta.has Meta.RealPath v.v_meta) then
-					v.v_meta <- (Meta.RealPath,[EConst (String v.v_name),e.epos],e.epos) :: v.v_meta
-			| _ ->
-				()
-		end;
-		v.v_name <- v.v_name ^ string_of_int !count;
-	in
-	let declare v p =
-		(match follow v.v_type with
-			| TAbstract ({a_path = [],"Void"},_) -> error "Arguments and variables of type Void are not allowed" p
-			| _ -> ());
-		(* chop escape char for all local variables generated *)
-		if is_gen_local v then v.v_name <- "_g" ^ String.sub v.v_name 1 (String.length v.v_name - 1);
-		let look_vars = (if not cfg.pf_captured_scope && v.v_capture then !all_vars else !vars) in
-		(try
-			let v2 = PMap.find v.v_name look_vars in
-			(*
-				block_vars will create some wrapper-functions that are declaring
-				the same variable twice. In that case do not perform a rename since
-				we are sure it's actually the same variable
-			*)
-			if v == v2 then raise Not_found;
-			rename look_vars v;
-		with Not_found ->
-			());
-		vars := PMap.add v.v_name v !vars;
-		if all_scope then all_vars := PMap.add v.v_name v !all_vars;
-	in
-	(*
-		This is quite a rare case, when a local variable would otherwise prevent
-		accessing a type because it masks the type value or the package name.
-	*)
 	let check t =
 		match (t_infos t).mt_path with
-		| [], name | name :: _, _ ->
-			let vars = if cfg.pf_locals_scope then vars else all_vars in
-			(try
-				let v = PMap.find name !vars in
-				if v == vtemp then raise Not_found; (* ignore *)
-				rename (!vars) v;
-				rebuild_vars := true;
-				vars := PMap.add v.v_name v !vars
-			with Not_found ->
-				());
-			vars := PMap.add name vtemp !vars
+		| [], name | name :: _, _ -> reserve name
 	in
 	let check_type t =
 		match follow t with
@@ -596,71 +542,62 @@ let rename_local_vars ctx e =
 		| TAbstract (a,_) -> check (TAbstractDecl a)
 		| TMono _ | TLazy _ | TAnon _ | TDynamic _ | TFun _ -> ()
 	in
-	let rec loop e =
-		match e.eexpr with
-		| TVar (v,eo) ->
-			if not cfg.pf_locals_scope then declare v e.epos;
-			(match eo with None -> () | Some e -> loop e);
-			if cfg.pf_locals_scope then declare v e.epos;
-		| TFunction tf ->
-			let old = save() in
-			List.iter (fun (v,_) -> declare v e.epos) tf.tf_args;
-			loop tf.tf_expr;
-			old()
-		| TBlock el ->
-			let old = save() in
-			(* we have to look ahead for vars on these targets (issue #3344) *)
-			begin match ctx.com.platform with
-				| Js ->
-					let rec check_var e = match e.eexpr with
-						| TVar (v,eo) ->
-							(match eo with None -> () | Some e -> loop e);
-							declare v e.epos
-						| TBlock _ ->
-							()
-						| _ ->
-							Type.iter check_var e
-					in
-					List.iter check_var el
-				| _ ->
-					()
-			end;
-			List.iter loop el;
-			old()
-		| TFor (v,it,e1) ->
-			loop it;
-			let old = save() in
-			declare v e.epos;
-			loop e1;
-			old()
-		| TTry (e,catchs) ->
-			loop e;
+	let rec collect e = match e.eexpr with
+ 		| TVar(v,eo) ->
+			declare v;
+			(match eo with None -> () | Some e -> collect e)
+		| TFor(v,e1,e2) ->
+			declare v;
+			collect e1;
+			collect e2;
+		| TTry(e1,catches) ->
+			collect e1;
 			List.iter (fun (v,e) ->
-				let old = save() in
-				declare v e.epos;
+				declare v;
 				check_type v.v_type;
-				loop e;
-				old()
-			) catchs;
+				collect e
+			) catches
+		| TFunction tf ->
+			List.iter (fun (v,_) -> declare v) tf.tf_args;
+			collect tf.tf_expr
 		| TTypeExpr t ->
 			check t
 		| TNew (c,_,_) ->
-			Type.iter loop e;
+			Type.iter collect e;
 			check (TClassDecl c);
 		| TCast (e,Some t) ->
-			loop e;
+			collect e;
 			check t;
 		| TConst TSuper ->
 			check_type e.etype
 		| _ ->
-			Type.iter loop e
+			Type.iter collect e
 	in
-	declare (alloc_var "this" t_dynamic) Ast.null_pos; (* force renaming of 'this' vars in abstract *)
-	if ctx.com.platform = Java then declare (alloc_var "_" t_dynamic) Ast.null_pos; (* Java 8 hates _ *)
+	(* Pass 1: Collect used identifiers and variables. *)
+	reserve "this";
+	if ctx.com.platform = Java then reserve "_";
 	begin match ctx.curclass.cl_path with
-		| s :: _,_ | [],s -> declare (alloc_var s t_dynamic) Ast.null_pos
+		| s :: _,_ | [],s -> reserve s
 	end;
-	loop e;
+	collect e;
+	(* Pass 2: Check and rename variables. *)
+	let count_table = Hashtbl.create 0 in
+	let maybe_rename v =
+		(* chop escape char for all local variables generated *)
+		if is_gen_local v then v.v_name <- "_g" ^ String.sub v.v_name 1 (String.length v.v_name - 1);
+		let name = ref v.v_name in
+		let count = ref (try Hashtbl.find count_table v.v_name with Not_found -> 0) in
+		while StringMap.mem !name !reserved do
+			incr count;
+			name := v.v_name ^ (string_of_int !count);
+		done;
+		reserve !name;
+		Hashtbl.replace count_table v.v_name !count;
+		if not (Meta.has Meta.RealPath v.v_meta) then
+			v.v_meta <- (Meta.RealPath,[EConst (String v.v_name),e.epos],e.epos) :: v.v_meta;
+		v.v_name <- !name;
+	in
+	List.iter maybe_rename (List.rev !vars);
 	e
 
 let check_unification ctx e t =