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

Avoid c++ coercions picking up user-set directives #4206

Merged
merged 5 commits into from Jul 20, 2021

Conversation

da-woods
Copy link
Contributor

For example if they're call on entry/exit to a decorated function
they pick up the directives. I don't have a specific bug that this
fixes right now

For example if they're call on entry/exit to a decorated function
they pick up the directives. I don't have a specific bug that this
fixes right now
@da-woods
Copy link
Contributor Author

It breaks a small amount of stuff... This change was mostly to help with something else (which is some way from done) so I'll mark this as draft and come back to it.

@da-woods da-woods marked this pull request as draft May 31, 2021 10:19
@scoder
Copy link
Contributor

scoder commented Jun 29, 2021

I think the idea is good. The problem here is that the recursive conversion of C++ data structures needs access to the directives that impact conversion and interfaces, specifically:

  • binding, always_allow_keywords, allow_none_for_extension_args (if it creates user visible Python functions)
  • auto_pickle (if it creates extension types)
  • ccomplex (if it uses complex arithmetic that is not type specific – not sure if we have that)
  • c_string_type, c_string_encoding (if it converts strings, explicitly or implicitly)
  • optimize.* (probably, since it impacts code size and certain feature usage, but should not change the functionality)

I went through the directive defaults dict and hope that this list is complete. If not, we'll probably notice at some point.

Could you add a helper function that copies only these directives from the current user provided ones, when creating Cython implemented utility code? I guess it would have to make a copy of the defaults and copy the above values over it (regardless if they are needed or not – shouldn't hurt to have them).

@da-woods
Copy link
Contributor Author

c_string_type, c_string_encoding (if it converts strings, explicitly or implicitly)

Yes - this definitely breaks in the current test-suite.

  • binding, always_allow_keywords, allow_none_for_extension_args (if it creates user visible Python functions)

  • auto_pickle (if it creates extension types)

  • ccomplex (if it uses complex arithmetic that is not type specific – not sure if we have that)

Do we currently have auto-conversion functions that create user-visible functions/types/or complex arithmetic? As far as I'm aware the auto-conversion functions are struct <-> dict and vector/map/set/etc <-> list/dict/set/etc. It's probably worth keeping the directives anyway in anticipation of future features?

optimize.* (probably, since it impacts code size and certain feature usage)

I'd argue for not having the utility code depend on these - i.e. in my mind the fact that the utility code is written in Cython is a hidden implementation detail and it's should generate a constant known result (especially since the combinations of them are probably poorly tested...). I suspect neither of the current optimizations really apply to the current conversion code though, so it probably doesn't matter.


But yes, I basically agree - there's a small list of directives to be preserved, and everything else wants setting to some known default. I suspect this applies to the C struct conversion code too.

@scoder
Copy link
Contributor

scoder commented Jun 29, 2021

On the one hand, it seems a good idea to give users control over the generated code and feature set, as long as it does not impact required functionality.

On the other hand, users cannot change directives purely for utility code, independently from their own module code. Can't say if that'll ever be needed, but in any case, there is no way to do that now. And it means that the shared ABI module might contain subtle differences at some point, if we start to include Cython implemented functionality (which we currently don't, I think).

Personally, I think the above list of directives is a reasonable one. I suggest to start with it, and then see where that gets us.

BTW, ccomplex might also be required for conversion and not only for arithmetic. Not sure.

@da-woods da-woods marked this pull request as ready for review July 17, 2021 12:35
Comment on lines 3810 to 3813
from . import Options
directives = Options.get_conversion_utility_code_directives(env.directives)
from .UtilityCode import CythonUtilityCode
env.use_utility_code(CythonUtilityCode.load(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to make the CythonUtilityCode directly responsible for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this. It seems wrong to me for CythonUtilityCode to ignore some of the directives that you've explicitly passed it. I can imagine that leading to a confusing debugging session sometime in the future.

Maybe if the compiler_directives keyword argument was split into explicit_directives (which are always used as-is) and filterable_directives (which pass through this process)? But that seems needlessly complicated.

Copy link
Contributor

@scoder scoder Jul 17, 2021

Choose a reason for hiding this comment

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

Right, that would complicate things.

But when I ask when I would use this filtering, then the sole answer seems to be: "when I want to pass directives into the CythonUtilityCode". So the two appear highly related and the selection very specific for this purpose. Maybe CythonUtilityCode could have a staticmethod filter_directives() to do only the filtering. Simply the length and specificness of the function name suggests to me that it would benefit from namespacing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense (and I've done it). I think I misunderstood what you were suggesting initially.

Comment on lines 179 to 188
def get_conversion_utility_code_directives(directives):
directives_out = dict(_directive_defaults)
allowed_directives = (
'binding', 'always_allow_keywords', 'allow_none_for_extension_args',
'auto_pickle', 'ccomplex',
'c_string_type', 'c_string_encoding')
for k,v in directives.items():
if k in allowed_directives or k.startswith('optimize.'):
directives_out[k] = v
return directives_out
Copy link
Contributor

@scoder scoder Jul 17, 2021

Choose a reason for hiding this comment

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

I would suggest
a) moving the names into a global (list) constant, next to the defaults and scopes, since those three are what we need to adapt together and consistently as part of future changes in the directives.
b) make the list complete, including the spelled out optimize names, to make it more visible that they are part of what is being copied and make them show up in name searches.
c) just iterating over the name list and not the items, since the list to copy is much smaller than the directives dict.

Comment on lines 240 to 241
for ad in allowed_directives:
if ad in directives_in:
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that ad is a word with an entirely unrelated meaning makes this needlessly difficult to read. Furthermore, the generic variable names in this method give little help in explaining the algorithm.

I suggest:
allowed_directives -> inherited_directive_names
ad -> name
directives_in -> current_directives
directives_out -> utility_code_directives

Comment on lines 3809 to 3811
# Override directives that should not be inherited from user code.
# TODO: filter directives with an allow list to keep only those that are safe and relevant.
directives = dict(env.directives, cpp_locals=False)
from .UtilityCode import CythonUtilityCode
directives = CythonUtilityCode.filter_directives(env.directives)
Copy link
Contributor

@scoder scoder Jul 18, 2021

Choose a reason for hiding this comment

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

The Override directives … comment seems ambiguous (override user directives or defaults?), but otherwise helpful, which suggests that the method name itself isn't sufficiently concrete. filter is a fairly broad term that doesn't hint at why or how. filter_inherited_directives(), maybe? Can't think of something better right now.

@scoder scoder merged commit c9a4287 into cython:master Jul 20, 2021
@scoder
Copy link
Contributor

scoder commented Jul 20, 2021

Thanks!

@scoder scoder added this to the 3.0 milestone Jul 20, 2021
@da-woods da-woods deleted the cpp_coercions_env branch July 20, 2021 07:35
0dminnimda added a commit to 0dminnimda/cython that referenced this pull request Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants