Fix false positive on shadowed variable lint due to lambdas

Wilfred · facebook-github-bot · commit 21afafcb3f19 · 2022-08-29T13:06:09.000-07:00
Summary: The shadowed variable lint is intended to warn users when they're unexpectedly overwriting a local variable by their `foreach` or `catch` block. ``` function demo(): void { $x = 99; foreach (vec[1, 2] as $x) { } $x; // 2 here } ``` However, we previously ignored that lambdas introduce a new scope. Ensure that we track local variables inside lambdas separately. Closes #8986 Reviewed By: mpu Differential Revision: D39003052 fbshipit-source-id: 6d6c0cf32dcea57bb0e7437e396631a00df7975f
diff --git a/hphp/hack/src/lints/linter_foreach_shadow.ml b/hphp/hack/src/lints/linter_foreach_shadow.ml
@@ -10,6 +10,21 @@
 open Linting_visitors
 open Aast
 
+(**
+   Lint on loop or catch variables that shadow local variables.
+
+   function foo(): void {
+     $x = 99;
+
+     // Lint on this $x:
+     foreach (vec[1, 2] as $x) {
+     }
+
+     // $x is 2 here.
+     $x;
+   }
+ *)
+
 let lvar_visitor =
   object
     inherit [lid list] Nast.Visitor_DEPRECATED.visitor
@@ -75,6 +90,16 @@ module VisitorFunctor (Parent : BodyVisitorModule) : BodyVisitorModule = struct
 
       method! on_catch () (_, var, block) =
         this#on_block_with_local_vars [var] block
+
+      method! on_expr () ((_, _, e_) as e) =
+        match e_ with
+        | Efun _
+        | Lfun _ ->
+          let old_frames = frames in
+          frames <- [[]];
+          parent#on_expr () e;
+          frames <- old_frames
+        | _ -> parent#on_expr () e
     end
 end
 
diff --git a/hphp/hack/test/lint/foreach_shadow.php b/hphp/hack/test/lint/foreach_shadow.php
@@ -1,14 +1,14 @@
 <?hh
 
-function f1(array<int> $xs): int {
+function f1(vec<int> $xs): int {
   $x = 0;
   foreach ($xs as $x) {
     $x++;
   }
   return $x;
 }
 
-function f2(array<int> $xs): int {
+function f2(vec<int> $xs): int {
   $x = 1;
   $x = $x - 1;
   foreach ($xs as $x) {
@@ -17,31 +17,31 @@ function f2(array<int> $xs): int {
   return $x;
 }
 
-function f3(array<int, int> $xs): int {
+function f3(dict<int, int> $xs): int {
   $x = 0;
   foreach ($xs as $x => $_) {
     $x++;
   }
   return $x;
 }
 
-function f4(array<int, int> $xs): int {
+function f4(dict<int, int> $xs): int {
   $x = 0;
   foreach ($xs as $_ => $x) {
     $x++;
   }
   return $x;
 }
 
-function f5(array<int, (int, int)> $xs): int {
+function f5(dict<int, (int, int)> $xs): int {
   $x = 0;
   foreach ($xs as $_ => list($x, $_)) {
     $x++;
   }
   return $x;
 }
 
-function f6(array<int> $xs): int {
+function f6(vec<int> $xs): int {
   $x = 0;
   if (true) {
     foreach ($xs as $x) {
@@ -51,7 +51,7 @@ function f6(array<int> $xs): int {
   return $x;
 }
 
-function f7(array<array<int>> $xss): int {
+function f7(vec<vec<int>> $xss): int {
   $s = 0;
   foreach ($xss as $xs) {
     $x = 0;
@@ -63,8 +63,8 @@ function f7(array<array<int>> $xss): int {
   return $x;
 }
 
-function f8(array<int> $xs): array<(int, int)> {
-  $result = varray[];
+function f8(vec<int> $xs): vec<(int, int)> {
+  $result = vec[];
   foreach ($xs as $x) {
     $x1 = $x;
     foreach ($xs as $x) {
@@ -75,15 +75,15 @@ function f8(array<int> $xs): array<(int, int)> {
   return $result;
 }
 
-function f9(array<int> $xs): int {
-  list($x, $_) = varray[0, 0];
+function f9(vec<int> $xs): int {
+  list($x, $_) = vec[0, 0];
   foreach ($xs as $x) {
     $x++;
   }
   return $x;
 }
 
-function f10(array<int> $xs): int {
+function f10(vec<int> $xs): int {
   $s = 0;
   try {
     throw new Exception();
@@ -96,7 +96,7 @@ function f10(array<int> $xs): int {
   return $s;
 }
 
-function f11(array<int> $xs): int {
+function f11(vec<int> $xs): int {
   $s = 0;
   // The loop variable $x doesn't shadow $x defined after the loop.
   foreach ($xs as $x) {
@@ -107,8 +107,8 @@ function f11(array<int> $xs): int {
   return $s;
 }
 
-function f12(array<int> $xs): int {
-  if (false) {
+function f12(vec<int> $xs): int {
+  if (1 === 2) {
     $x = 0;
     $s = $x;
   } else {
@@ -122,7 +122,7 @@ function f12(array<int> $xs): int {
   return $s;
 }
 
-function f13(array<int, array<int>> $xss): int {
+function f13(dict<int, vec<int>> $xss): int {
   $s = 0;
   foreach ($xss as $_ => $xs) {
     // Variables named $_ are ignored
diff --git a/hphp/hack/test/lint/foreach_shadow.php.exp b/hphp/hack/test/lint/foreach_shadow.php.exp
@@ -28,5 +28,3 @@ File "foreach_shadow.php", line 79, characters 8-9: (Lint[5568])
 File "foreach_shadow.php", line 92, characters 21-22:
 Loop variable `$x` shadows a local variable defined or last assigned here:
 File "foreach_shadow.php", line 90, characters 22-23: (Lint[5568])
-File "foreach_shadow.php", line 112, characters 5-11:
-In function \f12: This assignment may be unreachable. It appears after a condition which is always false. (Other[6001])
diff --git a/hphp/hack/test/lint/foreach_shadow_lambda.php b/hphp/hack/test/lint/foreach_shadow_lambda.php
@@ -0,0 +1,15 @@
+<?hh
+
+function foo(): void {
+  $x = 0;
+
+  $f = () ==> {
+    // This does not shadow the local in the outer scope, as the outer
+    // $x is not affected.
+    foreach (vec[] as $x) {
+    }
+  };
+}
+
+// A second function to silence the naming lint about single definition files.
+function bar(): void {}
diff --git a/hphp/hack/test/lint/foreach_shadow_lambda.php.exp b/hphp/hack/test/lint/foreach_shadow_lambda.php.exp
@@ -0,0 +1 @@
+No lint errors
