Skip to content

[refactor] Remove code that "disables deprecation warnings about \u" #19606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wyattscarpenter
Copy link
Contributor

I believe this code is now useless: it doesn't seem to do anything as far as I can tell.

The real puzzler is that I can't figure out when \u would have triggered a DeprecationWarning at all. And I tried a couple of things, and checked a lot of versions of the documentation. I even downloaded a couple old python versions, including Python 2.7, to try to trigger one with their versions of parse, and got nothing. But perhaps there's some combination I didn't think of.

This is essentially a revert of #7635 aka bd00106, although not 100% exactly.

I do not believe this changes any behavior, and therefore did not write a test. I wish the original PRer had written a test so I knew what I was looking for!

Probably now useless: it doesn't seem to do anything anymore as far as I can tell. And I checked a couple of things, and old documentation.

This is essentially a revert of python#7635 aka python@bd00106, although not 100% exactly.
@wyattscarpenter wyattscarpenter marked this pull request as ready for review August 7, 2025 11:21
Copy link
Contributor

github-actions bot commented Aug 7, 2025

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@wyattscarpenter wyattscarpenter marked this pull request as draft August 7, 2025 11:39
@wyattscarpenter
Copy link
Contributor Author

Oh cool, there actually is a difference. It's a little bit inscrutable to me, and hard to determine what it should actually do, but at least it's something!

@wyattscarpenter
Copy link
Contributor Author

wyattscarpenter commented Aug 7, 2025

_____________________________ testEscapesInStrings _____________________________
[gw0] linux -- Python 3.9.23 /home/runner/work/mypy/mypy/.tox/py/bin/python
data: /home/runner/work/mypy/mypy/test-data/unit/parse.test:113:
Failed: Invalid parser output (/home/runner/work/mypy/mypy/test-data/unit/parse.test, line 113)
----------------------------- Captured stderr call -----------------------------
Expected:
  MypyFile:1( (diff)
    ExpressionStmt:1( (diff)
      StrExpr(\u000d\u000a\u0009/\u123f)) (diff)
    ExpressionStmt:2( (diff)
      BytesExpr(\r\n\t/\\\u123f))) (diff)
Actual:
  main:2: error: Invalid escape sequence \u (diff)

Alignment of first line difference:
  E: MypyFile:1(
  A: main:2: error: Invalid escape sequence \u
     ^
Update the test output using --update-data (implies -n0; you can additionally use the -k selector to update only specific tests)
=========================== short test summary info ============================
FAILED mypy/test/testparse.py::ParserSuite::parse.test::testEscapesInStrings
===== 1 failed, 12365 passed, 322 skipped, 13 xfailed in 396.26s (0:06:36) =====
py: exit 1 (396.95 seconds) /home/runner/work/mypy/mypy> python -m pytest -n 4 pid=3214
  py: FAIL code 1 (397.19=setup[0.24]+cmd[396.95] seconds)
  evaluation failed :( (397.31 seconds)
Error: Process completed with exit code 1.

b'\r\n\t\x2f\u123f'

@wyattscarpenter
Copy link
Contributor Author

wyattscarpenter commented Aug 7, 2025

  1. b'\u' does actually become b'\\u' at runtime, so it's good that the parser was representing that.
  2. b'\u' raises a SyntaxWarning in Python, not a DeprecationWarning, so the fact that we ignore it with a DeprecationWarning is puzzling.
  3. Presumably we're escalating that warning to an error somewhere. Maybe in whatever parser actually gets tested...? So maybe that can just be changed.
  4. Both mypy on master and mypy after this change successfully pass a file with b'\u' with no warnings nor errors.

@brianschubert
Copy link
Member

brianschubert commented Aug 7, 2025

Thanks for taking the initiative of looking into this! A few notes to hopefully clarify what's going on:

  • Invalid escape sequences emit a SynaxWarning in Python 3.12+, but emit a DeprecationWarning in earlier versions:

    $ python3 -Wall
    >>> import ast
    
    >>> ast.parse(r"b'\u'", feature_version=(3, 12))
    <unknown>:1: SyntaxWarning: invalid escape sequence '\u'
    <ast.Module object at 0x7cf24a69c790>
    
    >>> ast.parse(r"b'\u'", feature_version=(3, 11))
    <unknown>:1: DeprecationWarning: invalid escape sequence '\u'
    <ast.Module object at 0x7cf24a69c7d0>

    This code was designed to catch those warnings so that invalid escape sequences in the code being type checked don't clutter mypy's output.

    Note that you won't see DeprecationWarnings unless you run Python with warnings enabled (they're silenced by default).

  • When you remove the warning filter, the uncaught DeprecationWarnings get promoted to errors since the test suite runs with -Werror.

  • The reason you see test failures in all Python versions (not just <3.12) is because parse.test runs with python_version = (3, 9). (I'm not sure why that is. Possibly it would make more sense to use the current Python version to get move coverage?)

  • There is actually one problem with this code, which is that it doesn't catch SyntaxWarnings on Python >=3.12:

    $ echo foo.py
    x = b"\u"
    
    $ mypy --cache-dir=/dev/null --python-version=3.11  foo.py
    Success: no issues found in 1 source file
    
    $ mypy --cache-dir=/dev/null --python-version=3.12  foo.py
    foo.py:1: SyntaxWarning: "\u" is an invalid escape sequence. Such sequences will not work in the future. Did you mean "\\u"? A raw string is also an option.
      x = b"\u"
    Success: no issues found in 1 source file

    To fix that, we could update the warning filter to something like this:

    category = SyntaxWarning if feature_version >= 12 else DeprecationWarning
    warnings.filterwarnings("ignore", category=category)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants