error on break/continue outside of loop

Summary:
This diff sets up the proper plumbing for Ruff's SemanticSyntaxChecker, which was being used incorrectly before. This lets us emit errors for break/continue outside of loops.

Not all errors are emitted here because some are redundant w/ existing errors that Pyrefly emits, and to reduce the scope of changes in this diff. We can turn additional errors on in the future with appropriate testing.

closes #1796

Reviewed By: grievejia

Differential Revision: D88700199

fbshipit-source-id: ac7e2515af83f9fe3ae4dc260807a695712f989c

Author: yangdanny97

crates/pyrefly_config/src/error_kind.rs

@@ -179,7 +179,7 @@ pub enum ErrorKind {
     /// e.g. calling `super(Y, x)` on an object `x` that does not match the class `Y`.
     InvalidSuperCall,
     /// Incorrect Python syntax, construct is not allowed in this position.
-    /// In many cases a syntax error will also be reported.
+    /// In many cases a parse error will also be reported.
     InvalidSyntax,
     /// An error related to type alias usage or definition.
     InvalidTypeAlias,

pyrefly/lib/binding/bindings.rs

@@ -5,6 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+use std::cell::RefCell;
 use std::fmt;
 use std::fmt::Debug;
 use std::fmt::Display;
@@ -30,6 +31,10 @@ use ruff_python_ast::Stmt;
 use ruff_python_ast::TypeParam;
 use ruff_python_ast::TypeParams;
 use ruff_python_ast::name::Name;
+use ruff_python_parser::semantic_errors::SemanticSyntaxChecker;
+use ruff_python_parser::semantic_errors::SemanticSyntaxContext;
+use ruff_python_parser::semantic_errors::SemanticSyntaxError;
+use ruff_python_parser::semantic_errors::SemanticSyntaxErrorKind;
 use ruff_text_size::Ranged;
 use ruff_text_size::TextRange;
 use ruff_text_size::TextSize;
@@ -197,6 +202,8 @@ pub struct BindingsBuilder<'a> {
     unused_parameters: Vec<UnusedParameter>,
     unused_imports: Vec<UnusedImport>,
     unused_variables: Vec<UnusedVariable>,
+    semantic_checker: SemanticSyntaxChecker,
+    semantic_syntax_errors: RefCell<Vec<SemanticSyntaxError>>,
 }
 
 /// An enum tracking whether we are in a generator expression
@@ -402,6 +409,8 @@ impl Bindings {
             unused_parameters: Vec::new(),
             unused_imports: Vec::new(),
             unused_variables: Vec::new(),
+            semantic_checker: SemanticSyntaxChecker::new(),
+            semantic_syntax_errors: RefCell::new(Vec::new()),
         };
         builder.init_static_scope(&x.body, true);
         if module_info.name() != ModuleName::builtins() {
@@ -426,6 +435,18 @@ impl Bindings {
         let unused_imports = builder.scopes.collect_module_unused_imports();
         builder.record_unused_imports(unused_imports);
         let scope_trace = builder.scopes.finish();
+
+        let semantic_errors = builder.semantic_syntax_errors.into_inner();
+        for error in semantic_errors {
+            if Self::should_emit_semantic_syntax_error(&error) {
+                builder.errors.add(
+                    error.range,
+                    ErrorInfo::Kind(ErrorKind::InvalidSyntax),
+                    vec1![error.to_string()],
+                );
+            }
+        }
+
         let exported = exports.exports(lookup);
         for (name, exportable) in scope_trace.exportables().into_iter_hashed() {
             let binding = match exportable {
@@ -456,6 +477,43 @@ impl Bindings {
             unused_variables: builder.unused_variables,
         }))
     }
+
+    fn should_emit_semantic_syntax_error(error: &SemanticSyntaxError) -> bool {
+        // TODO: enable and add tests for the other semantic syntax errors
+        match error.kind {
+            SemanticSyntaxErrorKind::BreakOutsideLoop
+            | SemanticSyntaxErrorKind::ContinueOutsideLoop
+            | SemanticSyntaxErrorKind::SingleStarredAssignment
+            | SemanticSyntaxErrorKind::DifferentMatchPatternBindings
+            | SemanticSyntaxErrorKind::IrrefutableCasePattern(_) => true,
+            SemanticSyntaxErrorKind::LateFutureImport
+            | SemanticSyntaxErrorKind::InvalidStarExpression
+            | SemanticSyntaxErrorKind::ReboundComprehensionVariable
+            | SemanticSyntaxErrorKind::DuplicateTypeParameter
+            | SemanticSyntaxErrorKind::MultipleCaseAssignment(_)
+            | SemanticSyntaxErrorKind::WriteToDebug(_)
+            | SemanticSyntaxErrorKind::InvalidExpression(_, _)
+            | SemanticSyntaxErrorKind::DuplicateMatchKey(_)
+            | SemanticSyntaxErrorKind::DuplicateMatchClassAttribute(_)
+            | SemanticSyntaxErrorKind::LoadBeforeGlobalDeclaration { .. }
+            | SemanticSyntaxErrorKind::LoadBeforeNonlocalDeclaration { .. }
+            | SemanticSyntaxErrorKind::AsyncComprehensionInSyncComprehension(_)
+            | SemanticSyntaxErrorKind::YieldOutsideFunction(_)
+            | SemanticSyntaxErrorKind::ReturnOutsideFunction
+            | SemanticSyntaxErrorKind::AwaitOutsideAsyncFunction(_)
+            | SemanticSyntaxErrorKind::DuplicateParameter(_)
+            | SemanticSyntaxErrorKind::NonlocalDeclarationAtModuleLevel
+            | SemanticSyntaxErrorKind::NonlocalAndGlobal(_)
+            | SemanticSyntaxErrorKind::AnnotatedGlobal(_)
+            | SemanticSyntaxErrorKind::AnnotatedNonlocal(_)
+            | SemanticSyntaxErrorKind::YieldFromInAsyncFunction
+            | SemanticSyntaxErrorKind::NonModuleImportStar(_)
+            | SemanticSyntaxErrorKind::MultipleStarredExpressions
+            | SemanticSyntaxErrorKind::FutureFeatureNotDefined(_)
+            | SemanticSyntaxErrorKind::GlobalParameter(_)
+            | SemanticSyntaxErrorKind::NonlocalWithoutBinding(_) => false,
+        }
+    }
 }
 
 impl BindingTable {
@@ -1467,4 +1525,81 @@ impl<'a> BindingsBuilder<'a> {
             }
         }
     }
