[python:S5886] False-positive for generators annotated with Iterator for return type

Clean Code Report False-positive / False-negative...
1

Hello,

SonarCloud reports a false-positive on rule python:S5886 when the annotation for the return type of a generator is Iterator[YieldType], for example:

def infinite_stream(start: int) -> Iterator[int]:
    while True:
        yield start
        start += 1

The previous example is taken from the Python documentation that states that generators that only yield values may be anotated “as having a return type of either Iterable[YieldType] or Iterator[YieldType]” instead of Generator[YieldType, None, None].

1 Like
4

Hello @Rogdham,

First of all, sorry for the late answer.

Indeed, S5886 should not raise any issue in this case. I’m also not able to reproduce any false positive with the example you gave.

However, I see we do have a false positive in the following example (taken from the same documentation page), due to the fact that the yield statement is part of an assignment statement.

def echo_round() -> Generator[int, float, str]:
    sent = yield 0
    while sent >= 0:
        sent = yield round(sent)
    return 'Done'  # FP S5886 here

Or, in this slightly modified reproducer:

def echo_round() -> Generator[int, float, str]:
    sent = 0
    while sent < 3:
        sent += 1
        x = yield sent
    return 'Done'  # FP S5886 here

To address this, I created the following ticket. Can you confirm the issue you’re facing is similar? If not, can you please give me a reproducer that raises a false positive?

Cheers,
Guillaume

5

Hello Guillaume and thank you for your reply.

I am honestly surprised the example I gave did not reproduce. My issue arise when the returned type is annotated with Iterator[XXX] (e.g. Iterator[str]). It happens quite a lot, since I find it more readable than Generator[str, None, None] so I use it a lot in my code.

In any case, I have many reproducers in the following opensource project: https://sonarcloud.io/project/issues?resolved=false&rules=python%3AS5886&id=Rogdham_sdkite&open=AYQuDYIyNu-VKzOZ3L9K

    def __iter__(self) -> Iterator[str]:
        for values in self._contents.values():
            yield values[0]
6

Hello @Rogdham,

Thank you for providing more context!

Thanks to the the SonarCloud example you gave, I managed to reproduce the false positive:

import sys
if sys.version_info < (3, 9):
    from typing import Iterator
else:
    from collections.abc import Iterator

def my_iterator() -> Iterator[str]:
    yield "hello" # FP S5886

The issue is raised because of the 2 imports of Iterator. The analyzer somehow fails to disambiguate them and to understand that both underlying symbols are valid return types for a generator.

I created SONARPY-1256 to address this.

Cheers,
Guillaume

7

Oh woah, good catch, thank you for the deep dive and follow-up!

9

Hello Guillaume,

I have found the same issue with a simpler reproducer that may indicate this is not a type disambiguation problem.

from __future__ import annotations

from collections.abc import Iterable

def foo() -> Iterable[str]:
  yield ""

Given the error of

annotate function “run” with “typing.Generator” or one of its supertypes

I suspect the underlying problem is that typing.Generator is not being abstract/erased to collections.abc.Generator, and so collections.abc.Iterable is not considered a supertype.

Edit to include example: SonarCloud

4 Likes
10

Confusingly it doesn’t work even when you specify the Generator type from collections.abc itself.
Using python 3.11:

import collections.abc

def foo() -> collections.abc.Generator[str]:
  yield ""

still raises the warning

11

Also having the same problem as Benedict Harcourt, with returning collections.abc.Iterable from a generator function.

We are using SonarQube 9.9, the Sonar Way profile for python and submitting with sonar.python.version=3.10

13

Hello,

Thanks for the additional examples.

Indeed, it seems that the rule strictly expects something annotated as typing.Iterator, typing.Iterable, typing.Generator or their async alternatives. And this flew under our radar for quite some time now…

I’ve updated the ticket accordingly and I’ve bumped its priority so that we make sure to address this.

Thanks for your feedback, and sorry we didn’t respond on this for quite some time.

Cheers,
Guillaume

1 Like