Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix error code precedence to ensure disable overrides enable#20356

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
franzengel04 wants to merge3 commits intopython:master
base:master
Choose a base branch
Loading
fromfranzengel04:fix-error-code-precedence

Conversation

@franzengel04
Copy link

@franzengel04franzengel04 commentedDec 3, 2025
edited
Loading

Add failing test for per-module error code precedence

Contribution Summary

This Pull Request doesnot contain a functional fix. It contributes afailing regression test to clearly document a configuration precedence bug in Mypy's options merging logic.

Problem Documented

The added test demonstrates a bug inOptions.apply_changes (used for merging per-module or inline configuration overrides):

  • An explicitdisable_error_code setting at the module level is incorrectly overridden by an inherited or defaultenable_error_code setting.
  • This violates the principle that the most specific, explicit instruction (the module-level disable) should always take precedence over a less specific setting.

Why No Functional Fix is Included

The functional fix for this issue conflicted with established behavior and caused multiple existing CI tests (e.g., intestglob.test andtest-default-error-codes.test) to fail.

The decision on a comprehensive fix requires reviewing both the per-module merging logic and the global precedence rule (whereenable overridesdisable inOptions.process_error_codes). Submitting a comprehensive fix would be a highly breaking change.

This PR isolates the bug proof, providing a robust test case that willpass only once the correct, non-breaking solution is applied.

Relates to Issue#20348

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

This commit introduces a unit test to document a bug in the configurationmerging logic within Options.apply_changes.The existing implementation allows an inherited or default 'enable_error_code'setting to incorrectly override an explicit module-level 'disable_error_code'setting, violating precedence rules.The test is expected to fail against the current implementation, serving as aregression test and documenting the bug until a non-breaking fix can be merged.Relates to Issuepython#20348
@github-actions

This comment has been minimized.

@wyattscarpenter
Copy link
Contributor

wyattscarpenter commentedDec 3, 2025
edited
Loading

Hey, thanks for contributing! I'm not a maintainer here, but I have occasionally contributed to this project, on the same subsystem even, so I thought I might give you some tips that might help you along. Feel free to disregard them if you want. I also have not spent much time reviewing the code as it stands. (I understand it may be in an intermediate state at time of writing.)

You should mark your PR as draft while you work on it until it's ready for review.

You may even want to do all your development locally until the contribution is perfect, in order to minimize visual noise for the reviewing maintainer later. There are instructions for running the test suite locally in the contribution documentation, and you can squash or rebase in git before you push to GitHub to PR. This also typically will give you a faster iteration time, compared to relying on the mypy GitHub CI especially because you can run specific tests that are relevant to you, which is much faster than running the whole suite.

If you're contributing a test, as the current PR description mentions, you usually should just write a test case in the appropriate .test file. (Sometimes you need to modify the python test framework code directly, but I think probably you won't need to do that for this case.) There is a way to provide an in-line configuration file to these test cases.

If you're adding a expected-fail test to a .test file, you should mark it as xfail. This will still allow the test suite to pass, which is mandatory for getting a PR accepted. I don't know if anyone actually reviews the xfail tests later, even though that's the point of them, so adding an xfail test may be of negligible value. Still, if the problem ever gets fixed later an xfail test will at least alert us about that.

This seems like a bunch of LLM-generated gobbledegook. Theoretically it's fine to contribute to mypy with an LLM, but I find in my experience LLMs are not very good at this.

There's no need to include these# Reverted comments — especially because they do not, at the moment, document any clues that would be useful to a later maintainer trying to fix the bug.

Those comments are in the right area. However, the fact that code-enabling overrides code-disabling is, for better or worse, mypy's correct declared current behavior, and one can't simply just reverse that. The correct fix for this problem is probably along the lines of keeping track of which code enables come from a config file instead of the command line and letting the command line disables override those.

I think you're on the right track and have confidence that with careful effort you can make a great PR. There is no rush.

Hope this helps! Godspeed.

@franzengel04franzengel04 marked this pull request as draftDecember 3, 2025 19:29
@github-actions
Copy link
Contributor

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

@franzengel04
Copy link
Author

franzengel04 commentedDec 12, 2025
edited
Loading

@wyattscarpenter You are immensely helpful, and I really appreciate all your suggestions!

As for this issue, I understand that code-enabling overrides code-disabling. However, what I'm not sure of yet is:Should the "enable overrides disable" rule transcend levels of precedence (e.g., config file vs. command-line flag)?

If not, then I feel that the fix you suggested likely requires a change to the highly-used core class (Options) to ensure command-line arguments and later-processed directivescorrectly override earlier configurations. As a first-time contributor, tackling a refactoring of this mechanism feels like it could be an overly ambitious task.

Because of this, I am currently most interested in adding an xfail test in the appopriate .test file that simply demonstrates this strange behavior (even though I am aware of its lesser immediate value.)

Maybe, you could help me figure out how to properly reproduce this in the .test file as pointed out here by another helpful person! ->#20348 (comment)

Testing locally, I am using this new test case in thetest-data/unit/check-errorcodes.test file, but I don't believe I was able to match it's behavior:

[case testDisableFlagFailsToOverrideDefaultEnable]# flags: --config-file tmp/mypy.ini --disable_error_code possibly-undefined[file mypy.ini]\[mypy]enable_error_code = possibly-undefined[file a.py]x = 1if x:    y = 1x = y # E: Name "y" may be undefined  [possibly-undefined]

Or, if I'm still far off from any of this, I'd appreciate any clarification!

wyattscarpenter reacted with heart emoji

@wyattscarpenter
Copy link
Contributor

wyattscarpenter commentedDec 13, 2025
edited
Loading

I see. Yep, you almost had it; the test format is just a little bit finicky (and also differs slightly between test files!). Here's a fixed version, which I've also minimized and (test-)commented slightly.

[casetestDisableFlagFailsToOverrideDefaultEnable-xfail]# flags: --config-file tmp/mypy.ini --disable-error-code possibly-undefinedif1:y=1x=y--ThiscurrentlygivesanerrorName"y"maybeundefined  [possibly-undefined],butthisisundesired.[filemypy.ini]\[mypy]enable_error_code=possibly-undefined

Regarding the levels of precedence: yes, we desire flags (including config options, etc) on higher levels of precedence to override those on lowers level of precedence, including en/disable-error-codes. The special behavior of that flag family's precedencewithin flags of the the same level of precedence doesn't override that. Totally up to you1 if you want to tackle that or not :)

Footnotes

  1. I mean, in my opinion. I have no deep insight into what the actual maintainers of this project will think; but I imagine they would also be fine with either way.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@franzengel04@wyattscarpenter

[8]ページ先頭

©2009-2025 Movatter.jp