From d2af3e8ca16ef9e6beae3f7645d0d11ef5bffb5c Mon Sep 17 00:00:00 2001
From: jrfnl <jrfnl@users.noreply.github.com>
Date: Wed, 15 Jan 2020 22:42:33 +0100
Subject: [PATCH] Bug fix: $this in nested function declaration

While `$this` can be used freely within classes, traits and closures, if a nested closed scope is defined within any of these, `$this` is not inherited and should be considered undefined.

As things were, the sniff would not throw a warning for this (false negative).
This commit fixes this.

Includes unit tests.

Includes removing an outdated comment. `T_CLOSURE` works perfectly fine with the conditions array (and has for quite a few years).

Fixes 109
---
 .../CodeAnalysis/VariableAnalysisSniff.php    | 13 +++-
 .../CodeAnalysis/VariableAnalysisTest.php     | 16 +++++
 .../ThisWithinNestedClosedScopeFixture.php    | 67 +++++++++++++++++++
 3 files changed, 94 insertions(+), 2 deletions(-)
 create mode 100644 VariableAnalysis/Tests/CodeAnalysis/fixtures/ThisWithinNestedClosedScopeFixture.php

diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
index d0f60fb8..51724827 100644
--- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
+++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
@@ -605,15 +605,24 @@ protected function checkForThisWithinClass(File $phpcsFile, $stackPtr, $varName)
       return false;
     }
 
+    $inFunction = false;
     foreach (array_reverse($token['conditions'], true) as $scopePtr => $scopeCode) {
       //  $this within a closure is valid
-      //  Note: have to fetch code from $tokens, T_CLOSURE isn't set for conditions codes.
-      if ($tokens[$scopePtr]['code'] === T_CLOSURE) {
+      if ($scopeCode === T_CLOSURE && $inFunction === false) {
         return true;
       }
       if ($scopeCode === T_CLASS || $scopeCode === T_ANON_CLASS || $scopeCode === T_TRAIT) {
         return true;
       }
+
+      // Handle nested function declarations.
+      if ($scopeCode === T_FUNCTION) {
+        if ($inFunction === true) {
+            break;
+        }
+
+        $inFunction = true;
+      }
     }
 
     return false;
diff --git a/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php b/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php
index 5ac9bb5f..369029a7 100644
--- a/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php
+++ b/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php
@@ -907,4 +907,20 @@ public function testGetDefinedVarsCountsAsRead() {
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
+
+  public function testThisWithinNestedClosedScope() {
+    $fixtureFile = $this->getFixture('ThisWithinNestedClosedScopeFixture.php');
+    $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile);
+    $phpcsFile->process();
+    $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
+    $expectedWarnings = [
+            5,
+            8,
+            20,
+            33,
+            47,
+            61,
+    ];
+    $this->assertEquals($expectedWarnings, $lines);
+  }
 }
diff --git a/VariableAnalysis/Tests/CodeAnalysis/fixtures/ThisWithinNestedClosedScopeFixture.php b/VariableAnalysis/Tests/CodeAnalysis/fixtures/ThisWithinNestedClosedScopeFixture.php
new file mode 100644
index 00000000..a55ab07d
--- /dev/null
+++ b/VariableAnalysis/Tests/CodeAnalysis/fixtures/ThisWithinNestedClosedScopeFixture.php
@@ -0,0 +1,67 @@
+<?php
+
+function foo() {
+    // Using $this will not work.
+    if ($this->something) {
+        function nestedFunctionDeclaration() {
+            // Using $this here will also not work as the nested function is a closed scope in the global namespace.
+            if ($this->something) {
+                // Do something.
+            }
+        }
+    }
+};
+
+$closure = function() {
+    // Using $this here is fine.
+    if ($this->something) {
+        function nestedFunctionDeclaration() {
+            // Using $this here is not ok as the nested function is a closed scope in the global namespace.
+            if ($this->something) {
+                // Do something.
+            }
+        }
+    }
+};
+
+class Foo {
+    public function bar() {
+        // Using $this here is fine.
+        if ($this->something) {
+            function nestedFunctionDeclaration() {
+                // Using $this here is not ok as the nested function is a closed scope in the global namespace.
+                if ($this->something) {
+                    // Do something.
+                }
+            }
+        }
+    }
+}
+
+$anonClass = class() {
+    public function bar() {
+        // Using $this here is fine.
+        if ($this->something) {
+            function nestedFunctionDeclaration() {
+                // Using $this here is not ok as the nested function is a closed scope in the global namespace.
+                if ($this->something) {
+                    // Do something.
+                }
+            }
+        }
+    }
+}
+
+trait FooTrait {
+    public function bar() {
+        // Using $this here is fine.
+        if ($this->something) {
+            function nestedFunctionDeclaration() {
+                // Using $this here is not ok as the nested function is a closed scope in the global namespace.
+                if ($this->something) {
+                    // Do something.
+                }
+            }
+        }
+    }
+}