+
+    pub fn with_semantic_checker(&mut self, f: impl FnOnce(&mut SemanticSyntaxChecker, &Self)) {
+        let mut checker = std::mem::take(&mut self.semantic_checker);
+        f(&mut checker, self);
+        self.semantic_checker = checker;
+    }
+}
+
+impl<'a> SemanticSyntaxContext for BindingsBuilder<'a> {
+    fn python_version(&self) -> ruff_python_ast::PythonVersion {
+        ruff_python_ast::PythonVersion {
+            major: self.sys_info.version().major as u8,
+            minor: self.sys_info.version().minor as u8,
+        }
+    }
+
+    fn source(&self) -> &str {
+        self.module_info.contents()
+    }
+
+    fn future_annotations_or_stub(&self) -> bool {
+        self.module_info.source_type() == ruff_python_ast::PySourceType::Stub
+    }
+
+    fn report_semantic_error(&self, error: SemanticSyntaxError) {
+        self.semantic_syntax_errors.borrow_mut().push(error);
+    }
+
+    fn global(&self, name: &str) -> Option<TextRange> {
+        self.scopes.get_global_declaration(name)
+    }
+
+    fn has_nonlocal_binding(&self, name: &str) -> bool {
+        self.scopes.has_nonlocal_binding(name)
+    }
+
+    fn in_async_context(&self) -> bool {
+        self.scopes.is_in_async_def()
+    }
+
+    fn in_await_allowed_context(&self) -> bool {
+        // await is allowed in functions, lambdas, and notebooks
+        self.scopes.in_function_scope() || self.in_notebook()
+    }
+
+    fn in_yield_allowed_context(&self) -> bool {
+        // yield is allowed in functions and lambdas, but not in comprehensions or classes
+        self.scopes.in_function_scope()
+    }
+
+    fn in_sync_comprehension(&self) -> bool {
+        self.scopes.in_sync_comprehension()
+    }
+
+    fn in_module_scope(&self) -> bool {
+        self.scopes.in_module_or_class_top_level() && !self.scopes.in_class_body()
+    }
+
+    fn in_function_scope(&self) -> bool {
+        self.scopes.in_function_scope()
+    }
+
+    fn in_generator_scope(&self) -> bool {
+        self.scopes.in_generator_expression()
+    }
+
+    fn in_notebook(&self) -> bool {
+        self.module_info.source_type() == ruff_python_ast::PySourceType::Ipynb
+    }
+
+    fn in_loop_context(&self) -> bool {
+        self.scopes.loop_depth() > 0
+    }
+
+    fn is_bound_parameter(&self, name: &str) -> bool {
+        self.scopes.is_bound_parameter(name)
+    }
 }

pyrefly/lib/binding/expr.rs

@@ -399,7 +399,7 @@ impl<'a> BindingsBuilder<'a> {
                         "`async` can only be used inside an async function".to_owned(),
                     );
                 }
-                self.scopes.push(Scope::comprehension(range));
+                self.scopes.push(Scope::comprehension(range, is_generator));
             }
             // Incomplete nested comprehensions can have identical iterators
             // for inner and outer loops. It is safe to overwrite it because it literally the same.
