It is interesting that we're paranoid about the dataclass not having its module imported on only this branch and not the branch above. Not suggesting a change, but a comment could be useful if you know why we only care in theelse branch and not theif not module_name branch.

I was following the checks that were already present in the function. In theelse branch, there was a check forif module:

module=sys.modules.get(cls.__module__)ifmoduleandmodule.__dict__.get(module_name)isa_module:ns=sys.modules.get(a_type.__module__).__dict__

I’d actually consider removing that check — it does seem unnecessary. At least, I can’t think of a case wherecls.__module__ would be missing fromsys.modules. And the tests are still passing.

Yes, I saw the original code. My suggestion would be to hoistmodule = sys.modules.get(cls.__module__) out of theif..else. If you want to preserve the rather paranoid sanity check alongside the hoisted assignment, you could. Something like:

module=sys.modules.get(cls.__module__)ifmodule:# not sure that we need to be this paranoidns=module.__dict__ifmodule_name:...

InitVar lives indataclasses,ClassVar lives intyping. Using an example ofdataclasses.ClassVar is likely to cause confusion.

There is an edge case that is not handled by this:foo.typing_filtered.ClassVar. The regex only accounts for one module level. While it is probably uncommon to importfoo.typing_filtered without aliasing it, it is possible.

I don't want to change the world for a simple bug fix, but it seems like partitioning on. is more robust (and probably more performant) than using the current regex.

While I agree that splitting on a dot would be faster, there are existing tests that cover cases likedataclasses.InitVar.[int] anddataclasses.InitVar+. If we want to support multiple module levels, we should extend the current regex pattern rather than replace it.

I think it's worth creating a new issue to track more levels of module nesting. And I think it's a mistake to test fordataclasses.InitVar.[int] and other invalid strings.

This bug was actually caught by__.typx.ClassVar in the real world. I typically stuff all of my common imports, includingtyping_extensions astypx, into an internals subpackage (__) so that I can dofrom . import __. Reduces module namespace pollution and lets me avoid setting__all__ to define module interfaces. But, I understand that my practice may not be common.

That said, any fix to multi-level traversal here or another PR is going to affect the code that is actually being fixed. Imo, it would make more sense to fix both together holistically (to save developer effort), if there is any interest in actually fixing the multi-level traversal.

I think this could be replaced with:

return (isinstance(a_type_module,types.ModuleType)andis_type_predicate(a_type_module.__dict__.get(type_name),a_module))

It looks like we can indeed replace it. But if we go with this version, thea_type parameter will no longer be used in the function body.

Also, I was thinking — in the original version, this line:

sys.modules.get(a_type.__module__)

could probably be replaced witha_module? Or am I missing something? It seems likea_type might not have been necessary from the start, and we should probably remove it.

If you think the code can be simplified, please do so, then I'll review that version.

I'm just starting to look at this PR, and will have more to say later. For now: I'd prefer this to say something more specific, like<some specific description> of ClassVar not recognized as typing.ClassVar. I'd like this because I'm sure we won't be fixing all instances of using ClassVar as string annotations, and we should mention the exact one being fixed here. Also, I don't want to mention__init__, because that's not the only method affected by this bug.