Support conditional import in functions/classes #2304

drvink · 2016-10-23T20:37:12Z

codecov-io · 2016-10-23T20:45:08Z

Current coverage is 83.10% (diff: 93.75%)

Merging #2304 into master will increase coverage by 0.01%

@@             master      #2304   diff @@
==========================================
  Files            72         72          
  Lines         19013      19052    +39   
  Methods           0          0          
  Messages          0          0          
  Branches       3909       3926    +17   
==========================================
+ Hits          15799      15834    +35   
- Misses         2616       2617     +1   
- Partials        598        601     +3

Powered by Codecov. Last update 2e19181...c2ca2db

gvanrossum · 2016-10-23T21:33:01Z

Here's a test:

diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test
index d540a2d..bf3c9f7 100644
--- a/test-data/unit/check-unreachable-code.test
+++ b/test-data/unit/check-unreachable-code.test
@@ -372,7 +372,7 @@ def foo() -> None:
 [out]
 main: note: In function "foo":

-[case testSysVersionInfoInMethod]
+[case testSysPlatformInMethod]
 import sys
 class C:
     def foo(self) -> None:
@@ -385,6 +385,19 @@ class C:
 [out]
 main: note: In member "foo" of class "C":

+[case testSysPlatformInFunctionImport]
+import sys
+def foo() -> None:
+    if sys.platform != 'fictional':
+        import a
+    else:
+        import b as a
+    a.x
+[file a.py]
+x = 1
+[builtins fixtures/ops.pyi]
+[out]    
+
 [case testCustomSysVersionInfo]
 # flags: --python-version 3.2
 import sys

You have to apply it yourself, I don't have push permission to your fork.

drvink · 2016-10-24T00:32:44Z

Sorry, I just saw this now (and weird, you should have permission). I added the test.

gvanrossum · 2016-10-24T02:09:19Z

Thanks!

(Suggestion: It's actually easier for us to review if you don't squash your local commits. We squash them for you when we merge the PR.)

gvanrossum

This looks well-crafted. It also looks remarkably complex (though I suspect every bit is needed!). On the off-chance that there's a better way I'm deferring to @JukkaL. But likely it's okay with him too -- and you've probably fixed some minor subtle thing in the process!

gvanrossum · 2016-10-24T02:47:50Z

mypy/semanal.py


    def visit_func_def(self, func: FuncDef) -> None:
        sem = self.sem
        func.is_conditional = sem.block_depth[-1] > 0
        func._fullname = sem.qualified_name(func.name())
-        if func.name() in sem.globals:
+        at_module = sem.is_module_scope()


I don't think it's worth a separate local variable to shorten the condition on the next line (and notice our line length limit is 99).

Oh, oops. The separation was indeed to make the line shorter (which mattered in some WIP copy) but is now unnecessary. I'll remove it.

(In the simplified version, at_module still eliminates some redundancy, so it has returned.)

gvanrossum · 2016-10-24T02:48:20Z

mypy/semanal.py

@@ -2418,22 +2418,25 @@ def visit_await_expr(self, expr: AwaitExpr) -> None:
    # Helpers
    #

-    def lookup(self, name: str, ctx: Context) -> SymbolTableNode:
+    def lookup(self, name: str, ctx: Context, include_globals: bool = True,
+               report: bool = True) -> SymbolTableNode:
        """Look up an unqualified name in all active namespaces."""


Would be nice to explain in the docstring what the new Boolean arguments mean.

gvanrossum · 2016-10-24T02:49:24Z

mypy/semanal.py

@@ -2418,22 +2418,25 @@ def visit_await_expr(self, expr: AwaitExpr) -> None:
    # Helpers
    #

-    def lookup(self, name: str, ctx: Context) -> SymbolTableNode:
+    def lookup(self, name: str, ctx: Context, include_globals: bool = True,
+               report: bool = True) -> SymbolTableNode:


The name "report(to me, in the context or mypy) suggests a linkage to the report generation functionality (e.g.--linecount-report,--html-report). Maybe lengthen toreport_errors`?

Alternatively, maybe combine the two flags? They are always both on or both off. (Also, when off, lookup() is used as a Boolean, but I'm not sure how to benefit from that to make the code simpler.)

report_errors will be less ambiguous, so I'll change it.

I intentionally made include_globals separate since even though they're only currently used both on or off, someone might later want to use the function in a way where not searching for globals is desired, but reporting an error is. Also, what to search and whether to report an error are totally unrelated features, so this makes the function's behavior clear, precise, and intuitive to its arguments, rather than subtly behaving differently based on a single toggle.

Personally, I'd split lookup into different functions for each step, and then have a function where the caller could specify which steps they wanted and whether they wanted an error reported, but that would be an unrelated refactor, so I opted for the least invasive change.

drvink · 2016-10-24T05:10:22Z

It seems it might not need to be as complicated: all tests still pass and the functionality still works if I remove all the code for adding local symbols. I'm not familiar enough with mypy's internals to understand why (it was hard to determine how much needed to be added during the first pass to make this work), so I'm pushing a simplified version on top of the cleanups to the original, and @JukkaL can just ignore the simplified one (the most recent commit) if it's wrong.

gvanrossum · 2016-10-25T18:25:28Z

Please let us worry about future scenarios. :-)

gvanrossum · 2016-10-27T01:18:42Z

mypy/semanal.py

-            sem.globals[func.name()] = SymbolTableNode(GDEF, func, sem.cur_mod_id)
+            if at_module:
+                sem.globals[func.name()] = SymbolTableNode(GDEF, func, sem.cur_mod_id)
+            sem.function_stack.append(func)


So this is the key part of this diff (processing function bodies in the FirstPass). Could you add a comment drawing attention to it? Also, I wonder if it would be prudent to add matching calls to self.errors.push_function() and self.errors.pop_function()?

gvanrossum · 2016-10-28T18:11:11Z

I'm going to make that edit (adding a comment) myself and then merge.

drvink · 2016-10-28T18:34:58Z

Sorry for being slow to reply. That should be fine, though I think you meant func.name() :)

gvanrossum · 2016-10-28T19:48:08Z

(Sorry bout that! Juggling too many eggs today. :-)

drvink mentioned this pull request Oct 23, 2016

Python version-conditional import inside function does not respect test #2260

Closed

Support conditional import in functions/classes

c0c8dd9

gvanrossum reviewed Oct 24, 2016

View reviewed changes

drvink added 2 commits October 24, 2016 13:22

Suggested cleanups

d43fbd2

Simplify code

c2ca2db

gvanrossum reviewed Oct 27, 2016

View reviewed changes

Added a comment and call sem.errors.{push,pop}_function()

09ecc92

Fix variable name

8e45bab

gvanrossum merged commit 075a8da into python:master Oct 28, 2016

refi64 mentioned this pull request Nov 14, 2016

Cannot find exec_module for _importlib_modulespec.exec_module with incremental #2451

Closed

drvink mentioned this pull request May 22, 2018

Is there any chance of type-checking Python 2 code? facebook/pyre-check#35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support conditional import in functions/classes #2304

Support conditional import in functions/classes #2304

drvink commented Oct 23, 2016 •

edited by gvanrossum

codecov-io commented Oct 23, 2016 •

edited

gvanrossum commented Oct 23, 2016

drvink commented Oct 24, 2016

gvanrossum commented Oct 24, 2016

gvanrossum left a comment

gvanrossum Oct 24, 2016

drvink Oct 24, 2016

drvink Oct 24, 2016 •

edited

gvanrossum Oct 24, 2016

drvink Oct 24, 2016

gvanrossum Oct 24, 2016

drvink Oct 24, 2016

drvink commented Oct 24, 2016

gvanrossum commented Oct 25, 2016 via email

gvanrossum Oct 27, 2016

gvanrossum commented Oct 28, 2016

drvink commented Oct 28, 2016

gvanrossum commented Oct 28, 2016

Support conditional import in functions/classes #2304

Support conditional import in functions/classes #2304

Conversation

drvink commented Oct 23, 2016 • edited by gvanrossum

codecov-io commented Oct 23, 2016 • edited

Current coverage is 83.10% (diff: 93.75%)

gvanrossum commented Oct 23, 2016

drvink commented Oct 24, 2016

gvanrossum commented Oct 24, 2016

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum Oct 24, 2016

Choose a reason for hiding this comment

drvink Oct 24, 2016

Choose a reason for hiding this comment

drvink Oct 24, 2016 • edited

Choose a reason for hiding this comment

gvanrossum Oct 24, 2016

Choose a reason for hiding this comment

drvink Oct 24, 2016

Choose a reason for hiding this comment

gvanrossum Oct 24, 2016

Choose a reason for hiding this comment

drvink Oct 24, 2016

Choose a reason for hiding this comment

drvink commented Oct 24, 2016

gvanrossum commented Oct 25, 2016 via email

gvanrossum Oct 27, 2016

Choose a reason for hiding this comment

gvanrossum commented Oct 28, 2016

drvink commented Oct 28, 2016

gvanrossum commented Oct 28, 2016

drvink commented Oct 23, 2016 •

edited by gvanrossum

codecov-io commented Oct 23, 2016 •

edited

drvink Oct 24, 2016 •

edited