Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Useless suppression false positives #55

Open
hmc-cs-mdrissi opened this issue May 9, 2023 · 3 comments · May be fixed by #56
Open

Useless suppression false positives #55

hmc-cs-mdrissi opened this issue May 9, 2023 · 3 comments · May be fixed by #56

Comments

@hmc-cs-mdrissi
Copy link

hmc-cs-mdrissi commented May 9, 2023

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.

@hmc-cs-mdrissi hmc-cs-mdrissi linked a pull request May 9, 2023 that will close this issue
@nelfin
Copy link
Owner

nelfin commented May 10, 2023

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
Copy link
Author

hmc-cs-mdrissi commented May 10, 2023

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
Copy link
Owner

nelfin commented Dec 10, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants