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

Replace builtins.function with types.FunctionType #3171

Closed
pkch opened this issue Apr 14, 2017 · 5 comments
Closed

Replace builtins.function with types.FunctionType #3171

pkch opened this issue Apr 14, 2017 · 5 comments
Labels
affects-typeshed Anything that blocks a typeshed change priority-1-normal topic-usability

Comments

@pkch
Copy link
Contributor

pkch commented Apr 14, 2017

Copied from #3107

It seems that with this change, every test needs types.FunctionType in the test stubs (probably because builtins.function was used as a fallback). And that means builtins.pyi (and every custom bultins fixture) would have to import types. (On top of that, we'll also need to define list everywhere since types.pyi uses it.)

Do we want to do some workaround, like manually inject types.FunctionType into the test environment, without importing types? Not that types.pyi is huge, but it does have a generic in it.

@ilevkivskyi

You could just try adding import types, and see what is the performance impact for tests, if it is few percent, then it is OK, but a manual injection would be needed if there will be a significant slow-down in tests.

There's an issue with importing types module everywhere; it breaks a lot of tests due to the presence of TypeVar in it, and therefore the requirement to bring in typing as well. In particular, it breaks incremental tests, tests that use [file builtins.py] (even after I added import types to all fixtures). Also fixing ThirdPass.builtin_type to make it work for types.FunctionType would be quite hacky.

I am now thinking that maybe we should not replace all builtins.function after all. If we want the user to be able to use types.FunctionType, that's ok, but perhaps internally it's not worth changing?

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Apr 15, 2017

but perhaps internally it's not worth changing?

The main reason for removing builtins.function is because there is no such thing in builtins. Not getting an "Undefined name" error for function() or seeing builtins.function in error messages could be quite confusing. If it is possible to fix this, then it would be already OK. But in general, I think using types.FunctionType for fallback instances is more consistent.

@JukkaL
Copy link
Collaborator

JukkaL commented May 20, 2019

types.FunctionType is not a good replacement since the type function does not directly map to any concrete type, as discussed in #5958. builtins.function is clearly problematic, but it's not clear what the replacement should be.

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 31, 2022

Closing as a duplicate of #3171 (it's a newer issue, but discusses the topic in a more general fashion, which is useful)

@JelleZijlstra
Copy link
Member

@AlexWaygood this is #3171 :)

@AlexWaygood
Copy link
Member

Thanks, I meant #8240 lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-typeshed Anything that blocks a typeshed change priority-1-normal topic-usability
Projects
None yet
Development

No branches or pull requests

5 participants