nelfin / pylint-protobuf

A plugin for making Pylint aware of the fields of protobuf-generated classes
MIT License
29 stars 12 forks source link

Useless suppression false positives #55

Open hmc-cs-mdrissi opened 1 year ago

hmc-cs-mdrissi commented 1 year ago

Summary

Enabling pylint-protobuf can lead to useless suppression triggering for places where it disables an error that it checks when pylint wouldn't have flagged it anyway. I'm mainly seeing this for no-member. I think fix would be every place that does self._disable should also disable useless-suppression. It may be enough to update here to also disable useless-suppresion too.

edit: I can confirm patching _disable to be,

try:
        self.linter.disable("useless-suppression", scope=scope, line=line)
        self.linter.disable(msgid, scope=scope, line=line)
except ValueError:
  pass

is enough to remove false positive.

nelfin commented 1 year ago

Hi @hmc-cs-mdrissi, thanks for the report. Do you have enable=all in your MESSAGES CONTROL section of pylintrc or pyproject.toml? useless-suppression is an informational I-message which is not considered by pylint to be an error (it doesn't trigger an error status return code for example) and are more like meta-messages for the operation of pylint itself.

Since disables are only line scoped, I wouldn't want to prevent useless-suppression from being emitted for some other message, not related to the attempts here to prevent no-member from being later emitted by pylint.typechecks. As far as I'm aware there's no retroactive deletion of messages possible, pylint-protobuf does and would need to continue to run before the builtin typecheck checkers to flag what fields are present and pre-emptively tell the typechecker to ignore this line, if the typechecker gets it right, then this has turned out to be a useless supression but the code can't know that up front.

I'd suggest instead changing your message control settings to be enable=C,R,W,E,F if you'd like "all" messages to be shown.

hmc-cs-mdrissi commented 1 year ago

I'd like to treat useless-suppression as a failed lint because my current codebase has hundreds of useless-suppressions that makes it tricky to tell which lines actually need it. History of it is some libraries we use interact in buggy ways with pylint so many of them were needed in past but no longer after upgrading pylint/library version.

I don't have enable=all and instead have enable E,F + specific warnings/information messages. useless-suppression is one I want to use and current way pylint-protobuf disables no-member/similar errors means it produces a lot of useless-suppressions.

I think my fix has no impact on users who don't use useless-suppression. For users who do use useless-suppression the status quo is pylint-protobuf will make false positive for most protobuf accesses currently. Disabling useless-suppression there can be wrong sometimes but I'd trade current high protobuf false positives with some protobuf false negatives. useless-suppression will still work as normal for places where pylint-protobuf doesn't use _disable_message and it's not a protobuf type.

nelfin commented 10 months ago

Since pylint checker priority was removed, pylint-protobuf no longer tries to suppress "no-member" via PyLinter.disable since we can't guarantee that we run before the builtin typecheck linter. Since nothing is disabled, there should be no "useless-supression" messages. See if upgrading to 0.22 resolves this issue.