Prevent useless-suppression on disables for stdlib deprecation checker

[jacobtylerwalls]

Type of Changes

	Type
✓	✨ New feature

Description

Before
The deprecated names checker ignored the py_version setting, raising warnings (or not) depending on the python interpreter version.
useless-suppression is raised on disables of deprecated-module and sim. on lower versions where the module is not yet deprecated. This can make it challenging to get a project to pass pylint on multiple versions with a disable for a deprecated module.

Now
Deprecated name warnings are emitted according to py_version (which falls back to the interpreter version).

Discussed at #5874 (comment).
useless-suppression is treated as incompatible with the stdlib deprecation checks--it is not emitted.

Tangent
As a follow up, we could consider adding --py-version=3.8 to the pre-commit config to match CI? After 2.13, I suppose it should be --py-version=3.7.

[coveralls]

Pull Request Test Coverage Report for Build 1966122943

• 12 of 12 (100.0%) changed or added relevant lines in 3 files are covered.
• 38 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.01%) to 94.018%

---

Files with Coverage Reduction	New Missed Lines	%
pylint/extensions/typing.py	4	97.08%
pylint/config/find_default_config_files.py	12	64.41%
pylint/config/option_manager_mixin.py	22	88.89%

Totals
Change from base Build 1952197418:	0.01%
Covered Lines:	14995
Relevant Lines:	15949

---

💛 - Coveralls

[Pierre-Sassoulas]

👍 Nice job, this is definitely a checker where the py-version is very useful.

[cdce8p]

Looks good. I left a few comments with some more detailed types.

[DanielNoord]

Bit late to the party here, but are we sure this should consider py-version? I was thinking the same thing earlier, but if we do we lose the "timely warning" effect of deprecated-module. Taking distutils as an example, we would only start to warn when py-version==3.10. What effect does depracted-module then have compared to the normal DeprectationWarnings that get emitted?

[jacobtylerwalls]

What effect does depracted-module then have compared to the normal DeprectationWarnings that get emitted?

