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

Type ignore comments erroneously marked as unused by dmypy#15043

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

Merged

Conversation

@meshy
Copy link
Contributor

@meshymeshy commentedApr 12, 2023
edited
Loading

There is currently a misbehaviour where "type: ignore" comments are erroneously marked as unused in re-runs of dmypy. There are also cases where errors disappear on the re-run.

As far as I can tell, this only happens in modules which contain an import that we don't know how to type (such as a module which does not exist), and a submodule which is unused.

There was a lot of commenting and investigation on this PR, but I hope that the committed tests and fixes illustrate and address the issue.

Related to#9655

@meshymeshy changed the titleType ignores comments erroneously marked as unused by dmypyType ignore comments erroneously marked as unused by dmypyApr 12, 2023
@AlexWaygoodAlexWaygood added topic-daemondmypy topic-type-ignore# type: ignore comments labelsApr 12, 2023
@chadrik
Copy link
Contributor

I ran into this issue testing a pre-release of 1.3 and it has created enough confusion on the team that I'm not sure we can roll it out until this is fixed. I would love to see this merged before release.

sambvfx reacted with thumbs up emoji

@meshy

This comment was marked as outdated.

@meshy

This comment was marked as outdated.

@meshymeshyforce-pushed theunexpected-unused-type-ignore branch from710c001 to0b84c46CompareMay 18, 2023 08:15
@github-actions

This comment has been minimized.

@meshymeshyforce-pushed theunexpected-unused-type-ignore branch from0b84c46 tofd8b35aCompareJuly 15, 2023 11:21
@meshymeshyforce-pushed theunexpected-unused-type-ignore branch from85b5144 tocca70fcCompareOctober 4, 2023 22:54
@github-actions

This comment has been minimized.

@meshy

This comment was marked as outdated.

@meshy

This comment was marked as outdated.

@meshymeshyforce-pushed theunexpected-unused-type-ignore branch fromb69fe5d to84271b0CompareOctober 22, 2023 17:08
@meshy

This comment was marked as outdated.

@github-actions

This comment has been minimized.

@meshymeshyforce-pushed theunexpected-unused-type-ignore branch 2 times, most recently from244c384 to84271b0CompareOctober 22, 2023 17:55
@github-actions

This comment has been minimized.

@meshy

This comment was marked as outdated.

@JelleZijlstra
Copy link
Member

The quotes shouldn't matter as this is a stub file, which isn't evaluated. The mutually recursive definition of Iterable and Iterator is indeed annoying to deal with, but mypy should be able to handle it.

@meshy

This comment was marked as outdated.

@seddonym
Copy link
Contributor

I'd like to try to help with this issue, and have some time next week.

From what I understand, the PR fixes the underlying issue, but causes some apparently unrelated tests to fail in a surprising way. (The odd behaviour is limited to tests and doesn't happen if we run mypy for real.)

I'm assuming this is a problem with the test machinery, and in particular with test fixtures that attempt to importIterator.

My plan next week is to understand better what's going on by stepping through the test run in a debugger, but if anyone has any inkling as to what might be going on I'd be very grateful for any pointers.@chadrik or@JelleZijlstra I don't suppose you have any idea?

meshy reacted with thumbs up emojimeshy reacted with heart emoji

@seddonym
Copy link
Contributor

seddonym commentedDec 18, 2023
edited
Loading

I've created a simple test case to show what is breaking. This test passes (but it shouldn't).

[case testDemo][file a.py][file a.py.2]# Comment to trigger reprocessing.[builtins fixtures/list.pyi][out]==builtins.pyi:18: error: Name "Iterator" is not definedbuiltins.pyi:18: note: Did you forget to import it from "typing"? (Suggestion: "from typing import Iterator")

This test is simulating the addition of a comment to an otherwise empty file. We are using thelist.pyi fixture. The first time the module is checked (above the==) there are no errors, but the second time (below the==) we get the"Iterator" is not defined error.

I'll continue to try to understand why this is...

@seddonym
Copy link
Contributor

seddonym commentedDec 18, 2023
edited
Loading

Good news - I think I've got somewhere.This WIP commit gets the number of failing tests down to only 3. 🥳

Background

I noticed while debugging that there were some additional errors in the error list following the first check run. These aren't surfaced anywhere obvious, I happened to find these by putting a breakpoint in the final statement ofmypy.dmypy_server.Server.initialize_fine_grained and then drilling down to
self.fine_grained_manager.manager.errors.error_info_map.

Most of the errors were along these lines:

'Method "__getitem__" is not using @override but is overriding a method in class "typing.Sequence"''Method "__iter__" is not using @override but is overriding a method in class "typing.Iterable"'

These errors aren't present when I run it on master.

How I fixed it

I'm not sure if it's the correct thing to do, but I found addingfrom typing_extensions import override and then decorating the methods in question made the tests pass.

@chadrik@JelleZijlstra Do you have any concerns with this as a fix? If so, please advise what would be a better thing to do.

There were a couple of other test cases where instead the fix seemed to be just to add the appropriatebuiltins declaration in the test case.

Remaining failures

The last three are a bit different - I'll continue to look at these tomorrow.

Further context

The reason for the more visibleIterator errors seems to be that the semantic analyzer'sglobals aren't populated with thetyping module when the second pass runs. I am not sure how this relates to the fix in question, I've only got limited understanding of what's going on so far.

@meshy
Copy link
ContributorAuthor

@CoolCat467 I've reproduced a crash on themaster branch using your example. The crash I got wasn't exactly the same, but it's possible that's because I didn't have exactly the same setup as you.

I don't think that the crash I got is related to these changes, though as I said, I didn't get the same one as you.

I've boiled it down to a minimal example, and added details of the crash in#14645 (comment)

CoolCat467 reacted with thumbs up emoji

@meshy
Copy link
ContributorAuthor

@CoolCat467 I'm not able to reproduce your issue with the disappearing messages using the script in that repo.

Can I please ask for detailed instructions on how to reproduce it?

@CoolCat467
Copy link
Contributor

CoolCat467 commentedJan 9, 2024
edited
Loading

@CoolCat467 I'm not able to reproduce your issue with the disappearing messages using the script in that repo.

Can I please ask for detailed instructions on how to reproduce it?

I have now updatedhttps://github.com/CoolCat467/NEAT-Template-Python/dmypy_test.sh to use absolute paths and I get the key error reliably. I would be happy to help if you need more details!

Edit: I think the important detail was the fact I am telling dmypy absolute paths, because my use case of dmypy is as a code editor extension (idlemypyextension), where it might be quite important to tell the mypy daemon exactly which file we want it to check, because potentially the user changes projects without restarting the daemon and they might happen to have files named exactly the same way or whatnot.

@meshy
Copy link
ContributorAuthor

@CoolCat467 Thank you for the clarification. I've got that working now, and can reproduce the dropped errors (on this branch and master). I'll try to turn this into a minimal test case.

Curiously, I'm now not seeing the crash on the third command, but I'll come back to that later.

@meshy
Copy link
ContributorAuthor

meshy commentedJan 10, 2024
edited
Loading

The best minimal example I have so far:

pyproject.toml:

[build-system]requires = ["setuptools >= 64"]build-backend ="setuptools.build_meta"[project]name ="dmypy_example"version ="0.0.1"dependencies = []

src/foo.py (see notes at end):

a:str=1

Commands:

# Create a new Python 3.12 virtualenv, then:pip install --editable.dmypy run --export-types$(pwd)/src/foo.pydmypy run --export-types$(pwd)/src/foo.py

Results:

$pip install --editable....$dmypy run --export-types$(pwd)/src/foo.pyDaemon startedsrc/foo.py:1: error: Incompatible types in assignment (expression has type "int", variable has type "str")  [assignment]Found 1 error in 1 file (checked 1 source file)$dmypy run --export-types$(pwd)/src/foo.pySuccess: no issues found in 1 source file

