Disable next should consider the next occurance of an error not only of the next line [Regression]

[CarliJoy]

Bug description

With #7411 by @DanielNoord # pylint: disable-next= only considers lint errors of next line as ignored (in order to fix #7401) So this is the Regression Issue that @Pierre-Sassoulas already saw comming ;-)

The problem a bit more in detail: Before this PR it was easy to ignore errors of bigger functions definition, i.e.

    @validator("sender_mail", always=True, pre=True)
    # pylint: disable-next=no-self-use, no-self-argument, unused-argument
    def set_email_if_possible(
        cls, value: Any, values: dict[str, Any], extra_information_place_holder: dict[str, Any]
    ) -> Optional[NameEmail]:
        try:
            return split_name_and_mail(values.get("sender_from", ""))
        except ValueError:
            return None

and the usage was quite clear to me. Now it behaves more like a disable-for-next-line and not like a disable the next occurrence of, which the name suggests to me and co-workers.

This behaviour was a blessing especially using black.

Black brakes too long lines, so if you add a disable line that is too long (at least it did in the past). This makes it hard to find a working combination things that is not braking black and pylint.

So with this MR/the new version I have start all over again, trying to get stuff to work with black:

    @validator("sender_mail", always=True, pre=True)
    # pylint: disable-next=no-self-argument, unused-argument
    def set_email_if_possible(
       # pylint: disable-next=unused-argument
        cls, value: Any, values: dict[str, Any], extra_information_place_holder: dict[str, Any]
    ) -> Optional[NameEmail]:
        try:
            return split_name_and_mail(values.get("sender_from", ""))
        except ValueError:
            return None

The unused argument are value and extra_information_place_holder. Now adding some more typing information will cause black to write each variable on a new line. So I have to adopt the ignore line again and worse duplicate it.

Configuration

not relevant

Command used

pylint file.py

Pylint output

Displays the error that was ignored before the merge of #7411

Expected behavior

Restore the old behaviour of ignoring the next occurrence of the defined errors.

Or at least consider the context of the next line. So I write # pylint: disable-next= before a function, I would think that all error of the function definition are ignored, i.e. unused-argument. And I also would assume that all errors, that exist in the context of a function, i.e. too-many-locals are also ignored within the function.

To give a bit more background: The # pylint: disable-next feature was one of the reasons we could convince people to use black company wide (pylint was already activated company wide). Before it was really a pain, when black reformatted stuff causing pylint to fail. Adding block of # pylint: disable #pylint: enable were error prone and ugly.

Pylint version

pylint 2.15.3
astroid 2.12.10
Python 3.10.6 | company distribution | (main, Sep 29 2022, 13:58:30) [GCC 11.2.1 20220127 (Red Hat 11.2.1-9)]

OS / Environment

No response

Additional dependencies

No response

[sscherfke]

🤔 If disable-next={issue} would disable the "next occurance of {issue}" and the next line would be okay – than we might ignore an issue a lot further down the code where we would not expect it to be ignored.

Imho, disable-next only disabling an issue in the next makes it's behavior a lot more predictable.

I don't know if disable-next{-statement} that considered the next multiline statement (e.g., a long function defnition) would be possible … but if you need to break a function def into mulitple lines, you can always insert a disable-next{-line} above the appropriate line.

[DanielNoord]

Or at least consider the context of the next line. So I write # pylint: disable-next= before a function, I would think that all error of the function definition are ignored, i.e. unused-argument. And I also would assume that all errors, that exist in the context of a function, i.e. too-many-locals are also ignored within the function.

I think this would be best. I agree that this is an annoying regression and I'm not sure if it is actually fixable without regressing on the earlier issue but we should definitely at least try.

I'm not really in favour of doing stuff like "until next occurrence" or allowing additional statements within that comment. We would just need to check if the next line is a scope creating node (ie., a LocalsDictNodeNG) and write some special handling for it.

If not possible I do think we should favour the current solution over reverting. Although I 100% agree it is annoying and less user-friendly it is more predictable and I think that's (almost) always better.

Edit: I'm adding High priority and hacktoberfest in the hopes of this picking up attention from someone to work on it. I don't have the time to do so myself but I'd happily help review this and merge a PR asap.

[CarliJoy]

Okay sounds good to me. As a try for a specification I would suggest the following disable-next disables the given codes:

• for the following line
• if the following line starts a multi line statement (function definition, long if, some statement wrapped in brackets, a dict definition), then the codes are disabled until this statement ends

[Pierre-Sassoulas]

As said in https://github.com/PyCQA/pylint/issues/7689 there's also the comment to take into consideration when specifying what disable-next should do. I think if we don't make it simple and stupid this is going to get very complicated very fast.

[finite-state-machine]

As Pierre mentioned, there's some discussion in #7689 that I hope will be helpful. The crux of that issue is that this should work as expected:

# pylint: disable-next=abstract-method
#         we don't override some abstract methods of 'Mapping', because
#         this is just an interface for mypy
class Something(Mapping[str, str]):
    ...

...but in fact the disable-next is applied to the first comment immediately below it.

(There's also an alternative suggestion that this form should be tolerated:)

# pylint: disable-next=abstract-method
# pylint: #     we don't override some abstract methods of 'Mapping',
# pylint: #     because this is just an interface for MyPy
class Something(Mapping[str, str]):
    ...

[DanielNoord]

I think the latter issue can't really be fixed due to how pylint and ast work. The recommendation is to just put the explaining comment above the disable comment instead of the other way around.

[finite-state-machine]

@DanielNoord that's the workaround I've been using, but it's really not ideal. Hopefully solving #7601 will solve the ugly comment problem as well.

[DanielNoord]

Sadly it won't. This is just a limitation of how the standard library parses Python code and comments in its ast library. There is not much we can do here

[CarliJoy]

I also would see this as minor Issue. You are very flexible in how you write your comments, whereas how Black formats stuff is not at all adjustable.