[ty] Consider from thispackage import y to re-export y in __init__.pyi by Gankra · Pull Request #21387 · astral-sh/ruff (original) (raw)

Fixes astral-sh/ty#1487

This one is a true extension of non-standard semantics, and is therefore a certified Hot Take we might conclude is simply a Bad Take (let's see what ecosystem tests say...).

mypy_primer results

Changes were detected when running on open source projects

cki-lib (https://gitlab.com/cki-project/cki-lib)

• cki_lib/timeout.py:23:20: error[unresolved-attribute] Module multiprocessing has no member context
• Found 205 diagnostics

• Found 204 diagnostics

scikit-build-core (https://github.com/scikit-build/scikit-build-core)

• src/scikit_build_core/build/wheel.py:98:20: error[no-matching-overload] No overload of bound method __init__ matches arguments

• Found 43 diagnostics

• Found 44 diagnostics

No memory usage changes detected ✅

Choose a reason for hiding this comment

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

Nice! One less false positive in the ecosystem 😆

I am fine with landing this, but this seems more like an argument that maybe typeshed should be changed here X3

My two cents is that I actually like how explicit/clear from . import x is compared to from thispackage import x, and I think pyright only supports the former so we're in some sense introducing less fragmentation by not shipping this.

This one is a true extension of non-standard semantics

But so is considering from . import y as a stub re-export, right? There doesn't seem to me to be a principled reason to privilege from . import y as a stub re-export but not from thispackage import y. According to the spec, neither should be considered a stub re-export, and they have the same semantics at runtime, so if we're special-casing one of them I think we should special-case the other too.

On the other hand -- yes, that typeshed import is definitely not intended to be an explicit re-export of the context submodule from multiprocessing/__init__.pyi. It's true that you can access .context from the multiprocessing module at runtime, but the multiprocessing.context module is entirely undocumented; people are supposed to use the public API of multiprocessing from the top-level module rather than use its undocumented submodules. It seems to me like it's an implementation detail of the runtime that the submodule is currently exposed from the __init__.py at runtime; I'm not sure users should be depending on that.

(Additional context: this stub includes an __all__ that actually doesn't actually list context)

If __all__ exists in the stub, maybe that should just have the final say over which import definitions in stubs are considered re-exported from the stub?

Would you also want it to be able to mask out from x import y as y? That's a pretty big change...

Would you also want it to be able to mask out from x import y as y? That's a pretty big change...

Hmm... the spec says:

Type checkers can use the following rules to determine which symbols are visible outside of the package.

• Symbols whose names begin with an underscore (but are not dunder names) are considered private.
• Imported symbols are considered private by default. A fixed set of import forms re-export imported symbols.
• A module can expose an __all__ symbol at the module level that provides a list of names that are considered part of the interface. This overrides all other rules above, allowing imported symbols or symbols whose names begin with an underscore to be included in the interface.

The from x import y as y convention is the second bullet point here -- but the third bullet point (regarding __all__ states that "this overrides all other rules above". So that would imply that yeah, if y doesn't appear in __all__, we should regard it as unexported even if it was imported with from x import y as y.

But on the other hand, there's literally no reason to use a redundant alias like that unless you want it re-exported. You can't get any more explicit than that. So I think leaving it as it is there would also be fine, even if it would arguably be most consistent to give __all__ top priority in all cases.

IMO the typing spec is not super clear about the relationship between the import x as x convention and respecting __all__, but personally I think the wording implies that they are additive -- if a symbol uses the x as x convention OR appears in __all__, it is exported. It does say that __all__ "overrides all rules above" but it never suggests that overriding in a negative direction is even considered as a possibility. (The additive version is also what mypy and pyright actually implement. And I think it's the better choice for users, since x as x is so clear and explicit.)

The trouble here is that we are adding some less-explicit re-exports, and we have to decide if we should treat them the same as the explicit x as x, or let them be overridden in a negative direction by __all__. Personally I don't think this is worth the complexity either in implementation or in mental model for users. We should just decide if we want to consider these forms re-exports, and if we do, then we should treat them the same as the fully-explicit re-exports.

The trouble here is that we are adding some less-explicit re-exports, and we have to decide if we should treat them the same as the explicit x as x, or let them be overridden in a negative direction by __all__. Personally I don't think this is worth the complexity either in implementation or in mental model for users. We should just decide if we want to consider these forms re-exports, and if we do, then we should treat them the same as the fully-explicit re-exports.

Yeah, that's fair. In that case, I think I favour landing this PR as-is, despite the fact that the multiprocessing.context thing is arguably an ecosystem regression. I think it makes our heuristic easier to understand if we consider from thispackage import y the same way as we consider from . import y, since they do the same thing at runtime.

The one thing that occurs to me: if we land this and don't let __all__ override it negatively, would there be any way for the stub author to say "no, actually, this submodule is private."?

If the answer is no, that maybe suggests we shouldn't land this? Because the other direction is always an easy fix for the stub author -- just use x as x in the import. (Though I agree that if we don't land this, we should also revert making from . import sub a re-export -- and that one seemed clearly positive in the ecosystem. So I guess I don't think we should revert that, which means I do think we should land this regardless, to maintain consistency.)

The other possible thing we could do is ensure the "initial underscore means private" rule does override this heuristic. That would mean that from thispackage import sub and from . import sub, if they were really meant to not be re-exports, could be rewritten as from thispackage import sub as _sub or from . import sub as _sub (or actually rename the submodule to begin with underscore.) But I think this is low priority, we can wait and see if we get any user requests for it.

I pushed up a docs change to add some more context:

• Unlike from . import x, this was done purely for consistency and not for compatibility
• The only way to opt out is with from . import x as y (this already worked and is tested)

The one thing that occurs to me: if we land this and don't let __all__ override it negatively, would there be any way for the stub author to say "no, actually, this submodule is private."?

I don't think so, no. If you need to import it in __init__.pyi for a type annotation, there'd be no way of excluding it from the public interface. But distinguishing between from . import y and from thispackage import y just seems very arbitrary; I think the only reason why one had more ecosystem impact is just that from thispackage import y is more characters to type out, so it's a less popular idiom in general in __init__.pyi files. It doesn't mean that people are using that syntactic form specifically so that type checkers will recognise the submodule as re-exported. I do think treating them both the same way is the most important thing here: if we want __all__ to be able to override these re-exports negatively, we should apply that to both from . import y and from thispackage import y.

But I think this is low priority, we can wait and see if we get any user requests for it.

Yes, I agree; we can come back to all this later. Nobody's going to complain if the ty beta lets you access .context as an attribute on the stdlib multiprocessing module, in contravention of the intentions of the typeshed maintainers 😄

The only way to opt out is with from . import x as y (this already worked and is tested)

Oh, we still have a nonstandardised syntactic distinction even after this PR? I'm not sure I like that either 😆

Oh to be clear from . import x as y and from thispackage import x as y both opt-out of the re-export. I basically prioritize the x as x check over the self-import check if there is an alias.

I think the exception for from {.,thispackage} import x as y is good. Conceptually with this re-export rule we are modeling automatic submodule attribute attachment, and saying "if you explicitly import a submodule, we'll consider it exported". In the case of x as y, the submodule attribute would be x, not y, but you chose to rename it, so we're certainly not going to consider x as exported, nor y because it doesn't match any other rule. It's only when the x and y line up with the same name that we're going to consider that "strong enough" to be a re-export.

I think the exception for from {.,thispackage} import x as y is good. Conceptually with this re-export rule we are modeling automatic submodule attribute attachment, and saying "if you explicitly import a submodule, we'll consider it exported". In the case of x as y, the submodule attribute would be x, not y, but you chose to rename it, so we're certainly not going to consider x as exported, nor y because it doesn't match any other rule. It's only when the x and y line up with the same name that we're going to consider that "strong enough" to be a re-export.

I guess that makes as much sense as anything else here 😆

Alright merging, have fun explaining your sins to all your typing counsel friends!

Alright merging, have fun explaining your sins to all your typing counsel friends!

😭

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})