Prepare to become ProhibitUnusedVariable to default-enabled (#275)

* Upgrade Vital.vim

* Refactor scope_plugin internal

* Add a test for #274

* Visit into NodeType.CURLYNAMEEXPR

* NodeType.CURLYNAME should be classified as dynamic

* Refactor IdentifierClassifier internal

* Rename the fixture for #274 to prepare for a test for

* Analyze NodeType.CALL for `call` or `function` when the first arg is NodeType.STRING

* fixup! Add a test for #274

* Refactor scope_plugin internal

* Fix false positives that is referenced at #274 comments

* Support ignored patterns for ProhibitUnusedVariable

* Fix a typo

* "..." should be not reported by ProhibitUnusedVariable

* Initial implementation for line config comments

* Add a test for README

* Better debugging

* fixup! Initial implementation for line config comments

* fixup! Add a test for README

* Fix order of plugins

* Keep disabled

* Update README.rst

* Fix tests

* Change line config comment to next line config comment

* `traverse()` can visit into NodeType.LAMBDA

* fixup! Initial implementation for line config comments

* Fix bug that caused by unexpected side-sffect of

* Add `source_name` to config_dict to debugging

* Fix a bug that occurred when a next line is skipped

* Tweak debugging output

* Fix a bug that is caused by unexpected side-effects from multiple linting

* Support lambda

* Allow func arg names that have a same name as builtins

* Fix tests

* Fix a test

* Care Python 2

README.rst

@@ -143,10 +143,16 @@ You can enable/disable linting policies by a comment as following:
     let &cpo = s:save_cpo
     unlet s:save_cpo
 
-This syntax is: ``" vint: [+-]<PolicyName> [+-]<PolicyName> ...``.
+And you can use line config comments. It can enable/disable linting policies in only one line by the postfix comment:
 
-You can see all policy names on `Vint linting policy
-summary <https://github.com/Kuniwak/vint/wiki/Vint-linting-policy-summary>`__.
+.. code:: vim
+
+    " vint: next-line -ProhibitUnusedVariable
+    let s:foobar = 'x'
+    echo s:{'foo' . 'bar'}
+
+This syntax is: `" vint: [next-line] [+-]<PolicyName> [+-]<PolicyName> ...`.
+You can see all policy names on `Vint linting policy summary <https://github.com/Kuniwak/vint/wiki/Vint-linting-policy-summary>`__.
 
 Code health
 -----------

dev_tool/show_scope.py

@@ -28,15 +28,16 @@ def prettify_node_type(node):
     enable_neovim = namespace['enable_neovim']
 
     scope_plugin = ScopePlugin()
-    parser = Parser(plugins={'scope': scope_plugin}, enable_neovim=enable_neovim)
+    parser = Parser(plugins=[scope_plugin], enable_neovim=enable_neovim)
 
     for filepath in filepaths:
         ast = parser.parse_file(filepath)
         traverse(ast, on_enter=prettify_node_type)
 
-        print("////////// AST //////////\n")
-        pprint(ast)
+        print("////////// SCOPE TREE //////////\n")
+        pprint(scope_plugin._ref_tester._scope_linker.scope_tree)
         print("\n\n")
 
-        print("////////// SCOPE TREE //////////\n")
-        pprint(scope_plugin._ref_tester._scope_tree)
+        print("////////// LINK REGISTRY //////////\n")
+        pprint(scope_plugin._ref_tester._scope_linker.link_registry._vars_to_declarative_ids_map)
+        pprint(scope_plugin._ref_tester._scope_linker.link_registry._ids_to_scopes_map)

test/asserting/policy.py

@@ -2,66 +2,35 @@
 from pathlib import Path
 from pprint import pprint
 from vint.compat.itertools import zip_longest
+from vint.linting.policy_set import PolicySet
 from vint.linting.linter import Linter
-from vint.linting.config.config_default_source import ConfigDefaultSource
+from vint.linting.level import Level
 
 
 class PolicyAssertion(unittest.TestCase):
-    class StubPolicySet(object):
-        def __init__(self, *policies):
-            self._policies = policies
-
-
-        def get_enabled_policies(self):
-            return self._policies
-
-
-        def update_by_config(self, policy_enabling_map):
-            pass
-
-
-    class StubConfigContainer(object):
-        def __init__(self, policy_names_to_enable):
-
-            default_config_dict = ConfigDefaultSource(None).get_config_dict()
-            policy_options = default_config_dict.get('policies', {})
-
-            for policy, options in policy_options.items():
-                options['enabled'] = False
-
-            for policy in policy_names_to_enable:
-                options = policy_options.setdefault(policy, {})
-                options['enabled'] = True
-
-            self._config_dict = {
-                'policies': policy_options,
-            }
-
-
-        def append_config_source(self, config_source):
-            # Ignore a comment config source
-            pass
-
-
-        def get_config_dict(self):
-            return self._config_dict
-
-
     def assertFoundNoViolations(self, path, Policy, policy_options=None):
         self.assertFoundViolationsEqual(path, Policy, [], policy_options)
 
 
     def assertFoundViolationsEqual(self, path, Policy, expected_violations, policy_options=None):
-        policy_to_test = Policy()
         policy_name = Policy.__name__
-
-        policy_set = PolicyAssertion.StubPolicySet(policy_to_test)
-        config = PolicyAssertion.StubConfigContainer(policy_name)
+        policy_set = PolicySet([Policy])
+
+        config_dict = {
+            'cmdargs': {
+                'severity': Level.STYLE_PROBLEM,
+            },
+            'policies': {
+                policy_name: {
+                    'enabled': True,
+                }
+            },
+        }
 
         if policy_options is not None:
-            config.get_config_dict()['policies'][policy_name].update(policy_options)
+            config_dict['policies'][policy_name] = policy_options
 
-        linter = Linter(policy_set, config.get_config_dict())
+        linter = Linter(policy_set, config_dict)
         violations = linter.lint_file(path)
 
         pprint(violations)
@@ -74,13 +43,9 @@ def assertFoundViolationsEqual(self, path, Policy, expected_violations, policy_o
     def assertViolation(self, actual_violation, expected_violation):
         self.assertIsNot(actual_violation, None)
         self.assertIsNot(expected_violation, None)
-
-        pprint(actual_violation)
-
-        assert actual_violation['name'] == expected_violation['name']
-        assert actual_violation['position'] == expected_violation['position']
-        assert actual_violation['level'] == expected_violation['level']
-
+        self.assertEqual(actual_violation['name'], expected_violation['name'])
+        self.assertEqual(actual_violation['position'], expected_violation['position'])
+        self.assertEqual(actual_violation['level'], expected_violation['level'])
         self.assertIsInstance(actual_violation['description'], str)
 
 

test/fixture/ast/scope_plugin/fixture_to_scope_plugin_issue_274_call.vim

@@ -0,0 +1 @@
+call call('s:Foo', [])

test/fixture/ast/scope_plugin/fixture_to_scope_plugin_issue_274_curlyname.vim

@@ -0,0 +1 @@
+let s:{x} = ""

test/fixture/ast/scope_plugin/fixture_to_scope_plugin_issue_274_curlyname_complex.vim

@@ -0,0 +1,4 @@
+function! s:foo(x) abort
+    let x = 'x'
+    let y = l:{x}
+endfunction

test/fixture/ast/scope_plugin/fixture_to_scope_plugin_issue_274_function.vim

@@ -0,0 +1 @@
+call function('s:Foo')

test/fixture/ast/scope_plugin/fixture_to_scope_plugin_lambda_param.vim

@@ -0,0 +1 @@
+call map([], { i, ... -> y })

test/fixture/config/fixture_for_line_comment_lambda_string_expr.vim

@@ -0,0 +1,12 @@
+echo 'report me because I have no line config comments'
+
+" vint: next-line -ProhibitStringPolicy
+call map([], '"do not report me"')
+
+echo 'report me because I have no line config comments, but the previous line have it'
+
+" vint: next-line -ProhibitStringPolicy
+call map(
+    \ [],
+    \ '"report me because I have no line config comments, but the parent node have it"'
+    \)

test/fixture/config/fixture_for_line_comment_simple.vim

@@ -0,0 +1,6 @@
+let s:x = 'report me because I have no line config comments'
+
+" vint: next-line -ProhibitStringPolicy
+let s:x = 'do not report me because I have a line config comment'
+
+let s:x = 'report me because I have no line config comments, but the previous line have it'

test/fixture/policy/prohibit_implicit_scope_builtin_variable_valid.vim

@@ -45,7 +45,7 @@ call b:BufferLocalFunc()
 call w:WindowLocalFunc()
 call t:TabLocalFunc()
 
-function! g:FuncContext(param)
+function! FuncContext(param)
     echo a:0
     echo a:000
     echo a:param
@@ -68,3 +68,9 @@ echo b:
 echo w:
 echo t:
 echo v:
+
+
+" Allow function parameter that have same name as builtins
+function! FuncContext(count)
+    echo a:count
+endfunction

test/fixture/policy/prohibit_implicit_scope_variable_invalid.vim

@@ -1,12 +1,9 @@
 " Prohibit implicit scope variables
-let explicit_global_var = 101
+let implicit_global_var = 101
 
 redir => output
 redir END
 
-" Prohibit implicit builtin variables
-let count = 110
-
 function! ImplicitGlobalFunc(param)
     " Make it easy to fix missing a:
     echo param
@@ -15,3 +12,5 @@ endfunction
 " Implicit global variable 'i'
 for i in [1, 2, 3]
 endfor
+
+call { -> implicit_global_var_in_lambda }()

test/fixture/policy/prohibit_implicit_scope_variable_valid.vim

@@ -69,3 +69,6 @@ call ImplicitGlobalFunc()
 function! autoload#ImplicitGlobalAutoloadFunc()
 endfunction
 call autoload#ImplicitGlobalAutoloadFunc()
+
+" Lambda arguments can not have any scope prefix
+call { i, ... -> i }(0)

test/fixture/policy/prohibit_unused_variable_ignored_patterns.vim

@@ -0,0 +1,6 @@
+let s:unused = 'report me'
+let s:unused_but_ignored = 'not report me'
+let s:used = 'not report me'
+let s:used_but_ignored = 'not report me'
+echo s:used_but_ignored
+echo s:used

test/fixture/policy/prohibit_unused_variable_invalid.vim

@@ -11,3 +11,5 @@ function! g:FuncContext(param_var, param_func)
     let l:explicit_func_local_var = 106
     let implicit_func_local_var = 107
 endfunction
+
+call { i -> 0 }()

test/fixture/policy/prohibit_unused_variable_issue_274.vim

@@ -0,0 +1,7 @@
+function! s:foo() abort
+    let x = 'x'
+    let y = l:{x}
+    echo y
+endfunction
+
+call call('s:foo', [])

test/fixture/policy/prohibit_unused_variable_readme.vim

@@ -0,0 +1,3 @@
+" vint: next-line -ProhibitUnusedVariable
+let s:foobar = 'x'
+echo s:{'foo' . 'bar'}

test/fixture/policy/prohibit_unused_variable_valid.vim

@@ -17,9 +17,9 @@ function! s:ScriptLocalFunc()
 endfunction
 call s:ScriptLocalFunc()
 
-redir => l:redit_variable
+redir => l:redir_variable
 redir END
-echo l:redit_variable
+echo l:redir_variable
 
 function! g:FuncContext(param_var, param_func, param_member)
     " Parameter should be used
@@ -38,4 +38,6 @@ function! g:FuncContext(param_var, param_func, param_member)
 endfunction
 
 " redir variable should be used
-redir => l:redit_variable
+redir => l:redir_variable
+
+call { i, ... -> i }(0)

test/integration/vint/linting/policy/test_prohibit_implicit_scope_variable.py

@@ -30,9 +30,9 @@ def test_get_violation_if_found_when_file_is_invalid(self):
         expected_violations = [
             self.create_violation(2, 5),
             self.create_violation(4, 10),
-            self.create_violation(8, 5),
-            self.create_violation(12, 10),
-            self.create_violation(16, 5),
+            self.create_violation(9, 10),
+            self.create_violation(13, 5),
+            self.create_violation(16, 11),
         ]
 
         self.assertFoundViolationsEqual(PATH_INVALID_VIM_SCRIPT,

test/integration/vint/linting/policy/test_prohibit_unused_variable.py

@@ -1,44 +1,69 @@
 import unittest
+import enum
 from test.asserting.policy import PolicyAssertion, get_fixture_path
-
 from vint.linting.level import Level
 from vint.linting.policy.prohibit_unused_variable import ProhibitUnusedVariable
 
-PATH_VALID_VIM_SCRIPT = get_fixture_path('prohibit_unused_variable_valid.vim')
-PATH_INVALID_VIM_SCRIPT = get_fixture_path('prohibit_unused_variable_invalid.vim')
+class Fixtures(enum.Enum):
+    VALID_VIM_SCRIPT = get_fixture_path('prohibit_unused_variable_valid.vim')
+    INVALID_VIM_SCRIPT = get_fixture_path('prohibit_unused_variable_invalid.vim')
+    ISSUE_274 = get_fixture_path('prohibit_unused_variable_issue_274.vim')
+    IGNORED_PATTERNS = get_fixture_path('prohibit_unused_variable_ignored_patterns.vim')
+    README = get_fixture_path('prohibit_unused_variable_readme.vim')
 
 
 class TestProhibitUnusedVariable(PolicyAssertion, unittest.TestCase):
     def test_get_violation_if_found_when_file_is_valid(self):
-        self.assertFoundNoViolations(PATH_VALID_VIM_SCRIPT,
+        self.assertFoundNoViolations(Fixtures.VALID_VIM_SCRIPT.value,
                                      ProhibitUnusedVariable)
 
 
-    def create_violation(self, line, column):
+    def create_violation(self, line, column, path):
         return {
             'name': 'ProhibitUnusedVariable',
             'level': Level.WARNING,
             'position': {
                 'line': line,
                 'column': column,
-                'path': PATH_INVALID_VIM_SCRIPT
+                'path': path
             }
         }
 
 
     def test_get_violation_if_found_when_file_is_invalid(self):
         expected_violations = [
-            self.create_violation(2, 5),
-            self.create_violation(4, 11),
-            self.create_violation(7, 25),
-            self.create_violation(7, 36),
-            self.create_violation(11, 9),
-            self.create_violation(12, 9),
+            self.create_violation(2, 5, Fixtures.INVALID_VIM_SCRIPT.value),
+            self.create_violation(4, 11, Fixtures.INVALID_VIM_SCRIPT.value),
+            self.create_violation(7, 25, Fixtures.INVALID_VIM_SCRIPT.value),
+            self.create_violation(7, 36, Fixtures.INVALID_VIM_SCRIPT.value),
+            self.create_violation(11, 9, Fixtures.INVALID_VIM_SCRIPT.value),
+            self.create_violation(12, 9, Fixtures.INVALID_VIM_SCRIPT.value),
+            self.create_violation(15, 8, Fixtures.INVALID_VIM_SCRIPT.value),
         ]
 
-        self.assertFoundViolationsEqual(PATH_INVALID_VIM_SCRIPT,
+        self.assertFoundViolationsEqual(Fixtures.INVALID_VIM_SCRIPT.value,
                                         ProhibitUnusedVariable,
                                         expected_violations)
 
+    def test_issue_274(self):
+        self.assertFoundNoViolations(Fixtures.ISSUE_274.value, ProhibitUnusedVariable)
+
+
+    def test_ignored_patterns(self):
+        expected_violations = [
+            self.create_violation(1, 5, Fixtures.IGNORED_PATTERNS.value),
+        ]
+
+        self.assertFoundViolationsEqual(Fixtures.IGNORED_PATTERNS.value,
+                                        ProhibitUnusedVariable,
+                                        expected_violations,
+                                        policy_options={'ignored_patterns': ['_ignored$']})
+
+
+    def test_readme(self):
+        self.assertFoundNoViolations(Fixtures.README.value,
+                                     ProhibitUnusedVariable)
+
+
 if __name__ == '__main__':
     unittest.main()