[flake8-self] Make SLF diagnostics robust to non-self-named variables

[charliermarsh]

Summary

We allow private attribute access within the implementing class (e.g., self._foo), but historically, this was implemented by matching on self, cls, and mcs. So, e.g., if you used a self variable name other than self, we'd still flag accesses.

With this PR, we now model self correctly by detecting it as the "first argument to a method", removing those false positives.

Closes #24275.

[astral-sh-bot]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

Hmm, while this is certainly more accurate, (almost) all ecosystem reports are now undesired false positives, where I'd prefer a few false negatives.

[charliermarsh]

I'm actually not sure that these are all false positives... The accesses within decorators seem like true positives?

[MichaReiser]

The accesses within decorators seem like true positives?

I didn't review all :) Which one is that?

[dscorbett]

There are more false positives than I expected. The main pattern is mocking or patching existing classes. It might be better to suppress the diagnostic when self is a function’s first parameter.

[charliermarsh]

@MichaReiser -- An example would be like the following from home-assistant:

def async_log_errors[_DenonDeviceT: DenonDevice, **_P, _R](
    func: Callable[Concatenate[_DenonDeviceT, _P], Awaitable[_R]],
) -> Callable[Concatenate[_DenonDeviceT, _P], Coroutine[Any, Any, _R | None]]:
    """Log errors occurred when calling a Denon AVR receiver.

    Decorates methods of DenonDevice class.
    Declaration of staticmethod for this method is at the end of this class.
    """

    @wraps(func)
    async def wrapper(
        self: _DenonDeviceT, *args: _P.args, **kwargs: _P.kwargs
    ) -> _R | None:
        available = True
        try:
            return await func(self, *args, **kwargs)
        except AvrTimoutError:
            available = False
            if self.available:
                _LOGGER.warning(
                    (
                        "Timeout connecting to Denon AVR receiver at host %s. "
                        "Device is unavailable"
                    ),
                    self._receiver.host,
                )
                self._attr_available = False
        except AvrNetworkError:
            available = False
            if self.available:
                _LOGGER.warning(
                    (
                        "Network error connecting to Denon AVR receiver at host %s. "
                        "Device is unavailable"
                    ),
                    self._receiver.host,
                )
                self._attr_available = False
        except AvrProcessingError:
            available = True
            if self.available:
                _LOGGER.warning(
                    (
                        "Update of Denon AVR receiver at host %s not complete. "
                        "Device is still available"
                    ),
                    self._receiver.host,
                )

I would be fine to keep the blanket exemption on (at least) self though. It seems fairly unobjectionable.

[MichaReiser]

I guess let's ignore Ruff's semantic limitations for now. What's unclear to me is what the rule's desired behavior for method decorators? It seems a legitimate pattern, at least when the decorator is defined in the same file. Or are there reasons why method decorators should be forbidden to read private fields and, if so, what should users do instead?

I'm okay with the blanked exemption of self for now. The ecosystem shows that the community primarily names variables self for types they consider to be the Self type.

(Link to one of the homeassistant examples https://github.com/home-assistant/core/blob/31a24446a8a1aefa1f039b5dcf647dedcdf7f090/homeassistant/components/denonavr/media_player.py#L136)

[charliermarsh]

It seems a legitimate pattern, at least when the decorator is defined in the same file.

Yeah, I think I agree here.

I'm going to avoid removing the self, cls, and mcs exemption, which should make this purely a bug fix to avoid what were previously false negatives.