Make invalid-name distinguish between module constants and module variables

[bersbersbers]

Steps to reproduce

1. pylint bug.py

"""False-positive invalid-name: `variable` is not constant, should not be UPPER_CASE."""
for i in (0, 1, 2):
    variable = i

Current behavior

************* Module cia.bug
bug.py:3:4: C0103: Constant name "variable" doesn't conform to UPPER_CASE naming style (invalid-name)

------------------------------------------------------------------
Your code has been rated at 5.00/10 (previous run: 5.00/10, +0.00)

Expected behavior

Perfect score.

pylint --version output

pylint 2.5.0
astroid 2.4.0
Python 3.8.2 (default, Feb 26 2020, 09:52:54) 
[GCC 7.5.0]

Problem does not appear with range(3). Problem does not appear with pylint<2.5.

[Project-Magenta]

variable is only assigned once, why is it not constant?

[bersbersbers]

@Project-Magenta variable is assigned not once, but three times. Consider this:

for i in (0, 1, 2):
    variable = i
    print(variable)

which prints

>>> for i in (0, 1, 2):
...     variable = i
...     print(variable)
...
0
1
2

[Project-Magenta]

oh ok i see

[PCManticore]

The variable is assigned once but as long as it is a Const in astroid's parlance (that is string, numbers, floats, bools), pylint asks you to use the UPPER_CASE style when defining these variables at module level. This is expected behaviour regardless if you assign the variable in a loop or not. I don't quite though why you have to set this variable at module level multiple times.

[bersbersbers]

Interesting. Coming from C++, I thought I knew what a "constant" is - will have to adapt apparently.

Now, the fact that this code does not give invalid-name is a bug, then?

"""False-negative invalid-name: should behave the same as (0, 1, 2)."""
for i in range(3):
    variable = i

Or this one?

"""False-negative invalid-name: should behave the same as (0, 1, 2)."""
for i in tuple(range(3)):
    variable = i

Or this one?

"""False-negative invalid-name: should behave the same as [0, 1, 2]."""
for i in list(range(3)):
    variable = i

Something is inconsistent here, I don't think this is ready to be closed.

[PCManticore]

