inconsistent-return-statements with raise in function call

[crashmaster]

Hello!

Steps to reproduce

def raise_boo(value):
    raise RuntimeError(format_boo(value))


def check_boo(value):
    if is_valid_boo(value):
        return value
    else:
        raise_boo(value)

Current behavior

Does not recognize raise in the function call.
In this case depth is only 1, but can be more.

Expected behavior

I think it should.

pylint --version output

% pylint --version
No config file found, using default configuration
pylint 1.8.1, 
astroid 1.6.0
Python 2.7.12 (default, Nov 19 2016, 06:48:10) 
[GCC 5.4.0 20160609]

Thanks!

[dmerejkowsky]

Same issue even with no function call:

def do_foo(a_arg):
    if a_arg:
        return "foo"
    assert False

R:  4, 0: Either all return statements in a function should return an expression, 
or none of them should. (inconsistent-return-statements)

[hippo91]

@PCManticore, for the first issue it seems that we have to ensure that the function called will always end with a raise statement. Do we need flow control for that or should i just code a function that recursively check if the function ends with raise only?

[ddaanet]

• Functions that always raise obfuscate the excecution flow of callers. Instead, they should return the exception to raise in the caller.
• assert False is equivalent to raise AssertionError, which is more obvious and does not cause inconsistent-return-statements.

But on the other hand, this causes a bogus inconsistent-return-statement:

from socket import error

def return_error():
    return error


def raise_error(flag):
    if flag:
        return None
    raise return_error()

But if return_error returns ValueError, then it does not cause inconsistent-return-statement, so that appears to just be a special case of #1794, second function.

[mthuurne]

This function also causes an inconsistent-return-statements message to be reported:

def func(done):
    while True:
        if done():
            return 42

So perhaps the problem is not in how raise is handled, but a general problem of knowing that if the end of the function is unreachable, there is no implicit return with no expression.

[hippo91]

@mthuurne your function should not raise anymore the inconsistent-return-statements message since PR #1806.
Can you confirm please?

[hippo91]

@ddaanet thank you for your remarks!
I totally agree with you, that's why i'm not sure we should work more on the two first issues.
@PCManticore what do you think about it?
@ddaanet, i will have a look on your bogus inconsistent-return-statement by answering issue #1794.
Thanks again.

[PCManticore]

@hippo91 I agree with you and with @ddaanet as well. The expectations for this check should be that it cannot detect all the edge cases, but should work most of the time. If it needs flow control understanding to determine if the end of a function is reachable or not, then the simplest solution would be to disable the message locally and move on.

[tuukkamustonen]

@hippo91 Indeed, @mthuurne's #1782 (comment) does not reproduce on 2.0 anymore. But this does:

def func():
    if True:
        while True:
            break
        return True
    else:
        raise RuntimeError()

(Sure, doesn't make much sense as is.)

[hippo91]

@tuukkamustonen thanks for your remark. I'll work on your issue ASAP.

[hippo91]

@tuukkamustonen in your example pylint warns you that the else statement is useless.
If you remove it then the inconsistent-return-statements message goes away.

For example:

#pylint: disable=C0111
def func(val):
    if val <= 3:
        while True:
            break
        return True
    raise RuntimeError()

doesn't raise any messages.
I close this issue then.
@tuukkamustonen feel free to reopen it or another one if i misinterpreted your comment.

[tuukkamustonen]

@hippo91 True, if you remove else then the warning goes away.

But the warning still shouldn't be there with else, or it should say something like "remove unnecessary else block" instead. Now it's just a false positive and confusing to the user.

Besides, behind that unnecessary else, there can be stylistic reasons. So I think there's still something to fix here. What do you think?

[mthuurne]

It's possible in pylint to disable individual messages, so a user might have the message about unnecessary else blocks disabled and the inconsistent-return-statements message enabled. The latter message is likely to find bugs while the former is about readability (and it's debatable whether removing unnecessary else blocks always improves readability), so enabling one and disabling the other is not just a theoretical case, but is likely to happen in practice too, when a user tries to create a configuration that reports only the most urgent issues.

[PCManticore]

I'm not exactly sure how is that message a false positive for the following:

#pylint: disable=C0111
def func(val):
    if val <= 3:
        while True:
            break
        return True
    else:
        raise RuntimeError()

This is exactly what this check is supposed to catch, so not sure where the stylistic reason comes into. Or is the complaint that the message should be improved?

@mthuurne Yes, it's possible in pylint to disable individual messages, both in the configuration file and at the CLI.

[mthuurne]

