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

Late Include Behavior breaks existing code #1997

Open
fried opened this Issue Nov 13, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@fried

fried commented Nov 13, 2017

See my comments on #1896

Macros defined in some header files used by cython can break late included files.

This behavior should never have been automatic, as it is new behavior and very unexpected.

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Nov 14, 2017

Contributor

Macros defined in some header files used by cython can break late included files.

...can break other included files. This has little to do with early/late includes.

Contributor

jdemeyer commented Nov 14, 2017

Macros defined in some header files used by cython can break late included files.

...can break other included files. This has little to do with early/late includes.

facebook-github-bot added a commit to facebook/fbthrift that referenced this issue Nov 14, 2017

Fix extern statements for Cython 0.28a
Summary:
Cython 0.28a introduces a late include behavior that is broken
see: cython/cython#1997

To bypass this behavior for now we can include "<>" characters that
make the include exempt for late includes.

Reviewed By: yfeldblum

Differential Revision: D6318277

fbshipit-source-id: dd814dd0d79e0cb1991c5c3046a190d04499af2f
@robertwb

This comment has been minimized.

Show comment
Hide comment
@robertwb

robertwb Nov 14, 2017

Contributor

If I understand correctly, the issue is that there are conflicts in your header files (and possibly Python's own header files) that were not exposed until the order of includes was rearranged.

Contributor

robertwb commented Nov 14, 2017

If I understand correctly, the issue is that there are conflicts in your header files (and possibly Python's own header files) that were not exposed until the order of includes was rearranged.

@fried

This comment has been minimized.

Show comment
Hide comment
@fried

fried Nov 14, 2017

structmember.h (python) included by cython generated code defines T_BOOL as a macro.

Normally this was included after our header files. So it wasn't an issue. But T_BOOL T_STRING define by that include are commonly used by apache:thrift as identifiers.

fried commented Nov 14, 2017

structmember.h (python) included by cython generated code defines T_BOOL as a macro.

Normally this was included after our header files. So it wasn't an issue. But T_BOOL T_STRING define by that include are commonly used by apache:thrift as identifiers.

@fried

This comment has been minimized.

Show comment
Hide comment
@fried

fried Nov 14, 2017

My argument is the new behavior can not be automatic, its dangerous to existing code.
That should matter more than a theoretical use case in the future using the new behavior.

fried commented Nov 14, 2017

My argument is the new behavior can not be automatic, its dangerous to existing code.
That should matter more than a theoretical use case in the future using the new behavior.

@fried

This comment has been minimized.

Show comment
Hide comment
@fried

fried Nov 14, 2017

We can work around by adding <> around our includes but, why new behavior should have to include special syntax, not expected existing behavior

Also adding <> has different meaning to the per-processor but in our case it still works.

fried commented Nov 14, 2017

We can work around by adding <> around our includes but, why new behavior should have to include special syntax, not expected existing behavior

Also adding <> has different meaning to the per-processor but in our case it still works.

@robertwb

This comment has been minimized.

Show comment
Hide comment
@robertwb

robertwb Nov 14, 2017

Contributor

This behavior would also manifest had we needed to move the include of structmember.h before any cdef extern from includes. It happened to be triggered by the late import change, but using overlapping symbols and relying on the relative import order of Python headers is solidly in undefined behavior territory.

On a practical note, this seems to affect very few (one?) projects, and you have an easy workaround.

Contributor

robertwb commented Nov 14, 2017

This behavior would also manifest had we needed to move the include of structmember.h before any cdef extern from includes. It happened to be triggered by the late import change, but using overlapping symbols and relying on the relative import order of Python headers is solidly in undefined behavior territory.

On a practical note, this seems to affect very few (one?) projects, and you have an easy workaround.

@fried

This comment has been minimized.

Show comment
Hide comment
@fried

fried Nov 14, 2017

That we know of, there were several internal projects. This is a change made to master so not many people have run it yet.

fried commented Nov 14, 2017

That we know of, there were several internal projects. This is a change made to master so not many people have run it yet.

@fried

This comment has been minimized.

Show comment
Hide comment
@fried

fried Nov 14, 2017

Includes are not stateless in C/C++ its a problem old as dirt, which is why people place standard includes before project specific includes. Its a known behavior that include ordering is important. Even in cpython they have this (https://github.com/python/cpython/blob/master/Python/ast.c#L587-L590) problem.

In our case its not overlapping symbols, one is symbol and one is a macro. If ordering is maintained its completely fine, like the cpython example above.

If this behavior makes it into the release for 0.28 then it should be gated somehow. Right now instead of requiring people to use a new syntax to get this behavior you are requiring "new syntax" to opt out of said behavior. Seems we need way more control of ordering than "automatic"

fried commented Nov 14, 2017

Includes are not stateless in C/C++ its a problem old as dirt, which is why people place standard includes before project specific includes. Its a known behavior that include ordering is important. Even in cpython they have this (https://github.com/python/cpython/blob/master/Python/ast.c#L587-L590) problem.

In our case its not overlapping symbols, one is symbol and one is a macro. If ordering is maintained its completely fine, like the cpython example above.

If this behavior makes it into the release for 0.28 then it should be gated somehow. Right now instead of requiring people to use a new syntax to get this behavior you are requiring "new syntax" to opt out of said behavior. Seems we need way more control of ordering than "automatic"

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Nov 16, 2017

Contributor

See #2006 for a solution to the concrete problem of <structmember.h>

Contributor

jdemeyer commented Nov 16, 2017

See #2006 for a solution to the concrete problem of <structmember.h>

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Nov 17, 2017

Contributor

On a practical note, this seems to affect very few (one?) projects, and you have an easy workaround.

We shouldn't forget that this feature was implemented to serve one specific project in the first place. It feels like the wrong trade-off to me to require existing code to be updated just for that.

Contributor

scoder commented Nov 17, 2017

On a practical note, this seems to affect very few (one?) projects, and you have an easy workaround.

We shouldn't forget that this feature was implemented to serve one specific project in the first place. It feels like the wrong trade-off to me to require existing code to be updated just for that.

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Nov 18, 2017

Contributor

We shouldn't forget that this feature was implemented to serve one specific project in the first place. It feels like the wrong trade-off to me to require existing code to be updated just for that.

I hope that you won't revert the "late include" feature. It's hard to tell how much it will break existing code. As one data point: within all of SageMath and its dependencies (several of which use Cython), one fix was needed.

One solution would be to make this behaviour non-default, either with a compiler directive or explicit syntax (as I originally proposed in #1650).

Contributor

jdemeyer commented Nov 18, 2017

We shouldn't forget that this feature was implemented to serve one specific project in the first place. It feels like the wrong trade-off to me to require existing code to be updated just for that.

I hope that you won't revert the "late include" feature. It's hard to tell how much it will break existing code. As one data point: within all of SageMath and its dependencies (several of which use Cython), one fix was needed.

One solution would be to make this behaviour non-default, either with a compiler directive or explicit syntax (as I originally proposed in #1650).

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Nov 22, 2017

Contributor

Ok, this needs a decision, @robertwb.

My personal feeling is that we shouldn't require existing code to adapt to this. It can already be difficult in some unfortunate cases to bring C includes into a non-conflicting and/or dependency enforced order, even without having Cython reorder them at will. This makes the situation worse, while providing no benefit for majority of users.

It also feels like the situation changes a bit with the new verbatim C code feature, but it's unclear if it's for better or worse, as the feature hasn't seen any real-world usage yet. From the surface, the interaction might have both benefits and uncertainties.

Overall, I would prefer if we could make this an opt-in of some sort, or really make the late includes explicit, as jdemeyer originally proposed.

Contributor

scoder commented Nov 22, 2017

Ok, this needs a decision, @robertwb.

My personal feeling is that we shouldn't require existing code to adapt to this. It can already be difficult in some unfortunate cases to bring C includes into a non-conflicting and/or dependency enforced order, even without having Cython reorder them at will. This makes the situation worse, while providing no benefit for majority of users.

It also feels like the situation changes a bit with the new verbatim C code feature, but it's unclear if it's for better or worse, as the feature hasn't seen any real-world usage yet. From the surface, the interaction might have both benefits and uncertainties.

Overall, I would prefer if we could make this an opt-in of some sort, or really make the late includes explicit, as jdemeyer originally proposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment