Enforcing justification for disabled messages

[jlanik]

Current problem

Currently we can suppress warnings in code with comments like this:
# pylint:disable=warning-type

What happens often in codebases I work on is that people add this without specifying any justification, hence I'd like to have a way to enforce it.

Desired solution

Add an option that would switch on the "justification enforcing". With this option, every suppression would have to be followed with another line:
# pylint:justification="Why we really need this"

So the final format would be as follows:
# pylint:disable=warning-type
# pylint:justification="Why we really need this"

If the option is not set, the behavior would not change.

Additional context

I'd like to implement it myself, however It would be good to get some form of green light first.

[Pierre-Sassoulas]

If we do this it should be # pylint:justify="Why we really need this" to keep the same semantic than disable and enable. There would be two things to do :

• Permits the parsing so pylint does not break
• Add an extension to warn when the justification is not there

I'm conflicted about it though, pylint might already feel bothersome and have too much false positives or enabled check by default (maybe I'm biased because I'm handling ALL the complaints as a maintainer). If disabling a pylint message forces even more work than the disable itself, then pylint acceptability could drop by a lot. How would your team feel about having to justify each disable (even false positive) @jlanik ?

[jlanik]

@Pierre-Sassoulas The key is to have this as an opt-in feature, so only people, who explicitly enable it will be forced to write justifications.

And for false positives they will need to write justification="False positive".
I think it's actually very helpful to see that some suppression is there because of a false positive and not because the developer decided to ignore a valid warning for other reason.

Honestly, I don't see that many false positives, what I see is 400 lines long functions starting with \#pylint:disable:too-many-statements,too-many-locals

My team is fine with it, on c++ side of the project they need to write a full document for every suppression and have it approved by voting, so the requirement to write an extra line for every suppression is nothing that would shock them.

[jlanik]

I'm thinking that we could implement it not with an extra option, but with an extra rule that would just check that every suppression line is followed by justification line and this rule would be disabled by default.

[DanielNoord]

I would like this as an opt-in option as well. It might not turn it on for every project, but I notice that often I do not know why I disabled something in a project I revisit a couple of months later.

[Pierre-Sassoulas]

extra rule that would just check that every suppression line is followed by justification line

Yes that's what I was referring to by an extension (ie: an optional rule that need to be enabled with load-plugin). See an example of opinionated extension here : /~https://github.com/PyCQA/pylint/blob/main/pylint/extensions/while_used.py

Do you think we should allow justify on the same line ?
# pylint: disable=missing-docstring,too-many-statements justify=deadlines!

Or multiple line ? Should the justify always be on the next line ? Should we have justify-next like we have disable-next ?

[DanielNoord]

I would propose the following:

def func(): # pylint: disable=message_one, message_two justify=Reasoning
    pass

# pylint: justify-next=Reasoning
def func(): # pylint: disable=message_one, message_two
    pass

# pylint: disable-next=message_one, message_two justify=Reasoning
def func():
    pass

# pylint: justify-next=Reasoning
# pylint: disable-next=message_one, message_two
def func():
    pass

The justify targets a disable on the same line, or if used as justify-next targets a disable on the next line.

If we want to go really advanced we could also do:

def func():
    # pylint: justify-scope=Reasoning
    var = 1 # pylint: disable=message_one, message_two
    var = 2
    var = 3 # pylint: disable=message_one, message_two

Using justify-scope would then justify all disables within a scope. This might be too hard to implement though.

Note that line-too-long currently does not handle disable too well, see #4802
I think this would also be problematic with "justify's", but it would be nice if they were implemented in a somewhat similar way so that eventually we can fix them both with #4802 instead of requiring an additional fix just for "justify's".

[Pierre-Sassoulas]

I think we should design this with a longer justification in mind. If reasoning takes one line everything will look nice on paper but what about:

# pylint: justify-next=Without going into too much detail the deadline is approaching and ClientA is really important for the financial well being of the company, and also I have a kid, a husband and a cat to feed, you know. To synthesize the current data succintely: this is acceptable technical debt that the next guy will handle.
# pylint: disable-next=message_one, message_two
def func():
    pass

What do you think ?

[DanielNoord]

# pylint: justify-next=Without going into too much detail the deadline is approaching and
# pylint: justify-next=ClientA is really important for the financial well being of the company,
# pylint: justify-next=and also I have a kid, a husband and a cat to feed, you know. 
# pylint: justify-next=To synthesize the current data succintely: this is acceptable technical debt
# pylint: justify-next=that the next guy will handle.
# pylint: disable-next=message_one, message_two
def func():
    pass

?

It looks awful but would allow us to group similar justify's together and consider them as one.

# pylint: justify-next=Without going into too much detail the deadline is approaching and
# ClientA is really important for the financial well being of the company,
# and also I have a kid, a husband and a cat to feed, you know. 
# To synthesize the current data succintely: this is acceptable technical debt
# that the next guy will handle.
# pylint: disable-next=message_one, message_two
def func():
    pass

I prefer this second example but worry about the difficulty of implementing this since you can longer do check_if_justify(node.lineno - 1)...

[Pierre-Sassoulas]

What about:

# pylint: justify=Without going into too much detail the deadline is approaching and
# pylint: justify=ClientA is really important for the financial well being of the company,
# pylint: justify=and also I have a kid, a husband and a cat to feed, you know. 
# pylint: justify=To synthesize the current data succinctly: this is acceptable technical debt
# pylint: justify=that the next guy will handle.
# pylint: disable-next=message_one, message_two
def func():
    pass

If the justify next can span multiple line the 'next' is less meaningful

Maybe:

# pylint: justify=Without going into too much detail the deadline is approaching and
# pylint: ClientA is really important for the financial well being of the company,
# pylint: and also I have a kid, a husband and a cat to feed, you know. 
# pylint: To synthesize the current data succinctly: this is acceptable technical debt
# pylint: that the next guy will handle.
# pylint: disable-next=message_one, message_two
def func():
    pass

or

# pylint: justify=Without going into too much detail the deadline is approaching and
# ClientA is really important for the financial well being of the company,
# and also I have a kid, a husband and a cat to feed, you know. 
# To synthesize the current data succinctly: this is acceptable technical debt
# that the next guy will handle.
# pylint: disable-next=message_one, message_two
def func():
    pass

or

# pylint: justify
# Without going into too much detail the deadline is approaching and
# ClientA is really important for the financial well being of the company,
# and also I have a kid, a husband and a cat to feed, you know. 
# To synthesize the current data succinctly: this is acceptable technical debt
# that the next guy will handle.
# pylint: end-justify
# pylint: disable-next=message_one, message_two
def func():
    pass

is doable ? It seems the handling in pylint will be a lot harder to do though.

[DanielNoord]

Maybe:

# pylint: justify=Without going into too much detail the deadline is approaching and
# pylint: ClientA is really important for the financial well being of the company,
# pylint: and also I have a kid, a husband and a cat to feed, you know. 
# pylint: To synthesize the current data succinctly: this is acceptable technical debt
# pylint: that the next guy will handle.
# pylint: disable-next=message_one, message_two
def func():
    pass

I like this one. Adding the pylint will probably help with implementation and also signals the intent of the comment.

Justify-next is not necessarily meaningless if we interpret it as "justifying a disable that is on the line after the justification" which is similar to "disabling a message that is on the line after the disable". However, we could also make this the normal behaviour, ie., justify can be used on the same or the line directly above a disable. I think that might be best, although it should be properly documented.

[Pierre-Sassoulas]

Regarding the scope justification, we already have something in place for disable/enable, justify could work the same way and be re-enabled the same way also.