Thanks for the additional examples. Something is definitely inconsistent with this check. In the latter examples you gave, pylint cannot fully infer that the variable is a constant or not, while it can do so in the original example. I believe in this case we should simply emit the invalid-name as long as the variable is assigned only once at module level (thus indicating that it's an actual constant)

[Project-Magenta]

but it is assigned 3 times because python variables are module/function scoped
???

[bersbersbers]

Yes, something is off with this check. While I get a warning in the original code,

for i in (0, 1, 2):
    variable = i

I do not get any in this case:

for i in (0, 1, 2):
    variable, another = i, i

[bodograumann]

This seems related:

x = 0
for i in range(7):
    x+=1

Here pylint also says x is a constant, when it is obviously mutated.

[bersbersbers]

Whoever works on this, please consider adapting the error message if required. Currently, it says

Constant name "variable" doesn't conform to UPPER_CASE naming style

variable is not a constant as I understand it. In particular, the "name" variable is not constant. But digesting #3585 (comment) made me realize that what apparently counts is not (only) if variable is itself constant (that is, variable has the same value at all times), but (also) if variable has a value that is constant - in the sense of immutable. I hope this interpretation is correct - if it is, it may help to clarify the error message.

[jamesquilty]

I had a very simple script, similar to the above, which pylint failed with the same apparently false-positive and which led me to this bug and #3466. The discussion in #3466 was revealing: pylint raises C0103 when a variable is declared at module level and does not conform to the naming style. The coding style this is expressing is "a variable declared at module level should be a constant".

Defining a function and declaring the variables inside the function avoided pylint raising C0103. As I was running this script from the command line I used:

def main():
    """Do some stuff"""

    for i in (0, 1, 2):
        variable = i


if __name__ == "__main__":
    main()

While doing it this way obviously incurs some overhead, it is compliant with pylint's style rules.

[DanielNoord]

@Pierre-Sassoulas or @PCManticore do we actually consider this a false positive and should pylint be made to recognise non-constant module-level variables and ignore them for invalid-name?
Or do we want to enforce "a variable declared at module level should be a constant" and make pylint complain about some of the additional examples discussed here? The latter would also include an update to the warning mentioning the fact that the variable is declared at module level?

[Pierre-Sassoulas]

There's actually an example where we don't want this to be a constant : LOGGER = logging.getLogger(__name__) vs logger = logging.getLogger(__name__) those both valid variable name at module level and depending on project the convention used is not the same. I can't find the issue (after 15mn searching) but there was something with not having any check on module level constant (you could use Logger or Logger_Camel for example, which obviously is not desirable).

[DanielNoord]

So, we want to start recognising non-constant module-level variables? Guess we can stick the control-flow label on this one as well then 😅

[Pierre-Sassoulas]

No, I think we need to keep it simple but also enforce one convention or the other but still have a rule (ie LOGGER, or logger are okay but not LoGgeR). Maybe this was already fixed and that's why I'm not finding the corresponding issue.

[DanielNoord]

Okay, so what's needed to close this issue? A better way to set the pattern for module level variables?

[Pierre-Sassoulas]

I think a functional test like this would do it:

for i in (0, 1, 2):
    variable = i # okay
    
for i in (0, 1, 2):
    VARIABLE= i # also okay
    
for i in (0, 1, 2):
    vAriAbLE = i #[invalid-name]

Maybe an option for configuring what module level variable are acceptable or not ? (UPPERCASE, snake_case, both)

[wmorgansoftware]

I happened to also notice this using pylint 2.14.4 using the following code example:

sym_perc_held = 1.0 # Problem line that gets flagged
if sym_exp_val > 0.0:
  sym_perc_held = sym_curr_val / sym_exp_val

In this case, I'm trying to do a safety check against dividing by zero. The variables sym_curr_val and sym_exp_val are floating-point numbers that vary based on calculations made in a loop, and don't get flagged.

[Pierre-Sassoulas]

This require a prior refactor to accept multiple name style for a node type in self._name_regexps (a change to the regex itself is possible but feel 'dirty' as you don't reuse existing regex).

[DanielNoord]

I think a functional test like this would do it:
...

Revisiting that example: I'm not sure I agree anymore. #3585 (comment) seems to indicate what pylint is currently doing and I think that is both consistent and predictable. Every constant defined in the module scope should use upper case, see also PEP8 which recommends this.

The use of logger or sym_perc_held is "incorrect" as those are constants at the module level. By using upper case you also signal that you understand that these constants could be imported by other modules from this module since they are in the module scope.
If you don't want this you should probably enclose them in a function scope like the example in that comment does.

This argument applies to all examples in this issue, even those that get assigned multiple times: they are importable by other modules and can therefore be considered "module constants". Since the current status quo is actually predictable I would vote to keep it.

The only thing I would change is:

Constant name "variable" doesn't conform to UPPER_CASE naming style (invalid-name)

To:

Constant or module variable name "variable" doesn't conform to UPPER_CASE naming style (invalid-name)

[wmorgansoftware]

The biggest issue with this is, although sym_perc_held may start out as a specific number, it does not remain that constant number. It does change, and not exclusively by the divide by zero safety check, either. To provide more context, it exists within a for loop iterating through symbols (hence the "sym" in the name), and every symbol gives a different number.

Since the entirety of the module is within an if __name__ == '__main__' conditional (simple engine script), I can try sticking it into a function (when I get to it) and see if that's a decent workaround...

[wmorgansoftware]

Inside a function looks like it works; that'll be sufficient for what I'm seeking.

[macdjord]

@DanielNoord You are correct that PEP8 recommends constants be UPPER_CASE. However, PEP8 also acknowledges that module level variable are a thing, and recommends formatting them the same way as function variables, i.e. lower_case.

Pylint is entirely right to enforce that constants be uppercase, but it is wrong in assuming that every module-level name is a constants and not a variable.

The best solution would be to detect whether a given name is a constant or a variable and enforce the appropriate convention. However, this would be difficult; for example, I have a case where a module-level variable only has a single module-level assignment (setting it to None at definition), but gets initialized and modified by functions using global.

The second-best solution would be to acknowledge that any given module-level name might be a constant or a variable, and allow by default either naming convention (while still enforcing that it must be one of the two conventions; mixed-case is still out).

[jacobtylerwalls]

👋 @DanielNoord

I find the reply at #3585 (comment) convincing, and I think fits well with our principle when in doubt, be less opinionated. WDYT -- can we mark this as ready for an implementation? In other words, accept a PR that removes false positives for invalid-name on lower-cased module "constants" that are later reassigned?

[DanielNoord]

Considering the high amount of 👍 I guess this is what the people want 😄

I'm fine with changing this but it should probably happen in a 4.x? I'm fine with releasing such a version though :)

[jacobtylerwalls]

That's fair, it's a pretty clear breakage from the current documentation.

Also, that gives us time to try to do it the "right way":

The best solution would be to detect whether a given name is a constant or a variable and enforce the appropriate convention. However, this would be difficult; for example, I have a case where a module-level variable only has a single module-level assignment (setting it to None at definition), but gets initialized and modified by functions using global.

Honestly the global edge case doesn't bother me. Barring that, this doesn't sound so hard.

[jacobtylerwalls]

Two kinds of implementations could be accepted here, I think:

• 1. detect whether a given name is ever reassigned (to find out whether to check it against the module constant or variable regexes)
• 2. just check module names against both regexes and allow a pass against either one

The second approach is simpler but probably involves a small refactor to allow "as-if" checking against regexes without immediately emitting a message.

Another consideration is whether we need a new regex for module-level variables versus reusing the function variable regex (and updating the doc).