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

Preserve implicitly exported types via attribute access #16129

Merged
merged 5 commits into from Sep 19, 2023

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Sep 16, 2023

Resolves #13965. Follow up to #13967. Unblocks #14086

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

LG, but I have some suggestions.

test-data/unit/check-flags.test Outdated Show resolved Hide resolved
e.set_line(mx.context)
e.node = v
return mx.chk.expr_checker.analyze_ref_expr(e, lvalue=mx.is_lvalue)
return ret
Copy link
Member

Choose a reason for hiding this comment

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

Ideally I would put all this logic in checkexpr.py. We should try to start making checkmember.py more self-consistent. I am still hoping to refactor everything to resolve #7724

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, doesn't moving this out of checkmember.py mean we're more likely to not handle hidden attributes elsewhere?

i.e. is the following diff what you meant?

diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py
index f46c8cb15..ac4e29469 100644
--- a/mypy/checkexpr.py
+++ b/mypy/checkexpr.py
@@ -3178,14 +3178,28 @@ class ExpressionChecker(ExpressionVisitor[Type]):
             else:
                 is_self = False
 
+            if (
+                module_symbol_table is not None
+                and e.name in module_symbol_table
+                and not module_symbol_table[e.name].module_public
+            ):
+                # If we wouldn't be able to find the member due to --no-implicit-reexport,
+                # report an error, but try to get the correct type anyway.
+                v = module_symbol_table[e.name].node
+                ref = NameExpr(e.name)
+                ref.set_line(e)
+                ref.node = v
+                self.msg.has_no_attr(original_type, original_type, e.name, e, module_symbol_table)
+                return self.analyze_ref_expr(ref, lvalue=is_lvalue)
+
             member_type = analyze_member_access(
                 e.name,
                 original_type,
-                e,
-                is_lvalue,
-                False,
-                False,
-                self.msg,
+                context=e,
+                is_lvalue=is_lvalue,
+                is_super=False,
+                is_operator=False,
+                msg=self.msg,
                 original_type=original_type,
                 chk=self.chk,
                 in_literal_context=self.is_literal_context(),
diff --git a/mypy/checkmember.py b/mypy/checkmember.py
index 4316a5928..03c8f77ee 100644
--- a/mypy/checkmember.py
+++ b/mypy/checkmember.py
@@ -609,19 +609,13 @@ def analyze_member_var_access(
         mx.msg.undefined_in_superclass(name, mx.context)
         return AnyType(TypeOfAny.from_error)
     else:
-        ret = report_missing_attribute(mx.original_type, itype, name, mx)
-        # Avoid paying double jeopardy if we can't find the member due to --no-implicit-reexport
-        if (
-            mx.module_symbol_table is not None
-            and name in mx.module_symbol_table
-            and not mx.module_symbol_table[name].module_public
-        ):
-            v = mx.module_symbol_table[name].node
-            e = NameExpr(name)
-            e.set_line(mx.context)
-            e.node = v
-            return mx.chk.expr_checker.analyze_ref_expr(e, lvalue=mx.is_lvalue)
-        return ret
+        # We should have handled --no-implicit-reexport cases already
+        assert (
+            mx.module_symbol_table is None
+            or name not in mx.module_symbol_table
+            or mx.module_symbol_table[name].module_public
+        )
+        return report_missing_attribute(mx.original_type, itype, name, mx)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we are using analyze_member_access() in many places, so maybe it is not worth it. You can go ahead as is.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

porcupine (https://github.com/Akuli/porcupine): typechecking got 1.54x slower (20.6s -> 31.7s)
(Performance measurements are based on a single noisy sample)

cwltool (https://github.com/common-workflow-language/cwltool)
+ tests/test_singularity_versions.py: note: In function "set_dummy_check_output":
+ tests/test_singularity_versions.py:24:9: error: Incompatible types in assignment (expression has type "Callable[[Any, Any], str]", variable has type overloaded function)  [assignment]

urllib3 (https://github.com/urllib3/urllib3)
+ src/urllib3/contrib/securetransport.py:761: error: Invalid index type "int" for "dict[_SSLMethod, tuple[int, int]]"; expected type "_SSLMethod"  [index]
+ src/urllib3/contrib/pyopenssl.py:424: error: Invalid index type "int" for "dict[_SSLMethod, Any]"; expected type "_SSLMethod"  [index]

@hauntsaninja hauntsaninja merged commit 1dcff0d into python:master Sep 19, 2023
18 checks passed
@hauntsaninja hauntsaninja deleted the jeopardy branch September 19, 2023 03:50
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.

Make --no-implicit-reexport friendlier
2 participants