def func():
    # pylint: justify=This justify 'message' for the whole scope
    # pylint: disable=message
    # pylint: disable=other-message justify=This justify 'other_message' for the whole scope
    something = 1/0 # pylint: disable=E, justify=some valid reason that justify only this line
    violating_message_here = "without pylint warning"
    violating_other_message_here = "without pylint warning"
    # pylint: enable=message # Also end the justify for message
    violating_message_here = "with a pylint warning!"  # pylint: disable=message # it will also need an inline justify
    violating_other_message_here = "without pylint warning"

Could we create an alias for "disable" / "enable", respectfully "justify" / "condemn" that would work the same way than enable/disable and require a reason for "disable" ?

def func():
    # pylint: justify=message reason=This justify 'message' for the whole scope
    # pylint: and can span multiple line that start with 'pylint:'
    # pylint: justify=other-message reason=This justify 'other_message' for the whole scope
    something = 1/0 # pylint: justify=E, reason=some valid reason that justify only this line
    violating_message_here = "without pylint warning"
    violating_other_message_here = "without pylint warning"
    # pylint: condemn=message # End the justification for 'message' also enable 'message'
    violating_message_here = "with a pylint warning!"  # pylint: justify=message # it will also need a reason
    violating_other_message_here = "without pylint warning"

Or maybe we're complicating this for nothing and we can add a reason to what already exists:

def func():
    # pylint: disable=message reason=This justify 'message' for the whole scope
    # pylint: and can span multiple line that start with 'pylint:'
    # pylint: disable=other-message reason=This justify 'other_message' for the whole scope
    something = 1/0 # pylint: disable=E, reason=some valid reason that justify only this line
    violating_message_here = "without pylint warning"
    violating_other_message_here = "without pylint warning"
    # pylint: enable=message # End the justification for 'message' also enable 'message'
    violating_message_here = "with a pylint warning!"  # pylint: disable=message # it will also need a reason
    violating_other_message_here = "without pylint warning"

[DanielNoord]

Regarding the scope justification, we already have something in place for disable/enable, justify could work the same way and be re-enabled the same way also.

def func():
    # pylint: justify=This justify 'message' for the whole scope
    # pylint: disable=message
    # pylint: disable=other-message justify=This justify 'other_message' for the whole scope
    something = 1/0 # pylint: disable=E, justify=some valid reason that justify only this line
    violating_message_here = "without pylint warning"
    violating_other_message_here = "without pylint warning"
    # pylint: enable=message # Also end the justify for message
    violating_message_here = "with a pylint warning!"  # pylint: disable=message # it will also need an inline justify
    violating_other_message_here = "without pylint warning"

Something like this should work I think, combined with your earlier example of splitting across multiple lines.

Could we create an alias for "disable" / "enable", respectfully "justify" / "condemn" that would work the same way than enable/disable and require a reason for "disable" ?

def func():
    # pylint: justify=message reason=This justify 'message' for the whole scope
    # pylint: and can span multiple line that start with 'pylint:'
    # pylint: justify=other-message reason=This justify 'other_message' for the whole scope
    something = 1/0 # pylint: justify=E, reason=some valid reason that justify only this line
    violating_message_here = "without pylint warning"
    violating_other_message_here = "without pylint warning"
    # pylint: condemn=message # End the justification for 'message' also enable 'message'
    violating_message_here = "with a pylint warning!"  # pylint: justify=message # it will also need a reason
    violating_other_message_here = "without pylint warning"

I think decoupling the justify from a disable might complicate this and require additional work. For a first implementation I think it might be better to focus on creating a working implementation that allows: 1) justification on the same line, 2) justification on the preceding line, 3) multi-line justification.

Later on we could consider stuff like condemn and justify-scope.

With a basic implementation this should still work:

def func():
     # pylint: justify=I think z is a nice name
     # pylint: disable=invalid-name
     z = 1
     z = 2

In the future justify-scope could do something like this:

def func():
     # pylint: justify-scope=invalid-name reason=I think z is a nice name
     z = 1 # pylint: disable=invalid-name
     z = 2 # pylint: disable=invalid-name

Or maybe we're complicating this for nothing and we can add a reason to what already exists:

