Skip to content

Make sure that __dir__ returns new copies of __all__ #135

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

Merged
merged 2 commits into from
Mar 19, 2025

Conversation

lagru
Copy link
Member

@lagru lagru commented Dec 10, 2024

__dir__() should probably return new copies for each call so that the result will stay the same regardless of how the result of previous calls was modified. That seems to be the standard behavior for Python's default object.__dir__.

E.g.,

import module_with_lazy_loading

assert dir(module_with_lazy_loading) is not dir(module_with_lazy_loading)

x = dir(module_with_lazy_loading)
x.pop()
assert x != dir(module_with_lazy_loading)

should all pass but currently they don't.

__dir__ should probably return new copies for each call so that the
result will stay the same regardless of how the result of previous calls
was modified.
@lagru lagru changed the title Make sure that __dir__ returns a new copies of __all__ Make sure that __dir__ returns new copies of __all__ Dec 10, 2024
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.01%. Comparing base (4e78314) to head (df4f89a).
Report is 17 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
+ Coverage   95.74%   96.01%   +0.27%     
==========================================
  Files           5        5              
  Lines         235      251      +16     
==========================================
+ Hits          225      241      +16     
  Misses         10       10              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Makes sense to me. I will say, from reading the documentation, it surprises me that dir() does not make a copy, since it says it will sort the result.

@@ -90,7 +90,7 @@ def __getattr__(name):
raise AttributeError(f"No {package_name} attribute {name}")

def __dir__():
return __all__
return list(__all__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very minor nit: I prefer to use the copy method over a new constructor. As a reader, it tells me that __all__ is already a list and not being converted.

Suggested change
return list(__all__)
return __all__.copy()

On a more theoretical note (as these lists will generally be small enough to be negligible), using a method over a constructor permits the implementation to skip generic logic to handle multiple input types, so upstream optimizations in Python can potentially have higher impact.

Some timeit logs (py3.13)
In [2]: _dir = dir()

In [4]: %timeit _dir[:]
41.7 ns ± 0.858 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [5]: %timeit list(_dir)
46.5 ns ± 0.816 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [6]: %timeit _dir.copy()
38.3 ns ± 0.667 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [9]: x = list(range(100000))

In [10]: %timeit list(x)
273 μs ± 12.1 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [11]: %timeit x.copy()
243 μs ± 8.06 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [12]: %timeit x[:]
231 μs ± 11.1 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [13]: x = list(range(1000))

In [14]: %timeit list(x)
1.13 μs ± 15 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [15]: %timeit x.copy()
1.13 μs ± 16 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [16]: %timeit x[:]
1.15 μs ± 16.9 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [17]: x = list(range(10))

In [18]: %timeit list(x)
36.3 ns ± 0.642 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [19]: %timeit x.copy()
26.5 ns ± 0.504 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [20]: %timeit x[:]
30.6 ns ± 0.748 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

Copy link
Member Author

@lagru lagru Dec 10, 2024

Choose a reason for hiding this comment

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

Yeah, I was debating that myself but chose to use list() over .copy() to be consistent with

return __getattr__, __dir__, list(__all__)

I'm happy to update both to .copy() if desired. 😊

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, that's fine with me. Wasn't going to suggest it since it's a little out-of-scope, but I don't see the harm in changing it in passing.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mind updating, @lagru, then we can merge this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the copy approach still support the case of __all__ being a tuple?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should, because the __all__ in question is a list generated inside this function.

@effigies effigies added the type: Bug fix Something isn't working label Dec 12, 2024
@@ -90,7 +90,7 @@ def __getattr__(name):
raise AttributeError(f"No {package_name} attribute {name}")

def __dir__():
return __all__
return list(__all__)
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind updating, @lagru, then we can merge this.

@effigies
Copy link
Collaborator

@stefanv Let's merge this since it works, and I can make a quick PR with my alternate proposal that can be approved rejected on its own merits.

@effigies effigies merged commit 22dcd39 into scientific-python:main Mar 19, 2025
29 of 30 checks passed
@lagru lagru deleted the return-copy-in-dir branch March 19, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Bug fix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants