vint poorly handle functions and their scopes

[Coacher]

Hello.

Consider the following VimL code:

call vundle#begin('~/.vim/plugins/')
Plugin 'todesking/vint-syntastic'
Plugin 'scrooloose/syntastic'
call vundle#end()

vint complains:

Make the scope explicit like `g:vundle#begin` (see Anti-pattern of vimrc (Scope of variable))
Make the scope explicit like `g:vundle#end` (see Anti-pattern of vimrc (Scope of variable))

The comment about Scope of variable is out of place here. Anti-pattern of Vim also says nothing about function scopes.

Now consider the following code that describes a function that should be available globally:

function! mysuperplugin#do()
    echom 'Hello world!'
endfunction

Again vint complains:

Make the scope explicit like `g:mysuperplugin#do` (see Anti-pattern of vimrc (Scope of variable))

Logically this function should have g: scope, but Vim actually forbids it:

The name must be made of alphanumeric characters and '_',
and must start with a capital or "s:" (see above).
Note that using "b:" or "g:" is not allowed.

See :h E884 for more info.

While the previous behavior is still somewhat tolerable, the following code:

function! g:mysuperplugin#do()
    echom 'Hello world!'
endfunction

is valid for vint, while for Vim it is clearly not. This is really bad.

Please fix.

[Coacher]

scope_detector.py says:

# Functions can have the scope visibility only explicit global or
# implicit global or explicit script local. So a function have implicit
# scope visibility is always global function.

This is true. But vint should ignore function definitions when trying to fix function scopes unless it is inside ScopeVisibility.SCRIPT_LOCAL.

[Coacher]

But vint should ignore function definitions when trying to fix function scopes unless it is inside ScopeVisibility.SCRIPT_LOCAL.

One way to achieve this is via the following patch:

--- a/vint/ast/plugin/scope_plugin/scope_detector.py
+++ b/vint/ast/plugin/scope_plugin/scope_detector.py
@@ -286,7 +286,7 @@ def _get_explicit_scope_visibility(id_node):
     scope_prefix = id_node['value'][0:2]

     if is_function_identifier(id_node) and is_declarative_identifier(id_node):
-        return FunctionDeclarationIdentifierScopePrefixToScopeVisibility.get(scope_prefix)
+        return FunctionDeclarationIdentifierScopePrefixToScopeVisibility.get(scope_prefix, not None)
     else:
         return VariableIdentifierScopePrefixToScopeVisibility.get(scope_prefix)

If function declaration has scope prefix, then leave it untouched, otherwise it is okay for a function declaration to not have a scope at all, so return not None as this is what triggers is_explicit in get_explicity_of_scope_visibility().

[Coacher]

Hmm, Vim actually allows the g: scope for functions, though its own documentation states otherwise. Looks like g: and s: are the only scopes that a function definition can have.

[Coacher]

Now I see that vint tries to do best with function scopes. You've done a great job! Many thanks.

Though I have one more question: Autoload functions cannot be local to script in order to work properly. Vint requires the explicit g: scope on them, but they are easily distinguishable in the code. Maybe it is not absolutely necessary to enforce an explicit scope on autoload functions?

Please share your thoughts on this one if you have time.

See also PR #129

[Kuniwak]

Thanks for reporting!

I think that an option to ignore autoload functions for the policy Scope of variable is the better way.

1. The current policy of Scope of variable is useful for beginners because they can understand auload functions are global.
2. However, power users get annoyed, and they can ignore this warning by the option.

What do you think this option?

[Coacher]

I think such option would be even better. This way backwards compatibility is also preserved so users who prefer their autoload functions to be explicitly scoped will get the warnings.

[Coacher]

I am not sure how to implement this. The straightforward way is to add a boolean parameter to get_explicity_of_scope_visibility and use it in conjunction with is_autoload_identifier check. But this changes the signature of get_explicity_of_scope_visibility. Maybe you can see another way?

[Kuniwak]

In my design, a Policy class has a responsibility of suppressing their warnings. But now, ProhibitImplicitScopeVariable cannot know whether an identifier is autoloaded. So, we should export is_autoload_identifier to ProhibitImplicitScopeVariable.

Step1: Export IdentifierClassifier.is_autoload_identifier(node) as ScopePlugin#is_autoload_identifier(node).

Step2: In ProhibitImplicitScopeVariable#is_valid(identifier, lint_context), we can check whether the identifier is an autoload identifier via lint_context['plugins']['scope'].

Step3: Export Linter#_config to Policy via lint_context.

Step4: We can check the desired option "suppress_autoload" in ProhibitImplicitScopeVariable#is_valid.

[Coacher]

Thanks for the detailed info.

I've updated my PR. Tested and works as intended on my machine. Please review.

Update: it also passes tests now.

[Coacher]

Also I'd personally prefer to store a policy option as an instance attribute and update it if necessary from PolicySet#update_by_config. Again I am not sure how it would fit into your design.

[Kuniwak]

As you say, we should define the comment config syntax for each policies.

But I feel this feature is not so important. Because, usually, a comment config is used to disable a policy.

I want to pend the issue until the such request is arrived.

[Coacher]

As you say, we should define the comment config syntax for each policies.

I am sorry, but I didn't understand what you've meant here. Could you rephrase/expand please?

[Kuniwak]

Oh... sorry it is my operation mistake.

[Kuniwak]

I meant that we should define the "comment syntax for police options".

For example:

" vint: ProhibitImplicitScopeVariable.suppress_autoload=true

" vint: ProhibitImplicitScopeVariable={supresd_autoload: true}

But these examples seems not good to me. And I have no good idea.

[Coacher]

Hmm, I still don't quite understand what you've meant here since vint uses YAML configuration and your example is something different. Are we talking about different things?

If we are speaking about YAML-based configuration, then the standard YAML notation will suffice I think.

For example:

policies:
  ProhibitImplicitScopeVariable:
    suppress_autoload: no

If you check my update_by_config branch it does things this way.

[vito-c]

@Kuniwak The comment syntax doesn't work I tried both

" vint: ProhibitImplicitScopeVariable.suppress_autoload=true

" vint: ProhibitImplicitScopeVariable={supresd_autoload: true}

but was only able to get this to work via the vintrc yaml file

[Kuniwak]

@vito-c Sorry, this feature is not implemented yet.