My reply would be: it's better to divorce these things from your python interpreter. If you haven't (or won't be) upgrading your python interpreter from 3.8 to 3.10, then you can run with --py-version=3.10 to get an early warning.

The docs should probably be updated to suggest running --py-version for highest and lowest supported versions, not just lowest.

[jacobtylerwalls]

What effect does depracted-module then have compared to the normal DeprectationWarnings that get emitted?

Well, if you have poor test coverage and then never trigger a warning during tests, pylint comes to the rescue! 😉

[Pierre-Sassoulas]

Wow, I think Daniel is right. We probably want to do the opposite change and warn for all versions regardless of the interpreter used. (Edit: We followed the same logic than for the typing checker but in the case of typing you can't use the new typing on old interpreter, but you can always use the new function instead of the deprecated one).

[DanielNoord]

My reply would be: it's better to divorce these things from your python interpreter. If you haven't (or won't be) upgrading your python interpreter from 3.8 to 3.10, then you can run with --py-version=3.10 to get an early warning.

Agreed, but at the same time it kind of defeats the purpose of this message? A completely (and perhaps equally flawed approach) could be to remove deprecated-... from the useless-suppression warning. I believe that's the only thing that is really troublesome about this, right?

The docs should probably be updated to suggest running --py-version for highest and lowest supported versions, not just lowest.

That runs a bit counter to what that setting is supposed to represent (both in pylint and in other tools that are slowly starting to support the setting).

[jacobtylerwalls]

That runs a bit counter to what that setting is supposed to represent (both in pylint and in other tools that are slowly starting to support the setting).

Got it, fair enough. But I wonder about other things besides the deprecated checker. What about the consider-using-alias that only operates on 3.7 and 3.8. If there's a disable for that in a codebase being linted against py 3.7, won't a python 3.10 interpreter flag that as useless-suppression? And now back to the same problem with pre-commit blocking people with different interpreters. (EDIT: this isn't the worst thing in the world; if you're going to work on a project, use the python version they're using.)

Agreed, but at the same time it kind of defeats the purpose of this message? A completely (and perhaps equally flawed approach) could be to remove deprecated-... from the useless-suppression warning. I believe that's the only thing that is really troublesome about this, right?

If we warn on all versions for, say, distutils deprecation, projects that might not support py 3.10 for another two years or maybe never (more likely, with say, 4.0 than 3.10...), then have to add disables everywhere they use the particular modules deprecated in 3.10. They can't do a global disable without losing warnings they want on 3.8 and below (e.g.). So they're wading into picking and choosing all the deprecated names.

---

(I'm open to whatever; interested in this for its own sake, not some passionate commitment to any direction.)

[Pierre-Sassoulas]

I think it also makes sense to follow the logic used elsewhere (in the typing checker for example).

A completely (and perhaps equally flawed approach) could be to remove deprecated-... from the useless-suppression warning.

I like this that way we could both have minimum false negative and false positive. Maybe more generally if a message uses the py-version at all we disable useless-suppression for it ? I think it make sense, the potential for false positive is high for multi-interpreter project. (The other false positives for useless-suppression I can think off are various versions of the same lib --numpy 1.19 vs 1.20, for example-- but we do not have this level of support for lib in astroid brain.)

I don't have a strong opinion about this. I think @cdce8p, as the creator of py-version, will though 😄

[cdce8p]

What about the consider-using-alias that only operates on 3.7 and 3.8. If there's a disable for that in a codebase being linted against py 3.7, won't a python 3.10 interpreter flag that as useless-suppression?

The reason why I added py-version in the first place was to deal with these exact issues. Personally, I'm using Python 3.10. If I use that for pylint, I would end up with suggestions for typing which don't work in all supported version.
For your example, if the project sets py-version = 3.7, anyone linting it would get the same result. Regardless if they are using 3.7 or 3.10. Thus useless-suppression isn't an issue.

Some points to take away. py-version should always indicate the oldest supported version and it's used to emit suggestions, not warnings. At least I think of it that way.

--
I'm not completely sure anymore it can be applied that way to the deprecation checker. Both arguments are valid here, use py-version - errors should only be emitted when all supported versions would emit one - and use sys.version_info at least that way maintainers know about upcoming deprecations earlier.

It could even be that exact conflict why we haven't changed it already.

So how do we resolve this? Tbh I don't have a good answer here. One moment I like py-version and the next sys.version_info. Maybe the later is slightly better, considering that projects can actively choose to resolve deprecation warnings earlier?

--

Agreed, but at the same time it kind of defeats the purpose of this message? A completely (and perhaps equally flawed approach) could be to remove deprecated-... from the useless-suppression warning. I believe that's the only thing that is really troublesome about this, right?

Not sure. Wouldn't that defeat the purpose of the useless-suppression check?
IMO useless-suppression just shouldn't be enabled by default. Just using it from time to time to cleanup messages is enough. That would resolve the whole issue. I believe that's also what I argued for when it was added to pylintrc, but I might be wrong.

[jacobtylerwalls]

Thanks, that's helpful. I think now --py-version is the wrong way to go, and this PR shouldn't tie it to the deprecated checker.

I looked at the original deprecated checker, no discussion about multiple-interpreter projects.

Warn for all versions and don't emit useless-suppression for the deprecated checker are not incompatible with each other, but nobody's asking for the former ATM, so I suggest just doing the smaller change to deal with the little bump I ran into. :-)

[DanielNoord]

Not sure. Wouldn't that defeat the purpose of the useless-suppression check? IMO useless-suppression just shouldn't be enabled by default. Just using it from time to time to cleanup messages is enough. That would resolve the whole issue. I believe that's also what I argued for when it was added to pylintrc, but I might be wrong.

I think that just comes down to personal preference.

Fact remains that a project which uses distutils and runs its CI both on 3.9 and 3.10 will be unable to pass CI when both useless-suppression and deprecated-module are turned on. Having two messages be incompatible with each other seems like something we should try and avoid (and fix).

Another (far-fetched) idea would be to change deprecated-module to the Information category? That way it's easier to let CIs pass even though the message is emitted?

[Pierre-Sassoulas]

Some points to take away. py-version should always indicate the oldest supported version and it's used to emit suggestions, not warnings

Thank you for the reminder ! Taking that into account if py-version is 3.6 it means you can be supporting 3.10 (what we currently do in pylint). So if something deprecated in 3.7 is removed in 3.10 you can have 3.10 code not working because the deprecated function was removed and pylint not raising a warning (yet) because you still support 3.6.

By "emit suggestions, not warnings" are you saying we should decrease the level of the message like @DanielNoord suggested ? I think information message are deactivated by default so if we decrease to I most user won't see it at all.

[cdce8p]

By "emit suggestions, not warnings" are you saying we should decrease the level of the message like @DanielNoord suggested ? I think information message are deactivated by default so if we decrease to I most user won't see it at all.

No, suggestions is just the mental model I use here. As you said, information messages likely wouldn't be seen at all. So those don't help.

Warn for all versions and don't emit useless-suppression for the deprecated checker are not incompatible with each other, but nobody's asking for the former ATM, so I suggest just doing the smaller change to deal with the little bump I ran into. :-)

Sounds good to me. In your changes, I saw that we already exclude cyclic-import messages. So adding a few more wouldn't be that big of a change. If @Pierre-Sassoulas and @DanielNoord agree, I would suggest to move forward with that.

[Pierre-Sassoulas]

I'm wondering how hard it would be to still raise useless-suppression if the function/class/module/decorator... is not deprecated in any python version. I don't think this checker is very performance intensive, maybe relaunching with the highest python version (3.10 right now) is possible ?

[cdce8p]

Though about something similar, but in the context of a bare disable once the deprecation has been removed.

I don't think this checker is very performance intensive, maybe relaunching with the highest python version (3.10 right now) is possible ?

Not sure that is practical. It would almost certainly change major parts of the application flow.
Maybe it's simpler to add a new config option to show all useless-suppression messages? We should mention however, that it should only be used temporarily and not added to pylintrc.

[DanielNoord]

@cdce8p I like your last suggestion!

[jacobtylerwalls]

Thanks for the reviews -- they helped point this in the right direction and get it polished.

Maybe it's simpler to add a new config option to show all useless-suppression messages? We should mention however, that it should only be used temporarily and not added to pylintrc.

Shall we defer that to another issue?

[cdce8p]

Maybe it's simpler to add a new config option to show all useless-suppression messages? We should mention however, that it should only be used temporarily and not added to pylintrc.

Shall we defer that to another issue?

👍🏻

[cdce8p]

Looks good to me. I'll leave the final call for @DanielNoord and @Pierre-Sassoulas.
Thanks @jacobtylerwalls 🐬

[DanielNoord]

Thanks @jacobtylerwalls!

Could you make that follow-up issue about finding an elegant way to force useless-suppression to show all warnings?