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

Inconsistent behavior w.r.t missing builtins #14547

Open
ikonst opened this issue Jan 28, 2023 · 3 comments
Open

Inconsistent behavior w.r.t missing builtins #14547

ikonst opened this issue Jan 28, 2023 · 3 comments
Labels
bug mypy got something wrong topic-developer Issues relevant to mypy developers topic-tests

Comments

@ikonst
Copy link
Contributor

ikonst commented Jan 28, 2023

Issue

Some of the builtins fixtures are sparse to the point that they're missing even the declaration of dict, list, tuple etc. types. The austerity is justified by improved tests performance, and it's understood that specific tests can specify a broader fixture without slowing down other tests.

However, some core features rely on the presence of those types, e.g.

  • functions' **kwargs
  • modules' __annotations__
  • namedtuple's attributes like _field_types

I've observed 3 different strategies used to handle the types absence:

I think we should recommend and, ideally, consolidate on one strategy.

Motivation

Case in point: Recently while implementing #14526, I had to "play" a whack-a-mole with unrelated tests breaking. I've used **kw which required me to add dict to a number of builtins fixtures, which in turn caused __annotations__ to materialize where they didn't before, and failed some other tests.

I think ideally we shouldn't have this whack-a-mole, and tests should be less brittle.

Recommendation

Which strategy to adopt? While it's good practice to structure code to be more testable, I think it's normally discouraged to have code paths that are only due to testing. Since those builtins should universally exist in "production", I think crash should be our strategy.

Since code typically relies only on the presence of those classes (not their methods), we can put in place a rule (validated by tests) that any builtins fixture must define a baseline of types, even if empty. This shouldn't affect performance greatly, and provide a comfortable baseline.

@ikonst ikonst added the bug mypy got something wrong label Jan 28, 2023
@AlexWaygood AlexWaygood added topic-developer Issues relevant to mypy developers topic-tests labels Jan 28, 2023
@JelleZijlstra
Copy link
Member

The fixtures in general are a confusing pain point for developing mypy. It would be better if we can just use the typeshed stubs in tests.

As I understand it, the reason for using the fixtures is that typeshed's builtins.pyi is big and parsing it takes a long time. Perhaps we can come up with some clever form of caching to alleviate that concern.

@ikonst
Copy link
Contributor Author

ikonst commented Jan 28, 2023

I agree, that'll be a big win for contributors' ergonomics.

As an intermediate step I'd suggest eliminating the fallback strategies from the code, i.e. fail-fast.

hauntsaninja pushed a commit that referenced this issue Jan 30, 2023
As discussed in #14547, some mypy features were degrading rather than
failing-fast when certain built-in types (list, dict) were not present
in the test environment.

- The degraded state (e.g. lack of `__annotations__`) didn't make the
culprit (sparse fixture) obvious, making tests harder to debug.
- Having the code work around quirks of the testing environment ("sparse
fixtures") is an anti-pattern.
@ikonst
Copy link
Contributor Author

ikonst commented Jan 30, 2023

Another case - falling back from types.ModuleType to builtins.object:

mypy/mypy/checkexpr.py

Lines 392 to 398 in 91e8581

try:
result = self.named_type("types.ModuleType")
except KeyError:
# In test cases might 'types' may not be available.
# Fall back to a dummy 'object' type instead to
# avoid a crash.
result = self.named_type("builtins.object")

To exclude this special-case, we'd have to

  • import types in all builtins
  • add class tuple: pass to all builtins

The types fixture is pretty small, so unless I'm suggesting "death by a thousand cuts", I think we should go ahead with this. This will also trigger fixing some tests that are obviously wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-developer Issues relevant to mypy developers topic-tests
Projects
None yet
Development

No branches or pull requests

3 participants