Skip to content

fix False negative: Pyrefly does not emit an unpacking error in for k, v in dict #3352 - #3353

Open
asukaminato0721 wants to merge 2 commits into
meta:mainfrom
asukaminato0721:3352
Open

asukaminato0721 wants to merge 2 commits into
meta:mainfrom
asukaminato0721:3352

Conversation

@asukaminato0721

Copy link
Copy Markdown
Collaborator

Summary

Fixes #3352

unpack-length checking rejects string-like unpack sources (str, string literals, LiteralString) with bad-unpacking, which catches for k, v in {"x": 1} because dict iteration yields keys.

Test Plan

update && add test

@meta-cla meta-cla Bot added the cla signed label May 9, 2026
@asukaminato0721
asukaminato0721 marked this pull request as ready for review May 9, 2026 20:38
Copilot AI review requested due to automatic review settings May 9, 2026 20:38
@github-actions github-actions Bot added the size/s label May 9, 2026
@asukaminato0721
asukaminato0721 marked this pull request as draft May 9, 2026 20:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes a false-negative where tuple-unpacking in for loop targets did not report an unpacking error when the iterated element type was string-like (e.g., dict key iteration yielding str), by adding a string-like guard to unpack-length checking and updating/adding regression tests.

Changes:

  • Add is_string_like helper and emit bad-unpacking when unpack-length checking is applied to string-like types.
  • Update existing string-literal unpacking tests to assert the expected error.
  • Add a new regression test covering for k, v in {"x": 1} and for a, b in list[str].

Reviewed changes

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

File Description
pyrefly/lib/alt/solve.rs Add string-like detection and produce BadUnpacking in BindingExpect::UnpackedLength for string-like types.
pyrefly/lib/test/assign.rs Update existing unpack-string tests and add for-loop unpacking regression coverage.

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

Comment thread pyrefly/lib/alt/solve.rs Resolved
@github-actions github-actions Bot added size/m and removed size/s labels May 9, 2026
@asukaminato0721
asukaminato0721 marked this pull request as ready for review May 9, 2026 20:44
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@yangdanny97

Copy link
Copy Markdown
Collaborator

@asukaminato0721

Copy link
Copy Markdown
Collaborator Author
from __future__ import annotations
x: list[ (str, str) ]= []
Expected 1 type argument for `list`, got 2 [[bad-specialization](https://pyrefly.org/en/docs/error-kinds/#bad-specialization)]

@github-actions github-actions Bot added size/m and removed size/m labels May 10, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@asukaminato0721

Copy link
Copy Markdown
Collaborator Author

Looks like the problem with tuple handling.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@codspeed

codspeed Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 35 untouched benchmarks
⏩ 9 skipped benchmarks1


Comparing asukaminato0721:3352 (b01e367) with main (1af3067)

Open in CodSpeed

Footnotes

  1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@stroxler stroxler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! I think this has a number of improvements it needs before we can merge:

Parenthesized generic arguments change meaning

expr.rs:5367–5375

Python treats dict[(str, int)] as dict[str, int]. The new helper instead constructs one tuple-type argument.

Confirmed on the PR head:

  • dict[(str, int)] errors: “Expected 2 type arguments for dict, got 1.”
  • For class Box[T], Box[(int,)] becomes Box[tuple[int]] instead of Box[int].

Fix: Remove the tuple reinterpretation and preserve Ast::unpack_slice semantics. The new fixture should use list[tuple[str, str]]; list[(str, str)] supplies two arguments to list, not one tuple type.

Star-only string unpacking is rejected despite always being valid

solve.rs:2789–2805

def f(s: str) -> None:
    *chars, = s

The PR reports “Cannot unpack str into 0+ values.” This assignment succeeds for every string length, including zero. Both new rejection branches ignore the SizeExpectation::Ge(0) case; LiteralString has the same problem.

Fix: At minimum, exempt Ge(0) and retain ordinary iteration.

Coverage gap

Union members bypass the new string rejection: unpacking elements of list[str | tuple[int, int]] produces no diagnostic. If the broader string-unpacking policy is retained, apply it consistently across branches.

@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

Diff from mypy_primer, showing the effect of this PR on open source code:

============================================================
SUMMARY
============================================================
Total: +15 new errors, -2 fixed errors
By preset: +15/-0 (default), +15/-0 (strict)

Projects with changes (10):
  apprise: +4 -0
  setuptools: +1 -0
  streamlit: +1 -0
  cloud-init: +4 -0
  aiohttp: +1 -0
  pywin32: +1 -0
  werkzeug: +1 -1
  spack: +1 -0
  pylint: +1 -0
  porcupine: +0 -1
============================================================

FULL DIFF DETAILS
------------------------------------------------------------

apprise (https://github.com/caronc/apprise)
+ ERROR apprise/plugins/blink1.py:328:9-16: Cannot unpack str into 3 values [bad-unpacking]
+ ERROR apprise/plugins/xmpp/adapter.py:254:25-38: Cannot unpack str into 2 values [bad-unpacking]
+ ERROR apprise/plugins/xmpp/adapter.py:964:17-30: Cannot unpack str into 2 values [bad-unpacking]
+ ERROR apprise/plugins/xmpp/base.py:374:17-28: Cannot unpack str into 2 values [bad-unpacking]

setuptools (https://github.com/pypa/setuptools)
+ ERROR setuptools/tests/test_build_ext.py:387:11-27: Cannot unpack str into 2 values [bad-unpacking]

streamlit (https://github.com/streamlit/streamlit)
+ ERROR lib/streamlit/runtime/state/query_params.py:917:13-23: Cannot unpack str into 2 values [bad-unpacking]

cloud-init (https://github.com/canonical/cloud-init)
+ ERROR tests/unittests/cmd/devel/test_make_mime.py:40:5-17: Cannot unpack str into 1 value [bad-unpacking]
+ ERROR tests/unittests/test_net.py:2128:17-37: Cannot unpack str into 2 values [bad-unpacking]
+ ERROR tests/unittests/test_net.py:2804:17-37: Cannot unpack str into 2 values [bad-unpacking]
+ ERROR tests/unittests/test_net.py:3053:17-37: Cannot unpack str into 2 values [bad-unpacking]

aiohttp (https://github.com/aio-libs/aiohttp)
+ ERROR aiohttp/cookiejar.py:492:13-25: Cannot unpack str into 2 values [bad-unpacking]

pywin32 (https://github.com/mhammond/pywin32)
+ ERROR win32/Demos/getfilever.py:28:5-19: Cannot unpack str into 2 values [bad-unpacking]

werkzeug (https://github.com/pallets/werkzeug)
- ERROR src/werkzeug/datastructures/headers.py:504:78-104: Unused `# type: ignore` comment [unused-type-ignore]
+ ERROR src/werkzeug/middleware/shared_data.py:115:13-23: Cannot unpack str into 2 values [bad-unpacking]

spack (https://github.com/spack/spack)
+ ERROR lib/spack/spack/spec.py:591:13-47: Cannot unpack str into 3 values [bad-unpacking]

pylint (https://github.com/pycqa/pylint)
+ ERROR pylint/checkers/variables.py:341:9-34: Cannot unpack str into 2 values [bad-unpacking]

porcupine (https://github.com/Akuli/porcupine)
- ERROR porcupine/plugins/directory_tree.py:410:64-78: Unused `# type: ignore` comment [unused-type-ignore]

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

False negative: Pyrefly does not emit an unpacking error in for k, v in dict

5 participants