@@ -506,6 +506,8 @@ impl<'a> BindingsBuilder<'a> {
 
     /// Execute through the expr, ensuring every name has a binding.
     pub fn ensure_expr(&mut self, x: &mut Expr, usage: &mut Usage) {
+        self.with_semantic_checker(|semantic, context| semantic.visit_expr(x, context));
+
         match x {
             Expr::If(x) => {
                 // Ternary operation. We treat it like an if/else statement.

pyrefly/lib/binding/scope.rs

@@ -866,7 +866,7 @@ impl ScopeMethod {
 enum ScopeKind {
     Annotation,
     Class(ScopeClass),
-    Comprehension,
+    Comprehension { is_generator: bool },
     Function(ScopeFunction),
     Method(ScopeMethod),
     Module,
@@ -989,11 +989,11 @@ impl Scope {
         )
     }
 
-    pub fn comprehension(range: TextRange) -> Self {
+    pub fn comprehension(range: TextRange, is_generator: bool) -> Self {
         Self::new(
             range,
             FlowBarrier::AllowFlowChecked,
-            ScopeKind::Comprehension,
+            ScopeKind::Comprehension { is_generator },
         )
     }
 
@@ -1417,6 +1417,90 @@ impl Scopes {
         self.current().loops.len()
     }
 
+    /// Check if a name is declared as global in the current scope.
+    /// Returns the range of the global statement if found.
+    pub fn get_global_declaration(&self, name: &str) -> Option<TextRange> {
+        if let Some(static_info) = self.current().stat.0.get(&Name::new(name))
+            && let StaticStyle::MutableCapture(MutableCapture {
+                kind: MutableCaptureKind::Global,
+                ..
+            }) = &static_info.style
+        {
+            return Some(static_info.range);
+        }
+        None
+    }
+
+    /// Check if a name has a nonlocal binding in an enclosing scope.
+    pub fn has_nonlocal_binding(&self, name: &str) -> bool {
+        let name_obj = Name::new(name);
+        // Skip the current scope and check enclosing scopes
+        for scope in self.iter_rev().skip(1) {
+            // Check if this name is defined in this scope (not as a mutable capture)
+            if let Some(static_info) = scope.stat.0.get(&name_obj) {
+                match &static_info.style {
+                    // Don't count mutable captures as nonlocal bindings
+                    StaticStyle::MutableCapture(..) => continue,
+                    // Any other definition counts as a binding
+                    _ => return true,
+                }
+            }
+        }
+        false
+    }
+
+    /// Check if we're currently in a comprehension scope (not a generator expression).
+    pub fn in_comprehension(&self) -> bool {
+        matches!(self.current().kind, ScopeKind::Comprehension { .. })
+    }
+
+    /// Check if we're in a synchronous comprehension.
+    /// A comprehension is synchronous unless we're in an async function.
+    pub fn in_sync_comprehension(&self) -> bool {
+        if !self.in_comprehension() {
+            return false;
+        }
+        // Check if any enclosing scope is an async function
+        for scope in self.iter_rev().skip(1) {
+            if let ScopeKind::Function(func_scope) = &scope.kind {
+                return !func_scope.is_async;
+            } else if let ScopeKind::Method(method_scope) = &scope.kind {
+                return !method_scope.is_async;
+            }
+        }
+        // If we didn't find a function, it's synchronous
+        true
+    }
+
+    /// Check if we're in a generator expression scope.
+    /// Generator expressions are created for `Expr::Generator` comprehensions.
+    pub fn in_generator_expression(&self) -> bool {
+        matches!(
+            self.current().kind,
+            ScopeKind::Comprehension { is_generator: true }
+        )
+    }
+
+    /// Check if a name is a bound parameter in the current function scope.
+    pub fn is_bound_parameter(&self, name: &str) -> bool {
+        let name_obj = Name::new(name);
+        // Check the current scope and enclosing scopes for a function with this parameter
+        for scope in self.iter_rev() {
+            match &scope.kind {
+                ScopeKind::Function(func_scope) => {
+                    return func_scope.parameters.contains_key(&name_obj);
+                }
+                ScopeKind::Method(method_scope) => {
+                    return method_scope.parameters.contains_key(&name_obj);
+                }
+                // Don't look past module or class boundaries
+                ScopeKind::Module | ScopeKind::Class(_) => return false,
+                _ => {}
+            }
+        }
+        false
+    }
+
     /// Track a narrow for a name in the current flow. This should result from options
     /// that only narrow an existing value, not operations that assign a new value at runtime.
     ///

pyrefly/lib/binding/stmt.rs

@@ -299,6 +299,8 @@ impl<'a> BindingsBuilder<'a> {
     /// Evaluate the statements and update the bindings.
     /// Every statement should end up in the bindings, perhaps with a location that is never used.
     pub fn stmt(&mut self, x: Stmt, parent: &NestingContext) {
+        self.with_semantic_checker(|semantic, context| semantic.visit_stmt(&x, context));
+
         match x {
             Stmt::FunctionDef(x) => {
                 self.function_def(x, parent);