Pylint does not warn on assignment to constants

[omarandlorraine]

Bug description

Here is the contents of the file sglobals.py

#pylint: disable=missing-module-docstring
TheGreetingWithABadName = "Hello, World!"

Here is the output of pylint sglobals.py:

sglobals.py:2:0: C0103: Constant name "TheGreetingWithABadName" doesn't conform to UPPER_CASE naming style (invalid-name) 

Clearly the opinion is that TheGreetingWithABadName is a constant, so it must be a codesmell if we later assign to this. But pylint has nothing to say about the following file in the same directory:

#pylint: disable=missing-module-docstring
import sglobals

sglobals.TheGreetingWithABadName = "Hello, OtherWorld!"

Similarly, assigning to module-level functions and so is not caught by pylint.

Configuration

No response

Command used

pylint a.py

Pylint output

Your code has been rated at 10.00/10

Expected behavior

I would have expected a warning to the effect that assigning to module level variables is at least frowned upon

Pylint version

pylint 2.14.4
astroid 2.11.7
Python 3.9.2 (default, Feb 28 2021, 17:03:44)
[GCC 10.2.1 20210110]

OS / Environment

Debian 11.4

Additional dependencies

No response

[mbyrnepr2]

Perhaps the warning should suggest annotating the constant with typing.Final; that way Pylint would avoid checking something that mypy already can take care of.

[DanielNoord]

I think this is also caused by the seemingly different interpretations of "constant" in Python and other languages.

I can think of examples where module constants get re-assigned after certain operations are finished. The constant seems to refer to "something that holds value for a longer period of time" rather than a variable which is discarded sooner such as loop variables or variables within function scopes.

I agree that this is a semantic difference and perhaps not everybody agrees with this, but warning for this by default might not be the right solution.

I'm also thinking of the Const node in ast/astroid which has the same issue with being a constant that can be reassigned.

[Pierre-Sassoulas]

Isn't that an issue with python itself ? There's no real constant in python. even typing.Final do not prevent reassignment. The current solution to detect immutability seems optimal considering the current state of the language ?

[DanielNoord]

Yeah I don't think pylint can do anything else without suggesting worse code/weird patterns. Using Final is also not the optimal solution in some cases and it assumes the user also uses mypy.

[mbyrnepr2]

Isn't that an issue with python itself ? There's no real constant in python. even typing.Final do not prevent reassignment. The current solution to detect immutability seems optimal considering the current state of the language ?

I think this touches on the confusion because Pylint is naming it a constant but it doesn't behave that way in Python. Perhaps the message can be updated to mention the preferred uppercase but avoid using the word constant?

[DanielNoord]

I'm on mobile, but in a recent discussion we found that pep8 actually uses the term constant, which is where this rule comes from.

[Pierre-Sassoulas]

The discussion become highly similar to https://github.com/PyCQA/pylint/issues/3585. It's hard/impossible to know if something is a constant or not for a module level variable. Right now we rely on mutability which is probably as good as it get.

[mbyrnepr2]

I agree with you @DanielNoord & @Pierre-Sassoulas; In my opinion this issue could be closed since the idea of constant for these variables is a convention in Python which doesn't stop you from re-assigning.

[omarandlorraine]

since the idea of constant for these variables is a convention in Python which doesn't stop you from re-assigning.

Huh? Isn't a big point of a linter to warn you when you're breaking convention?

[omarandlorraine]

The discussion become highly similar to #3585. It's hard/impossible to know if something is a constant or not for a module level variable. Right now we rely on mutability which is probably as good as it get.

PEP8 says this

Constants are usually defined on a module level and written in all capital letters with underscores separating words. Examples include MAX_OVERFLOW and TOTAL.

Also PEP8 says they should be after imports and before anything else.

I presume this is why C0103 warns that my global does not honor the naming convention.

[omarandlorraine]

I've just checked, there is no warning for module-level variables in SCREAMING_SNAKE_CASE coming after classes or module-level functions. Is this a problem?

[mbyrnepr2]

We get the invalid-name message because the variable is immutable and it is not uppercase. This goes back to Pierre's point about mutability - it is how Pylint determines what is or is not a constant. If your variable is a dictionary, the message is not emitted regardless of case.

Having said that, one case where this logic breaks is global variables. You expect to re-assign the module-level value somewhere from within a function, and that would be a string, as an example, which Pylint considers a constant.

I hope I'm not missing any point that was already made in the good discussion above, but since the issue here seems to be mainly around the complexity of identifying if a module-level variable is a constant or not, would it be an idea to tweak the logic so that the warning is emitted simply when Pylint sees a variable name which contains some upper & some lowercase characters? In other words, it looks like a user is trying to define a constant. Otherwise, snake_case should not emit the warning since the intention there is never clear if it will be re-assigned or not.

[Pierre-Sassoulas]

I checked and it's not really immutable/mutable as for example in the following code aad. aad and aaf are mutable and are not raising.

aaa = 42  # [invalid-name]
aab = tuple("42")
aac = "42"  # [invalid-name]
aad = [42]
aae = {42}
aaf = {"a": 42}

Here's the related code : https://github.com/PyCQA/pylint/blob/main/pylint/checkers/base/name_checker/checker.py#L409

There's already something regarding "Final" as far as I understand: https://github.com/PyCQA/pylint/blob/cfc393a8dff9ec09bd2fcb25857e772ae04a4991/pylint/checkers/base/name_checker/checker.py#L445

[mbyrnepr2]

I meant immutable. Corrected my comment! So, it breaks for global string, for example, which is re-assigned.

[mbyrnepr2]

@Pierre-Sassoulas thank you regarding your most recent comment with the link to the Final logic! I think this also highlights a separate issue - If the assignment is annotated with something other than Final, the message is not emitted. E.g. for my_var: int = 1 no message about using upper case is emitted.

[mbyrnepr2]

Do you think removing these lines would be acceptable & fix what we've discussed Pierre (sorry for tagging so much :D)?

When you look at the annassign case, I think this is logical because it only regards a constant as something which is annotated with Final & then only emits the warning if it doesn't match the 'const' regex (all uppercase). I think the logic of only triggering the warning when something of the form under_scored: Final = "hello" seems sound enough.

[Pierre-Sassoulas]

No problem @mbyrnepr2 . The more we look into it, the more it seems there's a big issue with this check consistency and we added wart after wart on top of the existing code without exhaustive tests. I don't have the time for it right now, but I feel we should list what is happening at the moment (when we have typing / no typing, final / not final, mutable / immutable, in another scope like for loop or not...) from all the issues opened regarding this and then decide what we want from the check. What I started in https://github.com/PyCQA/pylint/issues/7232#issuecomment-1196657334 already shows some inconsistencies regarding mutability.