FIX: Use covariant Sequence for executemany seq_of_parameters (#572)#586
Open
bewithgaurav wants to merge 2 commits into
Open
FIX: Use covariant Sequence for executemany seq_of_parameters (#572)#586bewithgaurav wants to merge 2 commits into
bewithgaurav wants to merge 2 commits into
Conversation
…oft#572) The 1.6.0 signature seq_of_parameters: Union[List[Sequence[Any]], List[Mapping[str, Any]]] regressed type-checking for callers passing a more precise list type such as list[tuple[str, str, str, str, int, str]]. Because typing.List is invariant, mypy rejects it with: error: Argument 2 to "executemany" of "Cursor" has incompatible type "list[tuple[...]]"; expected "list[Sequence[Any]] | list[Mapping[str, Any]]" [arg-type] note: "list" is invariant -- see ... note: Consider using "Sequence" instead, which is covariant In 1.5.0 the parameter was a single arm (List[Sequence[Any]]) which mypy accepted via the gradual-typing escape hatch on Any; the union introduced in 1.6.0 forces mypy to pick a matching arm and invariance kicks in, breaking previously valid call sites. Switch to the covariant Sequence outer container on both the public Cursor.executemany and the internal _transpose_rowwise_to_columnwise helper: seq_of_parameters: Union[ Sequence[Sequence[Any]], Sequence[Mapping[str, Any]], ] This: - accepts list[tuple[...]], list[list[...]], tuple of tuples, etc.; - matches PEP 249 ("sequence of sequences"); - aligns with pyodbc's typing; - keeps the 1.6.0 dict/pyformat support intact. Runtime behaviour is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses the Cursor.executemany typing regression by changing the outer parameter container from invariant List to covariant Sequence.
Changes:
- Updates
Cursor.executemanyparameter typing to acceptSequence[Sequence[Any]]andSequence[Mapping[str, Any]]. - Updates the internal row-to-column transposition helper signature similarly.
Comments suppressed due to low confidence (2)
mssql_python/cursor.py:2139
- The repository still contains
mssql_python/mssql_python.pyiwith the oldUnion[List[Sequence[Any]], List[Mapping[str, Any]]]signature forCursor.executemany. Leaving that stub stale means consumers or tooling that read the stub can continue seeing the invariantList[...]type and the reported typing regression is not fully fixed.
seq_of_parameters: Union[Sequence[Sequence[Any]], Sequence[Mapping[str, Any]]],
mssql_python/cursor.py:2139
- This typing regression fix is not covered by an automated type-checking test: the current CI mypy step checks only
mssql_python/, and the tests do not include a reproducer that callsexecutemanywith alist[tuple[...]]. Adding a small type-check fixture or equivalent regression check would prevent this public annotation from reverting unnoticed.
seq_of_parameters: Union[Sequence[Sequence[Any]], Sequence[Mapping[str, Any]]],
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.build._deps.simdutf-src.src.haswell.implementation.cpp: 0.4%
mssql_python.pybind.build._deps.simdutf-src.src.implementation.cpp: 6.7%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.implementation.h: 10.4%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.scalar.utf16_to_utf8.utf16_to_utf8.h: 25.3%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.7%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.internal.isadetection.h: 65.3%
mssql_python.row.py: 70.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 74.2%🔗 Quick Links
|
c7d496c to
ba2e287
Compare
Two follow-ups from the Copilot PR review on microsoft#586: 1. mssql_python/mssql_python.pyi still carried the old invariant signature seq_of_parameters: Union[List[Sequence[Any]], List[Mapping[str, Any]]] Sync it to the covariant Sequence form to match cursor.py and prevent a silent regression if the stub is ever relocated to __init__.pyi (where mypy would actually consume it). 2. _transpose_rowwise_to_columnwise is only ever called with already-converted positional rows (pyformat-to-qmark conversion happens upstream in executemany before transposition). Advertising it as accepting Mapping rows is misleading - the transpose loop would yield mapping keys, not values. Narrow its annotation to Sequence[Sequence[Any]] only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ba2e287 to
d41013f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
Fixes the typing regression reported in #572.
In v1.5.0, the second parameter of
Cursor.executemanywas typed as:In v1.6.0 it was widened to also accept
pyformat/ dict-style parameters:That widening is functionally correct, but it surfaced a long-standing typing problem.
typing.Listis invariant in its type parameter, so a perfectly validlist[tuple[str, str, str, str, int, str]]argument no longer matches either arm of theUnion, and mypy rejects previously valid call sites:In v1.5.0 the single-arm
List[Sequence[Any]]slipped through mypy's gradual-typing escape hatch onAny. TheUnionin v1.6.0 forces mypy to commit to one arm, invariance kicks in, and call sites that worked for years break.The fix
Switch the outer container to the covariant
Sequence, on both the publicCursor.executemanyand the internal_transpose_rowwise_to_columnwisehelper:This:
list[tuple[...]],list[list[...]], tuple-of-tuples, generator-backed sequences afterlist(), exactly what the docstring already promises ("Sequence of sequences or mappings of parameters").etc.executemanytakes a sequence of sequences, not a list of sequences.pyodbctypes the same parameter.Verification
Reproducer derived from the issue:
origin/main(before this PR):mypyreports the exact error from Invariant Type on Cursor.executemany #572.Success: no issues found in 1 source file.