-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix error code precedence to ensure disable overrides enable #20356
Conversation
b2f5bd0 to
4d61b4b
Compare
This comment has been minimized.
This comment has been minimized.
11289db to
4a58c92
Compare
This comment has been minimized.
This comment has been minimized.
857dedf to
b67afb5
Compare
This comment has been minimized.
This comment has been minimized.
This commit introduces a unit test to document a bug in the configuration merging 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 a regression test and documenting the bug until a non-breaking fix can be merged. Relates to Issue python#20348
e387770 to
1e0bf46
Compare
for more information, see https://pre-commit.ci
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
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 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. |
Add failing test for per-module error code precedence
Contribution Summary
This Pull Request does not contain a functional fix. It contributes a failing regression test to clearly document a configuration precedence bug in Mypy's options merging logic.
Problem Documented
The added test demonstrates a bug in
Options.apply_changes(used for merging per-module or inline configuration overrides):disable_error_codesetting at the module level is incorrectly overridden by an inherited or defaultenable_error_codesetting.Why No Functional Fix is Included
The functional fix for this issue conflicted with established behavior and caused multiple existing CI tests (e.g., in
testglob.testandtest-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 (where
enableoverridesdisableinOptions.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 will pass only once the correct, non-breaking solution is applied.
Relates to Issue #20348