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

Limit, document, test and deprecate default_value_type inference for 'Any' #1532

Merged
merged 4 commits into from
Sep 20, 2021

Conversation

mdickinson
Copy link
Member

The Any trait type currently goes through the normal inference for default_value_type. The only cases where this inference is likely to produce something other than DefaultValue.constant are traits of the form Any(some_list) or Any(some_dict). In those cases, we end up with DefaultValue.list_copy and DefaultValue.dict_copy respectively. This means that the default is copied for each HasTraits instance, so we end up with a per-instance non-shared default instead of a per-class shared default value.

This PR moves the inference from the TraitType base class to Any.__init__, and deprecates the use of the DefaultValue.list_copy and DefaultValue.dict_copy default value types. The intent is to use DefaultValue.constant consistently in Traits >= 7.0.

Rationale for the deprecation: the inference is already inconsistent in that it special-cases list and dict instances but not set instances. We could extend to set (at some cost - we'd need a new DefaultValue enum member and extra handling in ctraits.c), but that misses other mutable types, and just as with default values for Python functions, there's no realistic way that we can determine when a default value should be copied and when it shouldn't. The only sensible behaviour for Traits here is to follow Python's lead and not make copies when not explicitly asked to. If there are worries that this is a common failure mode, we could look into static analysis checking as a solution.

I suspect that patterns of the form Any(some_list) or Any(some_dict) are rather rare in real code; nevertheless, it seems prudent to have a deprecation period and only make the change in Traits 7.0.

I did a search of Traits-using codebases available to me (both public and private) for the regex pattern "= Any\(". After discarding one false positive, I was left with 878 hits, most of which were simply Any(). The rest included:

  • 2 occurrences of Any( [1, 2, 3 ] ) or Any([1, 2, 3]) in the traits documentation.
  • 2 cases of Any( [] ), in blockcanvas
  • 23 occurrences of Any( {} ) or Any({}), split between TraitsUI and blockcanvas, and 6 of Any(dict()), all in blockcanvas and codetools
  • no other cases that would be affected by this change.

The TraitsUI cases should be looked at; the rest are in outdated code and unlikely to be problematic.

@mdickinson mdickinson changed the title Limit and deprecate default_value_type inference for 'Any' Limit, document, test and deprecate default_value_type inference for 'Any' Sep 15, 2021
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM with one nitpicky comment

traits/trait_types.py Outdated Show resolved Hide resolved
traits/trait_types.py Outdated Show resolved Hide resolved
Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
@mdickinson mdickinson merged commit 243bee6 into main Sep 20, 2021
@mdickinson mdickinson deleted the cleanup/any-default-value-type branch September 20, 2021 14:18
@mdickinson
Copy link
Member Author

  • 2 occurrences of Any( [1, 2, 3 ] ) or Any([1, 2, 3]) in the traits documentation.

Fix in #1547

23 occurrences of Any( {} ) or Any({}), split between TraitsUI and blockcanvas

The TraitsUI cases were fixed in enthought/traitsui#1752

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