Skip to content
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

Don't check plugin-generated functions #16524

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Nov 20, 2023

There's no source code to fix for such functions, so there's no use checking them.

Fixes #16454.

Related attrs change

Since this caused a false positive for attrs with missing annotations, I've added an explicit check for that. I'm co-opting --disallow-untyped-defs for it.

One regression is that you can no longer use --disallow-incomplete-defs to disallow incomplete but allow wholly untyped classes, e.g. disallow

@attrs.define
class A:
  x: int = attrs.field()
  y = attrs.field()

but allow

@attrs.define
class B:
  x = attrs.field()
  y = attrs.field()

This was somewhat of a hack to begin with (--disallow-*-defs relates to function, not class definitions), so I'm fine with it.

This comment has been minimized.

@ikonst ikonst marked this pull request as ready for review November 20, 2023 04:40

This comment has been minimized.

@ikonst
Copy link
Contributor Author

ikonst commented Nov 20, 2023

👋 @hauntsaninja

mypy/nodes.py Outdated Show resolved Hide resolved

This comment has been minimized.

[case testDisallowIncompleteDefsAttrsNoAnnotations]
# flags: --disallow-incomplete-defs
[case testDisallowUntypedDefsAttrsNoAnnotations]
# flags: --disallow-untyped-defs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to also test with --disallow-any-explicit --disallow-any-decorated, which were used in #16454?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a test for the attrs "regression".

I didn't actually add a regression test for #16454, perhaps I should.

I remember I tried at first, and it was made tricky due --disallow-any-explicit --disallow-any-decorated flagging errors in the typeshed fixtures. We normally exclude errors in the typeshed from mypy's output, and this should apparently be fixed for test fixtures.

Copy link
Member

Choose a reason for hiding this comment

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

The easiest workaround might be to just # type: ignore the fixtures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about 33d06cc?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm I'm a little worried about it (see my comment). If there is no other way, it's fine to omit the test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow.

testDisallowUntypedDefsAttrsNoAnnotations is a "casualty" of this change. I can definitely add --disallow-any-explicit --disallow-any-decorated to the flags in this test and it still passes. I'm just concerned that it'll muddy the test, by confusing the reader about what I'm testing, i.e. it'll be akin to adding --strict-equality to the flags. Yes, it still passes, but it's not what's under test.

Copy link
Contributor

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

@@ -837,6 +837,8 @@ def parse_file(
self.errors.ignored_files.add(path)
tree = parse(source, path, id, self.errors, options=options)
tree._fullname = id
if options.use_builtins_fixtures and id in CORE_BUILTIN_MODULES:
tree._is_typeshed_file = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather not do this if this can silence errors if test fixtures have issues. It would be better to either add a minimal custom fixture that is only used in the relevant test cases or to omit the test cases that need this (I trust that you've tested it already).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_is_typeshed_file does not silence all errors, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The premise of errors in fixtures becoming obscured concerned me too, but having the test environment closer to actual environment seemed more prudent.

If you feel particularly iffy about it, sure, I can add # type:ignore[misc] to a handful of places in the fixtures.

[case testDisallowIncompleteDefsAttrsNoAnnotations]
# flags: --disallow-incomplete-defs
[case testDisallowUntypedDefsAttrsNoAnnotations]
# flags: --disallow-untyped-defs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm I'm a little worried about it (see my comment). If there is no other way, it's fine to omit the test cases.

@ikonst
Copy link
Contributor Author

ikonst commented Jan 4, 2024

@JukkaL another look?

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.

mypy 1.7.0 results in new [misc] type errors with pydantic usage
3 participants