Apply review suggestions

compiler/src/compile.rs

@@ -60,13 +60,10 @@ impl CodeInfo {
                 .cellvars
                 .iter()
                 .map(|var| {
-                    for (i, arg) in all_args.iter().enumerate() {
-                        if var == arg {
-                            found_cellarg = true;
-                            return i as isize;
-                        }
-                    }
-                    -1
+                    all_args.iter().position(|arg| var == arg).map_or(-1, |i| {
+                        found_cellarg = true;
+                        i as isize
+                    })
                 })
                 .collect::<Box<[_]>>();
             if found_cellarg {

compiler/src/symboltable.rs

@@ -108,6 +108,7 @@ pub struct Symbol {
     pub is_parameter: bool,
     pub is_annotated: bool,
     pub is_imported: bool,
+    pub is_nonlocal: bool,
 
     // indicates if the symbol gets a value assigned by a named expression in a comprehension
     // this is required to correct the scope in the analysis.
@@ -139,6 +140,7 @@ impl Symbol {
             is_parameter: false,
             is_annotated: false,
             is_imported: false,
+            is_nonlocal: false,
             is_assign_namedexpr_in_comprehension: false,
             is_iter: false,
             is_free_class: false,
@@ -218,7 +220,9 @@ mod stack {
         }
     }
     impl<T> StackStack<T> {
-        pub fn append<F, R>(&mut self, x: &mut T, f: F) -> R
+        /// Appends a reference to this stack for the duration of the function `f`. When `f`
+        /// returns, the reference will be popped off the stack.
+        pub fn with_append<F, R>(&mut self, x: &mut T, f: F) -> R
         where
             F: FnOnce(&mut Self) -> R,
         {
@@ -273,7 +277,7 @@ impl SymbolTableAnalyzer {
         let sub_tables = &mut *symbol_table.sub_tables;
 
         let mut info = (symbols, symbol_table.typ);
-        self.tables.append(&mut info, |list| {
+        self.tables.with_append(&mut info, |list| {
             let inner_scope = unsafe { &mut *(list as *mut _ as *mut SymbolTableAnalyzer) };
             // Analyze sub scopes:
             for sub_table in sub_tables.iter_mut() {
@@ -1162,6 +1166,7 @@ impl SymbolTableBuilder {
         match role {
             SymbolUsage::Nonlocal => {
                 symbol.scope = SymbolScope::Free;
+                symbol.is_nonlocal = true;
             }
             SymbolUsage::Imported => {
                 symbol.is_assigned = true;

derive/src/pyclass.rs

@@ -615,9 +615,7 @@ impl PropertyItemMeta {
     fn property_name(&self) -> Result<(String, GetSetItemKind)> {
         let inner = self.inner();
         let magic = inner._bool("magic")?;
-        let setter = inner._bool("setter")?;
-        let deleter = inner._bool("deleter")?;
-        let kind = match (setter, deleter) {
+        let kind = match (inner._bool("setter")?, inner._bool("deleter")?) {
             (false, false) => GetSetItemKind::Get,
             (true, false) => GetSetItemKind::Set,
             (false, true) => GetSetItemKind::Delete,
@@ -636,54 +634,37 @@ impl PropertyItemMeta {
             name
         } else {
             let sig_name = inner.item_name();
-            let name = if setter {
-                if let Some(name) = sig_name.strip_prefix("set_") {
+            let extract_prefix_name = |prefix, item_typ| {
+                if let Some(name) = sig_name.strip_prefix(prefix) {
                     if name.is_empty() {
-                        return Err(syn::Error::new_spanned(
+                        Err(syn::Error::new_spanned(
                             &inner.meta_ident,
                             format!(
-                                "A #[{}(setter)] fn with a set_* name must \
-                                 have something after \"set_\"",
-                                inner.meta_name()
+                                "A #[{}({typ})] fn with a {prefix}* name must \
+                                 have something after \"{prefix}\"",
+                                inner.meta_name(),
+                                typ = item_typ,
+                                prefix = prefix
                             ),
-                        ));
+                        ))
+                    } else {
+                        Ok(name.to_owned())
                     }
-                    name.to_string()
                 } else {
-                    return Err(syn::Error::new_spanned(
+                    Err(syn::Error::new_spanned(
                         &inner.meta_ident,
                         format!(
                             "A #[{}(setter)] fn must either have a `name` \
                              parameter or a fn name along the lines of \"set_*\"",
                             inner.meta_name()
                         ),
-                    ));
+                    ))
                 }
-            } else if deleter {
-                if let Some(name) = sig_name.strip_prefix("del_") {
-                    if name.is_empty() {
-                        return Err(syn::Error::new_spanned(
-                            &inner.meta_ident,
-                            format!(
-                                "A #[{}(deleter)] fn with a del_* name must \
-                                 have something after \"del_\"",
-                                inner.meta_name()
-                            ),
-                        ));
-                    }
-                    name.to_string()
-                } else {
-                    return Err(syn::Error::new_spanned(
-                        &inner.meta_ident,
-                        format!(
-                            "A #[{}(deleter)] fn must either have a `name` \
-                             parameter or a fn name along the lines of \"del_*\"",
-                            inner.meta_name()
-                        ),
-                    ));
-                }
-            } else {
-                sig_name
+            };
+            let name = match kind {
+                GetSetItemKind::Get => sig_name,
+                GetSetItemKind::Set => extract_prefix_name("set_", "setter")?,
+                GetSetItemKind::Delete => extract_prefix_name("del_", "deleter")?,
             };
             if magic {
                 format!("__{}__", name)

vm/src/builtins/dict.rs

@@ -492,6 +492,7 @@ impl PyDictRef {
         }
     }
 
+    #[cold]
     fn _subcls_getitem_option(
         &self,
         key: PyObjectRef,
@@ -504,7 +505,7 @@ impl PyDictRef {
         }
     }
 
-    pub fn get2<K: IntoPyObject + DictKey + Clone>(
+    pub fn get_chain<K: IntoPyObject + DictKey + Clone>(
         &self,
         other: &Self,
         key: K,
@@ -513,7 +514,7 @@ impl PyDictRef {
         let self_exact = self.class().is(&vm.ctx.types.dict_type);
         let other_exact = self.class().is(&vm.ctx.types.dict_type);
         if self_exact && other_exact {
-            self.entries.get2(&other.entries, vm, &key)
+            self.entries.get_chain(&other.entries, vm, &key)
         } else if let Some(value) = self._get_item_option_inner(key.clone(), self_exact, vm)? {
             Ok(Some(value))
         } else {

vm/src/dictdatatype.rs

@@ -284,7 +284,7 @@ impl<T: Clone> Dict<T> {
         Ok(ret)
     }
 
-    pub fn get2<K: DictKey>(
+    pub fn get_chain<K: DictKey>(
         &self,
         other: &Self,
         vm: &VirtualMachine,

vm/src/frame.rs

@@ -470,22 +470,14 @@ impl ExecutingFrame<'_> {
                 let x = self.locals.get_item_option(name.clone(), vm)?;
                 let x = match x {
                     Some(x) => x,
-                    None => self
-                        .globals
-                        .get2(self.builtins, name.clone(), vm)?
-                        .ok_or_else(|| {
-                            vm.new_name_error(format!("name '{}' is not defined", name))
-                        })?,
+                    None => self.load_global_or_builtin(name, vm)?,
                 };
                 self.push_value(x);
                 Ok(None)
             }
             bytecode::Instruction::LoadGlobal(idx) => {
                 let name = &self.code.names[*idx];
-                let x = self
-                    .globals
-                    .get2(self.builtins, name.clone(), vm)?
-                    .ok_or_else(|| vm.new_name_error(format!("name '{}' is not defined", name)))?;
+                let x = self.load_global_or_builtin(name, vm)?;
                 self.push_value(x);
                 Ok(None)
             }
@@ -952,6 +944,13 @@ impl ExecutingFrame<'_> {
         }
     }
 
+    #[inline]
+    fn load_global_or_builtin(&self, name: &PyStrRef, vm: &VirtualMachine) -> PyResult {
+        self.globals
+            .get_chain(self.builtins, name.clone(), vm)?
+            .ok_or_else(|| vm.new_name_error(format!("name '{}' is not defined", name)))
+    }
+
     #[cfg_attr(feature = "flame-it", flame("Frame"))]
     fn get_elements(
         &mut self,

vm/src/stdlib/symtable.rs

@@ -205,7 +205,7 @@ mod decl {
 
         #[pymethod(name = "is_nonlocal")]
         fn is_nonlocal(&self) -> bool {
-            matches!(self.symbol.scope, SymbolScope::Free)
+            self.symbol.is_nonlocal
         }
 
         #[pymethod(name = "is_referenced")]