afonasev / flake8-return

Flake8 plugin for return expressions checking.
MIT License
62 stars 69 forks source link

In some circumstances the error R506 can be dangerous if the `raise` will be changed later #128

Closed sshishov closed 1 year ago

sshishov commented 1 year ago

Description

The error reported for R506 can be dangerous in fast changing development environment

What I Did

Let assume we have this piece of code

def main():
    for a in range(10):                             # | 0 exception bad value
        try:                                        # | 1 ordinary value
            if a % 5 == 0:                          # | 2 good value
                raise ValueError('bad value')       # | 3 ordinary value
            elif a % 2 == 0:                        # | 4 good value
                print(a, 'good value')              # | 5 exception bad value
            else:                                   # | 6 good value
                print(a, 'ordinary value')          # | 7 ordinary value
        except ValueError as e:                     # | 8 good value
            print(a, 'exception', e)                # | 9 ordinary value

if __name__ == '__main__':
    main()

The error reported is the following:

myfile.py:4:13: R506 unnecessary elif after raise statement.
            if a % 5 == 0:

Now let assume we change it according to the proposal, but after short period of time another developer come and changed the code slightly (because IF can be quite big and not intuitive), he removed raise and add the handling of situation inside and removed try...except all together. Now let's see the result:

def main():                                         # | 0 exception bad value
    for a in range(10):                             # | 0 good value
        if a % 5 == 0:                              # | 1 ordinary value
            print(a, 'exception', 'bad value')      # | 2 good value
        if a % 2 == 0:                              # | 3 ordinary value
            print(a, 'good value')                  # | 4 good value
        else:                                       # | 5 exception bad value
            print(a, 'ordinary value')              # | 5 ordinary value
                                                    # | 6 good value
                                                    # | 7 ordinary value
if __name__ == '__main__':                          # | 8 good value
    main()                                          # | 9 ordinary value

As we can see the result is not what the developer expected, but... sometimes it is very difficult to notice such issues. As elif has SOME MEANING behind, usually...

afonasev commented 1 year ago

Thank you for your issue! I agree that this is a controversial case. I'm not sure if it's possible to solve using the rules in the plugin. I would be glad to accept your contributions.

sshishov commented 1 year ago

@afonasev I assume we can close this issue as it is something we cannot change... It is the modification of the code causing the issue, not the applier rule.

The same way we can complain if new developer comes and add the line like a = 100 / 0, therefore let's not overthink and overreact.

Closing as won't fix.

afonasev commented 1 year ago

Ok. If needed, you can always disable the R506 rule via the noqa instruction locally or via configuration globally.