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

Fail-fast on missing builtins #14550

Merged

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Jan 29, 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.

For those reasons, I'm suggesting we add class list: pass and class dict: pass (simplest form without members or type args) to all builtins fixtures. This PR doesn't add specific tests for this, but the assertions in semanal.py should make it painfully obvious.

P.S. I agree with @JelleZijlstra that the end goal is not to need those sparse fixtures. I think having mypy code not "work around" sparse fixtures is a good first step.

@github-actions

This comment has been minimized.

@@ -16,9 +16,7 @@ m.x # E: "object" has no attribute "x"
from typing import Dict
def f(x: Dict[int]) -> None: pass
[out]
main:1: error: Module "typing" has no attribute "Dict"
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you update this test with something more exotic from typing.pyi?

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 can try, but if having dict and list is an invariant now, maybe this test should just go away?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are other things in typing.pyi that probably shouldn't be in the default fixture, e.g. like dataclass_transform. This error is still useful there.

(In general, if we can get away with omitting something from the fixture and having devs hit this error, I think that's much more preferable than having an empty class, where degradation from missing API is less obvious. I'm fine with this PR for list and dict, because they're fundamental enough that their complete absence is annoying)

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 leads me to think that maybe the "Maybe your test fixture does not define ..." message should be triggered by attribute access to a type that was defined in a fixture, not only absence of the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In its current state, this test can be removed, and "builtins.list" and "builtins.dict" can be removed from SUGGESTED_TEST_FIXTURES, since we'd no longer have a "trip wire" for devs.

I do think this trip wire can remain useful if we warn about type missing or type's attributes missing, but
a) I can't tell how noisy that'd be
b) Implementing something like this would require determining whether a TypeInfo comes a fixture (or, perhaps, if its defn comes from a fixture). I can't see anything on either of them that allows me to know it right now. Before investing more work, I'm curious what people think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for updating! And sorry my message was confusing, didn't realise there were several other testXMissingFromStubs tests.

I think logic for b) we could use is just checking if module is typing.pyi or builtins.pyi?

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@hauntsaninja hauntsaninja merged commit f31d162 into python:master Jan 30, 2023
@ikonst ikonst deleted the 2023-01-28-fail-fast-on-missing-builtins branch January 30, 2023 15:36
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.

2 participants