False negative - assign `list.reverse()` instead of `reversed(list)`

[orSolocate]

Current problem

Parallel issue (being 'brought back from the grave') - #5722.

when you use the reverse() method on a list object, it modifies (reverses) the list and returns None.
A common mistake is to assign the return value of this assignment (i.e. None) to a variable thinking you assigned the sorted list instead.

lst=[3,2,1]
A=lst.reverse()

Results with:
None in A and [1,2,3] in lst.

The problem is that Pylint does not detect that, running this snippet shows no messages.

Desired solution

When this case is detected, a message should be emitted explaining the user to use the built-in reversed() function instead.
Issue is a parallel case of issue #5722, using the assignment-from-none message looks proper to me.

Additional context

There might be a compatibility issue with detecting the list.sort() and list.reverse() methods in newer version of python, where pylint sees the stub function (that contains only pass) and thinks it is a function with no return, emitting the assignment-from-no-return error message. When running pylint on the following snippet on Python3.8 and Python3.10 I got:

Snippet:

lst = [1, 2, 3]
rev_lst = lst.reverse()
sort_lst_err = lst.sort()

Python3.8: got assignment-from-none for the "lst.sort()" line.
Python3.10: got assignment-from-no-return for both lst.reverse() and lst.sort() lines.

Is it a false-positive? or did I do something wrong?

[orSolocate]

I could work on it if we decide we want to have it.

[Pierre-Sassoulas]

Thank you for opening the issue. I think the first thing to do for assignment-from-none is to follow Jacob's suggestion here: #7853 (comment) : it's going to have a big impact maybe even on your specific case. Then I suppose that adding a proper message to hint about how to fix on known treacherous functions would be worth it. Also remind me of #8263 (it's the opposite, you should assign but you're not. Creating a generic message with a proper specific hint instead of a new message for each is better)

[orSolocate]

Hi @Pierre-Sassoulas , long time no see, nice to work with you again. I saw the #7853 comment, I could retry fixing the infer logic now that astroid is hopefully improved. But I think it can be done in parallel with implementing the issue here. A specific message for this one is needed in my opinion because, like in the list.sort() case, we want to advice the user to use the reversed() function. Could you enlighten how #8263 is related (it was hard for me to understand what it is about).

[Pierre-Sassoulas]

Hey likewise @orSolocate , glad to see you're back :)

I think the message can be generic with an added precision: i.e.:
Assigning result of 'foobar' to a variable, but 'foobar' returns None in the general case.
Assigning result of 'reverse' to a variable, but 'reverse' returns None, did you mean 'reversed(...)' ? for the specific case based on a dict:

KNOWN_TREACHEROUS_FUNCTIONS = {
    "reverse": "reversed",
    "sort": "sorted"
}

The same could be done for function that are known to be treacherous but that return something when you don't expect them to (the message in #8263 is kind of the opposite of assignment-from-none or assignment-from-no-return).

[orSolocate]

@Pierre-Sassoulas elegant idea, I like it. Do I have a green light to start working on it?

[Pierre-Sassoulas]

Sure, the need PR label was maybe a little premature before but now I feel it's even better specified. Thank you for working on it, this checker would have been useful for me in the past for sure and probably even today.

[orSolocate]

I looked a little deeper in the code, I think I have a better context of this issue.
First of all, regarding python3.10+ emitting assignment-from-no-return instead of assignment-from-none - it is the intended behavior, see #8810. I went on a tour along history lane and discovered that there was a suggestion to merge the two messages, but it was rejected as the information that the assignment was from function that explicitly return None might be more helpful for the user, but for built-in function like list.sort() it was agreed to "merge it with" the assignment-from-no-return message.

Regarding python3.8, I don't know why it returned the old message, anyhow the version is not supported by Pylint anymore so I guess it does not matter too much (does it?)

It all makes me think:

1. Are we happy with that built-in return the assignment-from-no-return message? I understand why the message was correct only after understanding the history behind it..

2. what should we do with this issue? I could either closed it or, since list.reverse() is one of the common cases, add it as a test case in tests\functional\a\assignment\assignment_from_no_return_2.py? That being said, I know that the current approach is to keep the functional test concise.

@jacobtylerwalls If you are still around, what do you think? (I hope you remember this change)

[jacobtylerwalls]

I think the current message makes sense. Happy to add a test case. Or look at a docs suggestion if you think understanding the message can be made simpler?

[orSolocate]

Could add the test case for list.reverse() in tests\functional\a\assignment\assignment_from_no_return_2.py, should this be the new purpose of this issue?

assignment-from-no-return raised in three cases:

1. no 'return` statement
2. empty 'return'
3. built-in method that returns only None
   The docs suggestion covers only case 1. It might be confusing, since maybe not all developers know that all of the 3 cases, either explicitly or implicitly, return None.

Current description in docs:
Used when an assignment is done on a function call but the inferred function doesn't return anything.

Suggestion description:
Used when an assignment is done on a function call but the inferred function doesn't return anything either implicitly or explicitly.

Current suggestion in docs:

def add(x, y):
    print(x + y)


value = add(10, 10)  # [assignment-from-no-return]

suggested improvement:

def add(x, y):
    print(x + y)

def add_empty_return(x, y):
    print(x + y)
    return

value = add(10, 10)  # [assignment-from-no-return]
value = add_empty_return(10, 10)  # [assignment-from-no-return]

lst = [10, 10]
value = lst.sort() # [assignment-from-no-return]

assignment-from-none raised in one case:

1. "custom" function that returns only 'None'
   The docs suggestion covers this case :)

What do you think?