Notes:

  • If I use a name other thansrc this doesn't seem to work. This is so odd that I feel like I must be doing something wrong.
  • This doesn't happen if we don'tpip install.
  • It doesn't matter what the error infoo.py is. All errors seem to disappear in the second run.
  • This happens on themaster branch of Mypy too.

My conclusion:

It's disappointing that this branch doesn't fix this error, but I don't think that it'scaused by this branch, so perhaps we should consider this a new bug for handling separately?

@CoolCat467
Copy link
Contributor

It's disappointing that this branch doesn't fix this error, but I don't think that it's caused by this branch, so perhaps we should consider this a new bug for handling separately?

Yea, I think it would make sense to make a new issue for this in particular.

If I use a name other thansrc this doesn't seem to work. This is so odd that I feel like I must be doing something wrong.

I think this might be due to the project structure pip expects, but not completely sure

Also side note, thank you for showing the use ofpwd, I wasn't aware that was a thing, that's super helpful instead of using python to figure out the path!

@meshy
Copy link
ContributorAuthor

If I use a name other thansrc this doesn't seem to work. This is so odd that I feel like I must be doing something wrong.

I think this might be due to the project structure pip expects, but not completely sure

As far as I know,src is nothing to do withpip, it's a convention for structuring code that has been gaining momentum in the last few years. (Not a good one, in my opinion, but that's off topic.)

thank you for showing the use ofpwd

I'm glad you found it useful! 🙂

It's disappointing that this branch doesn't fix this error, but I don't think that it's caused by this branch, so perhaps we should consider this a new bug for handling separately?

Yea, I think it would make sense to make a new issue for this in particular.

Thanks! I've opened a new issue so we can track it there.#16768

The problem this solved has been addressed in another way, so we don'tneed to do this re-analysis any more in order to fix the bug we've beenchasing.This change is a partial revert of "Fix disappearing errors whenre-running dmypy check" from a few commits back.
@github-actions
Copy link
Contributor

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

@meshy
Copy link
ContributorAuthor

Hi all! It's been a while since I've had a look at this (I got a bit overwhelmed with it, TBH).

I notice that this was considered for release in 1.9, but wasn't merged because"while the PR itself is apparently ok, it doesn't actually fix the issue completely that its trying to address".

Am I right in saying thatthe issue discovered in this comment is the blocker? Before I go about investigating that, are there any other blockers that we're aware of?

CoolCat467 reacted with heart emoji

@meshy
Copy link
ContributorAuthor

@jhance I wonder if you're able to help me answerthe question above, so that I might be able to continue on this?

@github-actions
Copy link
Contributor

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

@meshymeshy mentioned this pull requestApr 28, 2025
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

CoolCat467 reacted with hooray emoji

@ilevkivskyiilevkivskyi merged commit0755a61 intopython:masterJun 21, 2025
19 checks passed
@ilevkivskyiilevkivskyi mentioned this pull requestJun 21, 2025
@meshy
Copy link
ContributorAuthor

OMG you merged it! I love you!

CoolCat467, darkdreamingdan, and UnknownPlatypus reacted with hooray emoji

esarp pushed a commit that referenced this pull requestJul 3, 2025
There is currently a misbehaviour where "type: ignore" comments areerroneously marked as unused in re-runs of dmypy. There are also caseswhere errors disappear on the re-run.As far as I can tell, this only happens in modules which contain animport that we don't know how to type (such as a module which does notexist), and a submodule which is unused.There was a lot of commenting and investigation on this PR, but I hopethat the committed tests and fixes illustrate and address the issue.Related to#9655---------Co-authored-by: David Seddon <david@seddonym.me>Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ilevkivskyiilevkivskyiilevkivskyi approved these changes

+1 more reviewer

@seddonymseddonymseddonym left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

topic-daemondmypytopic-type-ignore# type: ignore commentsupnext

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@meshy@chadrik@JelleZijlstra@seddonym@tmke8@CoolCat467@ilevkivskyi@hauntsaninja@AlexWaygood

[8]ページ先頭

©2009-2025 Movatter.jp