What I meant is that since individual messages can be disabled, every message should be considered in isolation. The motivation given for closing the inconsistent-return-statements issue was that it could only be reproduced while pylint reported the "unnecessary else" message. But those are two separate messages, so in my opinion the original issue with inconsistent returns is not fully fixed yet.

I think that the false positive that @tuukkamustonen referred to is the inconsistent-return-statements message, not to the unnecessary else message. The inconsistent-return-statements message is a false positive, since there is no way the function can exit successfully without returning a value: the only exits are return True and raise.

[PCManticore]

Ah gotcha, the false positive was inconsistent-return-statements not no-else-return, that makes sense.

[tuukkamustonen]

the false positive was inconsistent-return-statements not no-else-return

Yes, this.

(I wasn't even aware there's no-else-return - just threw an idea back there that there could be 😄.)

[hippo91]

@tuukkamustonen @mthuurne you are effectively right.
I will work on it ASAP.

[HemabhKamboj]

@PCManticore, @hippo91
Hello !
This is my pylint --version output

pylint 2.4.4
astroid 2.3.3
Python 3.8.1 (default, Dec 27 2019, 18:06:00)

I guess the issue still persists!

I am having the following code

if a>b :
    return
else:
    raise ValueError('some string')

Current Behaviour

R1705: Unnecessary "else" after "return" (no-else-return)

Expected Behaviour
It should work

Thanks!

[mthuurne]

I am having the following code

if a>b :
    return
else:
    raise ValueError('some string')

Current Behaviour

R1705: Unnecessary "else" after "return" (no-else-return)

I think PyLint is correct here: the else is unnecessary. But if you think the code is more readable with the else statement than without it (I would agree), you can disable the no-else-return message in your pylintrc.

Note that this issue was originally about inconsistent-return-statement being reported, which was incorrect.

[HemabhKamboj]

@mthuurne
got it.
My bad
Thanks!

[nikitha9792000]

"""importing datetime module"""
import datetime

def days_in_month(year, month):

"""
Inputs:
  year  - an integer between datetime.MINYEAR and datetime.MAXYEAR
          representing the year
  month - an integer between 1 and 12 representing the month

Returns:
  The number of days in the input month.
"""
if datetime.MINYEAR <= year <= datetime.MAXYEAR:
    if month in(1, 3, 5, 7, 8, 10, 12):
        return 31
    elif((month == 2) and((year%400 == 0) or(year%4 == 0 and year%100 != 0))):	
        return 29
    elif(month == 2):
        return 28
    return 30

def is_valid_date(year, month, day):
"""
Inputs:
year - an integer representing the year
month - an integer representing the month
day - an integer representing the day

Returns:
  True if year-month-day is a valid date and
  False otherwise
"""
isvaliddate = True
try:
    datetime.datetime(int(year), int(month), int(day))
except ValueError:
    isvaliddate = False

if(isvaliddate):
    return True
else:
    return False

x = int(input())
y = int(input())
print(days_in_month(x, y))

l = int(input())
m = int(input())
n = int(input())
print(is_valid_date(l, m, n))

Iam getting either all the return statements should return an expression or none of them should in days_in_month function.

[hippo91]

@nikitha9792000
I think pylint is right here. In your function days_in_month, if the first if condition is not checked then the function returns None by default. As you explicitly specified some of the returns then you should specify all of them. The following snippet should be more in agreement with pylint:

def days_in_month(year, month):
"""
Inputs:
  year  - an integer between datetime.MINYEAR and datetime.MAXYEAR
          representing the year
  month - an integer between 1 and 12 representing the month

Returns:
  The number of days in the input month.
"""
if datetime.MINYEAR <= year <= datetime.MAXYEAR:
    if month in(1, 3, 5, 7, 8, 10, 12):
        return 31
    elif((month == 2) and((year%400 == 0) or(year%4 == 0 and year%100 != 0))):	
        return 29
    elif(month == 2):
        return 28
    return 30
return None

[nikitha9792000]

@hippo91
Thankyou it worked.
But Iam getting the following import error.
(An exception occurred when the code was imported before testing: (EOFError) EOF when reading a line at line 80, in
. Be sure to comment out any lines that 'run' your code when the file is loaded.)
80th line means the line accepting x as input.

[earonesty]

functions that always raise are useful, be nice if you could just use type hints to point it out.

[mthuurne]

functions that always raise are useful, be nice if you could just use type hints to point it out.

Agreed: a NoReturn annotation could be used in code path analysis.

Given how confusing the history of this issue already is, it might be better to open a new issue for that feature request instead of continuing here.

[hippo91]

@earonesty @mthuurne you are right but no need to open a new issue as it would be a duplicate of #4122. There is already a PR for this #4304.