Skip to content

Type hints improvements - fixing static code analyses issues related to combined sync and async return types(vectorsets + commands in core.py)#3991

Merged
petyaslavova merged 20 commits intomasterfrom
type_hints
Mar 17, 2026
Merged

Conversation

@petyaslavova
Copy link
Copy Markdown
Collaborator

@petyaslavova petyaslavova commented Mar 6, 2026

Summary

This PR introduces a novel approach to provide precise return type hints for redis-py commands that work with both synchronous and asynchronous clients. Instead of using code generation (unasync) or maintaining duplicate implementations, we leverage Python's @overload decorator with self-type discrimination via Protocol markers.

Key Changes

  • Protocol-based type discrimination: Added SyncClientProtocol and AsyncClientProtocol in redis/typing.py that use a Literal marker attribute (_is_async_client) to distinguish client types at compile time.

  • VectorSet module fully typed: Applied the @overload pattern to all 12 VectorSet commands (vadd, vsim, vdim, vcard, vrem, vemb, vlinks, vinfo, vsetattr, vgetattr, vrandmember, vrange) with proper sync/async return types.

  • Module wrapper pattern: Split VectorSet into _VectorSetBase, VectorSet (sync), and AsyncVectorSet (async) classes, demonstrating the pattern for other Redis modules.

  • Pipeline compatibility: Added _is_async_client markers to all Pipeline classes (multidb, search, cluster) to ensure proper type inference when commands are called from pipelines.

  • Core commands POC: Applied overloads to all commands in core.py

  • Modern type syntax: Converted VectorSet commands to use PEP 604 union syntax (X | None instead of Optional[X]) with from __future__ import annotations.

Benefits

Type checkers (mypy, Pyright) can now correctly infer whether a command returns T (sync) or Awaitable[T] (async) based on the client type, eliminating false positives and improving IDE autocompletion.

As a result of this change, mypy errors in our tests_commands.py (async tests) and test_vsets.py(async only again) decreased to 115 from 526 (all warnings stating that int/bool/etc. is not Awaitable are now gone.)


Note

Low Risk
Primarily adds typing/documentation helpers; runtime changes are limited to new _is_async_client class attributes used for static type discrimination, with low likelihood of affecting behavior.

Overview
Adds an internal implementation guide (.agent/sync_async_type_hints_overload_guide.md) detailing a standardized @overload pattern to distinguish sync vs async command return types.

Introduces a Literal-typed _is_async_client marker on sync and async client classes (including cluster and multidb pipeline variants) to enable self-type discrimination for overload-based typing, and updates imports/__slots__ formatting accordingly.

Written by Cursor Bugbot for commit fd58ba3. This will update automatically on new commits. Configure here.

@jit-ci
Copy link
Copy Markdown

jit-ci Bot commented Mar 6, 2026

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces an @overload + self-type discrimination approach to improve static type checking for APIs shared between sync and asyncio Redis clients, aiming to eliminate Union[Awaitable[Any], Any] return-type ambiguity while keeping a single implementation.

Changes:

  • Add _is_async_client: Literal[...] markers to key sync/async client classes and introduce SyncClientProtocol / AsyncClientProtocol for overload discrimination.
  • Add initial overloads for CoreCommands.get, CoreCommands.set, and VectorSetCommands.vsim, plus an AsyncVectorSet client wrapper and async module accessor.
  • Add spec documents analyzing the approach and cataloging overload targets.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
