Reject semantic syntax errors for lazy imports#23757
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 87.10%. The percentage of expected errors that received a diagnostic held steady at 77.81%. |
Memory usage reportMemory usage unchanged ✅ |
|
|
03d95de to
7ca4cfe
Compare
487b7c5 to
2bf7354
Compare
7ca4cfe to
57fe98b
Compare
| Stmt::ImportFrom(StmtImportFrom { | ||
| module, is_lazy, .. | ||
| }) => { | ||
| // Allow __future__ imports until we see a non-__future__ import. |
There was a problem hiding this comment.
This comment needs updating. It seems any lazy import acts as import boundary, even if it imports future?
| self.visit_body(body); | ||
| for handler in handlers { | ||
| self.visit_except_handler(handler); | ||
| } | ||
| self.visit_body(orelse); | ||
| self.visit_body(finalbody); |
There was a problem hiding this comment.
Nit: Call walk_stmt instead of inlining the visitor code here
| python_version: PythonVersion, | ||
| source_text: OnceCell<SourceText>, | ||
| semantic_checker: SemanticSyntaxChecker, | ||
| try_depth: u32, |
There was a problem hiding this comment.
A simple bool should be sufficient. Before entering a try, store whether we were in a try before, then set it to true. After visiting the try, restore the value to its previous value
| ScopeKind::Function => return Some(LazyImportContext::Function), | ||
| ScopeKind::Class => return Some(LazyImportContext::Class), | ||
| ScopeKind::Comprehension | ||
| | ScopeKind::Lambda |
There was a problem hiding this comment.
The handling of lambda is inconsistent between Ruff and ty. Ruff returns Function whereas ty returns None. Which is correct? It probably doesn't matter because lambdas can't contain statements. I suggest that you change the match grouping in both implementations to something like:
match ... {
// Possible, but invalid positions
ScopeKind::Function => {..},
ScopeKind::Class => {...},
// valid positions
ScopeKind::Module => {},
// Impossible positions
ScopeKind::Comprehension | SK::Lambda | SK::TypeAlias | SK::TypeParams => {}
The comments and grouping will help future readers to understand that the last match arms group doesn't really matter at all.
|
I'm not sure if this is something you did in your previous PR but we should make sure that both ty and Ruff don't treat lazy future import as future imports. We don't need to do this in this PR but seems worth doing now, or we'll forget. |
57fe98b to
824e80e
Compare
Summary
We now reject cases like:
lazy import ...inside functions, includingasync deflazy from ... import ...inside functions, includingasync deflazy import ...andlazy from ... import ...inside class bodieslazy import ...andlazy from ... import ...anywhere inside atrystatement, includingexcept/except*lazy from ... import *anywherelazy from __future__ import ...anywhereA follow-up to #23755.
See: #21305.