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

Non-namespaced identifiers #46

Open
encukou opened this issue Jun 6, 2023 · 16 comments
Open

Non-namespaced identifiers #46

encukou opened this issue Jun 6, 2023 · 16 comments
Labels
evolution-proposed fixable theme: implementation flaws problems with the implementation of specific APIs

Comments

@encukou
Copy link
Contributor

encukou commented Jun 6, 2023

The headers define a number of identifiers that don't start with Py (or _Py), potentially clashing with other code.

Relatedly, header names should probably also be namespaced (or hidden in a subdirectory, expecially if users aren't supposed to include them directly)

@gvanrossum
Copy link

All of this can be solved incrementally, right? Just slap a PY_ prefix on various things? Agreed that if we're designing a new API we should take care of this. Honestly my memory is fuzzy on why we used the Py prefix for typedefs but not for struct names, for example. (I can see why originally we used it only for functions, since linker namespaces are much more global than included files.)

@iritkatriel iritkatriel added fixable theme: implementation flaws problems with the implementation of specific APIs labels Jun 7, 2023
@encukou
Copy link
Contributor Author

encukou commented Jun 7, 2023

I don't think we should rename struct _object, it's not a problem in practice. Any new API (or subset) should not include it of course.

Automation to catch mistakes might help.

@encukou
Copy link
Contributor Author

encukou commented Jun 7, 2023

(Somewhat related: Qt infamously breaks Python API by defining the macro slots, which we use for a struct member. Not really our problem to fix, but, once we allow anonymous unions in headers we can fix even unfortunate member names incrementally.)

@vstinner
Copy link
Contributor

vstinner commented Jun 8, 2023

I don't think we should rename struct _object, it's not a problem in practice

I was always annoyed that the structure is not simply called PyObject as the type.

Why is the structure of the PyInterpreterState called... _is? Who has to save some letters for this name?

In practice, the structure name are never used. So we can just fix them, no?

@vstinner
Copy link
Contributor

vstinner commented Jun 8, 2023

configure cruft like HAVE_STRFTIME or SIZEOF_SIZE_T

That's a hard problem since most names are generated by configure, and changing configure is not easy.

Maybe Python.h should not include pyconfig.h, but a new header which only exposes Py_ names needed to declare the C API. Or pyconfig.h can be fixed, and a new "full header" can be moved to the internal C API.

@vstinner
Copy link
Contributor

vstinner commented Jun 8, 2023

slot function typedefs like destructor or setter

Maybe we can introduce new Py_ names and deprecate the old ones. Removing them right now would break many C extensions.

For example, it's common to have to cast a function to (PyCFunction) to ignore compiler warnings about the number of arguments. I recently added _PyCFunction_CAST() to handle that. Maybe this cast should be made public. I don't know. This cast can cause some legit bugs break CFI checks, when a function has only 1 argument, whereas the ABI requires 2 arguments. So it should be used carefully (I tried to implement that in _PyCFunction_CAST()).

@erlend-aasland
Copy link

configure cruft like HAVE_STRFTIME or SIZEOF_SIZE_T

That's a hard problem since most names are generated by configure, and changing configure is not easy.

We can easily define autoconf macros that wrap AC_DEFINE and AH_TEMPLATE and use those throughout .ac file. I'd be tempted to call it low-hanging fruit :)

@gvanrossum
Copy link

@erlend-aasland

We can easily define autoconf macros that wrap AC_DEFINE and AH_TEMPLATE and use those throughout .ac file. I'd be tempted to call it low-hanging fruit :)

So maybe just create a cpython issue + PR for this (linking back here)?

@erlend-aasland
Copy link

erlend-aasland commented Jun 15, 2023

So maybe just create a cpython issue + PR for this (linking back here)?

Thanks for the ping. Yes, it's on my list; I just haven't found the time to do it yet. I'll create the issue right away, though.

UPDATE: I created python/cpython#105813

@vstinner
Copy link
Contributor

See my comment #43 (comment) about E_EOF constant of errcode.h which is used by public PyRun_InteractiveOneObject() function: the doc suggests including <errcode.h> explicitly.

@encukou
Copy link
Contributor Author

encukou commented Oct 23, 2023

Proposed guideline issue: capi-workgroup/api-evolution#21

@ronaldoussoren
Copy link

configure cruft like HAVE_STRFTIME or SIZEOF_SIZE_T

That's a hard problem since most names are generated by configure, and changing configure is not easy.

We can easily define autoconf macros that wrap AC_DEFINE and AH_TEMPLATE and use those throughout .ac file. I'd be tempted to call it low-hanging fruit :)

Having pyconfig.h as part of the public API is questionable in the first place (both as a header and through sysconfig). Adding and removing definitions from pyconfig.h is technically an API change and that means that adding a new configure check in a bug fix release can be dodgy.

@vstinner
Copy link
Contributor

vstinner commented Dec 8, 2023

Having pyconfig.h as part of the public API is questionable in the first place (both as a header and through sysconfig).

The problem is that Python.h requires these HAVE macros.

Adding and removing definitions from pyconfig.h is technically an API change and that means that adding a new configure check in a bug fix release can be dodgy.

I removed #include <unistd.h> from Python 3.13 in alpha1. It has a surprising and unexpected effect: _POSIX_SEMAPHORES and _POSIX_THREADS macros were no longer defined, and so Python picked a different thread/lock/semaphore implementation. It might be tricky to detect such issue if the code handles both cases, macro defined and not defined. You may not detect that your code uses a less efficient implementation.

I'm fine with adding Py_HAVE_xxx macros right now.

But to remove HAVE_xxx macros, I think that we need a smooth migration path. Something like capi-workgroup/api-evolution#24 is a nice path forward:

  • Let users to opt-in for the new clean API without HAVE_xxx macros
  • Don't affect users who didn't opt-in for the new clean API.
  • When enough users migrated, start deprecating the old API.
  • At some point, remove the old API: introduce the incompatible change, remove HAVE_xxx macros.

The difference is to give users the ability to decide when and how they are affected by these changes.

Previously, I pushed changes to affect everybody, and sometimes it went bad, it affected too many users and pushed against it and ask for revert. The reason for revert is basically that "too many changes" occurred at the same time, and that they were busy with other duties.

@ronaldoussoren
Copy link

But to remove HAVE_xxx macros, I think that we need a smooth migration path. Something like capi-workgroup/api-evolution#24 is a nice path forward:

I agree with the need for a clean migration path, but this repo is for listing problems and not solutions 😉. There's bound to be extensions that break when we'd remove or rename the macro's in pyconfig.h.

I'm fine with adding Py_HAVE_xxx macros right now.

Only if we decide that this is the right long term solution. It might be better to switch to private or unstable names (_Py_HAVE_xxx or PyUnstable_HAVE_xxx), and split pyconfig.h in two headers: one for the public API and one for macro's only used during CPython's build.

Not for here, but the same issue is present in sysconfig.get_config_var: that's an inherently unstable API with a badly defined interface. We've had bug reports from folks using Makefile variables that were never meant to be used outside of the build tree.

@vstinner
Copy link
Contributor

vstinner commented Dec 8, 2023

t this repo is for listing problems and not solutions

This project was created to write PEP 733 – An Evaluation of Python’s Public C API. The PEP is now written. Maybe it's time to convert these "problems" into solutions, and close these problems issues?

@encukou
Copy link
Contributor Author

encukou commented Dec 12, 2023

I wouldn't close until there's at least a plan to solve them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evolution-proposed fixable theme: implementation flaws problems with the implementation of specific APIs
Projects
None yet
Development

No branches or pull requests

6 participants