def func():
    # pylint: disable=message reason=This justify 'message' for the whole scope
    # pylint: and can span multiple line that start with 'pylint:'
    # pylint: disable=other-message reason=This justify 'other_message' for the whole scope
    something = 1/0 # pylint: disable=E, reason=some valid reason that justify only this line
    violating_message_here = "without pylint warning"
    violating_other_message_here = "without pylint warning"
    # pylint: enable=message # End the justification for 'message' also enable 'message'
    violating_message_here = "with a pylint warning!"  # pylint: disable=message # it will also need a reason
    violating_other_message_here = "without pylint warning"

What would we do when the disable is already at line length? We can't add a reason then. We will need some way of putting a justification on the preceding line I think.

[Pierre-Sassoulas]

What would we do when the disable is already at line length? We can't add a reason then. We will need some way of putting a justification on the preceding line I think.

If we're going to handle the multi-line justification, we might as well handle multi-line disable with a reason, right ? The disable-next could be applied after the last line of comment starting with # pylint: ie:

def func():
    # pylint: disable-next=message reason=This justify 'message' for the next line 
    # pylint: and can span multiple line that start with 'pylint:'
    violating_message_here = "without pylint warning"
    violating_message_here = "with a pylint warning"

[DanielNoord]

Just some additional thoughts:

Allowing justification on the scope level complicates things as it requires much more of this function.
If we only do same and preceding line we can just check for justification on node.lineno and node.lineno - 1. If we do something with scopes we will need to write a way to record, disable and enable justifications similar to how we do for messages.

I wonder if the work is comparable to the benefit? How often do you disable the same message multiple times in a scope when you can't disable it at the scope level itself? If you disable it at the scope level a justification on the same or preceding line will work anyway.
Only when you can't disable on the scope level you will need to add a justification to each individual message. This might clutter your code, but if you're requiring justification for each disable you probably don't mind large blocks of comments in your code anyway.
It seems that we might be trying to fix a niche problem and thereby unnecessarily complicating this suggestion which in a basic form could already be quite useful.

[DanielNoord]

If we're going to handle the multi-line justification, we might as well handle multi-line disable with a reason, right ? The disable-next could be applied after the last line of comment starting with # pylint: ie:

def func():
    # pylint: disable-next=message reason=This justify 'message' for the next line 
    # pylint: and can span multiple line that start with 'pylint:'
    violating_message_here = "without pylint warning"
    violating_message_here = "with a pylint warning"

def func():
    # pylint: disable-next=message, other-message, another-message-making-this-line-very-long
    # pylint: reason=This justifies the preceding line and can
    # pylint: itself span multiple line that start with 'pylint:'
    violating_message_here = "without pylint warning"
    violating_message_here = "with a pylint warning"

We should then also add this to the possible ways to justify I guess?

[Pierre-Sassoulas]

How often do you disable the same message multiple times in a scope when you can't disable it at the scope level itself? If you disable it at the scope level a justification on the same or preceding line will work anyway.

Not very often, but it's already implemented in the existing disable/enable. If we piggy pack on that feature and obly add a "reason" that can be checked by another extension, it might be reasonable to implement. The problem is really the multiline comment starting with # pylint: that could need some work to get right. If we consider each consecutive line starting with # pylint: as a single line we migght be able to take node.lineno and node.lineno - 1 with lineno the line of the last line of the comment ?

[Pierre-Sassoulas]

I like this, we could then do

def func():
    # pylint: disable-next=message, other-message, another-message-making-this-line-very-long
    # pylint: yet-another-message reason=This justifies the preceding line...

Instead of the current:

def func():
    # pylint: disable=message, other-message, another-message-making-this-line-very-long
    # pylint: disable=yet-another-message

[DanielNoord]

I like that! I feel like we're getting somewhere with this 😄

To make that version work though we will first need to make disable allow "multi-line parameters". I have no idea how difficult it will be to implement that, but might be a nice thing to have anyway.

[Pierre-Sassoulas]

I opened an issue for the preliminary work regarding multi-line disable: #5258