specs/sync_async_deduplication_analysis.md Adds analysis/spec for sync/async deduplication via overloads.
specs/commands_overload_inventory.md Adds inventory of commands requiring overloads for sync/async typing.
redis/typing.py Introduces sync/async client marker protocols used for overload self-type discrimination.
redis/commands/vectorset/commands.py Adds overloads for vsim() return type based on sync vs async client.
redis/commands/vectorset/init.py Refactors VectorSet client into base + sync/async variants with _is_async_client markers.
redis/commands/redismodules.py Adds async vset() accessor returning AsyncVectorSet.
redis/commands/core.py Adds overloads for get() and set() return types based on sync vs async client.
redis/cluster.py Adds _is_async_client: Literal[False] marker to RedisCluster.
redis/client.py Adds _is_async_client: Literal[False] marker to sync Redis.
redis/asyncio/cluster.py Adds _is_async_client: Literal[True] marker to async RedisCluster.
redis/asyncio/client.py Adds _is_async_client: Literal[True] marker to async Redis.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread redis/commands/core.py Outdated
Comment thread redis/commands/core.py Outdated
Comment thread redis/commands/core.py
Comment thread redis/typing.py
Comment thread redis/commands/vectorset/commands.py Outdated
Comment thread specs/sync_async_deduplication_analysis.md
Comment thread specs/commands_overload_inventory.md
Comment thread redis/commands/redismodules.py
@petyaslavova petyaslavova added breakingchange API or Breaking Change techdebt Things that can be improved or refactored labels Mar 9, 2026
@petyaslavova petyaslavova requested a review from Copilot March 9, 2026 13:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread redis/asyncio/cluster.py
Comment thread redis/commands/core.py Outdated
Comment thread redis/commands/core.py Outdated
@petyaslavova petyaslavova force-pushed the type_hints branch 2 times, most recently from 30c1265 to 02b197f Compare March 9, 2026 14:23
@petyaslavova petyaslavova changed the title Type hints improvements - fixing static code analyses issues related to combined sync and async return types Type hints improvements - fixing static code analyses issues related to combined sync and async return types(vectorsets + base get and set commands) Mar 9, 2026
@petyaslavova petyaslavova marked this pull request as ready for review March 9, 2026 14:38
Comment thread specs/commands_overload_inventory.md
Comment thread redis/commands/vectorset/commands.py
Comment thread redis/commands/core.py
Comment thread specs/sync_async_deduplication_analysis.md
Comment thread redis/commands/core.py
Comment thread redis/commands/core.py Outdated
Comment thread .agent/sync_async_type_hints_overload_guide.md
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

redis/commands/vectorset/init.py:36

  • In _MODULE_CALLBACKS/_RESP2_MODULE_CALLBACKS, the r and ... or None truthiness idiom makes behavior depend on whether the parsed value is falsy. For VINFO_CMD, this could incorrectly turn a valid empty dict into None. Consider using explicit conditional expressions for clarity (e.g., pairs_to_dict(r) if r else None), and if VGETATTR_CMD intentionally treats {} as “no attributes”, make that mapping explicit rather than relying on or None.
        self._MODULE_CALLBACKS = {
            VEMB_CMD: parse_vemb_result,
            VSIM_CMD: parse_vsim_result,
            VGETATTR_CMD: lambda r: r and json.loads(r) or None,
        }

        self._RESP2_MODULE_CALLBACKS = {
            VINFO_CMD: lambda r: r and pairs_to_dict(r) or None,
            VLINKS_CMD: parse_vlinks_result,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread redis/commands/redismodules.py
Comment thread redis/commands/vectorset/commands.py
Comment thread tests/test_asyncio/test_commands.py Outdated
Comment thread .agent/sync_async_type_hints_overload_guide.md Outdated
Comment thread redis/commands/vectorset/commands.py Outdated
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Comment thread redis/commands/vectorset/__init__.py
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Comment thread redis/asyncio/multidb/client.py
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread redis/typing.py
@petyaslavova petyaslavova mentioned this pull request Mar 17, 2026
6 tasks
@petyaslavova petyaslavova merged commit abd08b0 into master Mar 17, 2026
120 of 122 checks passed
@petyaslavova petyaslavova deleted the type_hints branch March 17, 2026 11:59
@Graeme22
Copy link
Copy Markdown

This is a good start. The logical next step would be adding typing for decode_responses. I believe this could be done without any additional overloads, as a generic type var with overloads in the client constructor. Something like:

D = TypeVar("D", str, bytes)

class Redis(Generic[D]):
    @overload
    def __init__(self: "Redis[str]", decode_responses: Literal[True]) -> None: ...

    @overload
    def __init__(self: "Redis[bytes]", decode_responses: Literal[False]) -> None: ...

Now commands could be something like:

class CoreCommands(Generic[D]):
    def get(self, key: str) -> D: ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breakingchange API or Breaking Change techdebt Things that can be improved or refactored

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants