Keep code reordering within region boundaries

README.md

@@ -81,6 +81,19 @@ It will print the files that need to be formatted.
 
 To see other possible options, run `gdscript-formatter --help`.
 
+### Reordering code regions
+
+With `--reorder-code`, top-level `#region` and `#endregion` markers separate
+independently sorted sections. Declarations can be reordered inside a region or
+in the code around it, but they never move across a region boundary. Nested
+regions keep their boundaries too, and empty or comment-only regions are
+preserved.
+
+This lets manual groups take precedence over the usual global declaration
+order. For example, a variable below a region stays below that region instead
+of moving to the top of the file. Files containing `# fmt: off` still skip
+reordering entirely.
+
 
 ## Using editorconfig
 

gdextension/src/lib.rs

@@ -18,7 +18,7 @@ unsafe impl ExtensionLibrary for FormatterExtension {}
 #[derive(GodotClass)]
 #[class(init, singleton, base=Object)]
 struct GDScriptFormatter {
-    base: Base<Object>,
+    base_object: Base<Object>,
 }
 
 #[godot_api]
@@ -61,7 +61,9 @@ macro_rules! extract_field {
         if let Some(variant) = $dict.get(stringify!($key)) {
             match variant.try_to::<$type>() {
                 Ok(val) => $target.$key = val,
-                Err(error) => godot_error!("Config '{}' is invalid: {}", stringify!($key), error),
+                Err(error) => {
+                    godot_error!("Config '{}' is invalid: {}", stringify!($key), error);
+                }
             }
         }
     };
@@ -70,7 +72,9 @@ macro_rules! extract_field {
         if let Some(variant) = $dict.get(stringify!($key)) {
             match variant.try_to::<$type>() {
                 Ok(val) => $target.$key = val as usize,
-                Err(error) => godot_error!("Config '{}' is invalid: {}", stringify!($key), error),
+                Err(error) => {
+                    godot_error!("Config '{}' is invalid: {}", stringify!($key), error);
+                }
             }
         }
     };
@@ -84,7 +88,9 @@ fn dict_to_formatter_config(dict: &Dictionary<Variant, Variant>) -> FormatterCon
     if let Some(variant) = dict.get("printer") {
         match variant.try_to::<Dictionary<Variant, Variant>>() {
             Ok(sub_dict) => result.printer = dict_to_printer_config(&sub_dict),
-            Err(error) => godot_error!("Config 'printer' is invalid: {}", error),
+            Err(error) => {
+                godot_error!("Config 'printer' is invalid: {}", error);
+            }
         }
     }
     if let Some(variant) = dict.get("quote_style") {
@@ -94,7 +100,9 @@ fn dict_to_formatter_config(dict: &Dictionary<Variant, Variant>) -> FormatterCon
             .and_then(|gstr| QuoteStyle::from_name(&gstr.to_string()))
         {
             Some(valid_style) => result.quote_style = valid_style,
-            None => godot_error!("Config 'quote_style' is invalid"),
+            None => {
+                godot_error!("Config 'quote_style' is invalid");
+            }
         }
     }
     result
@@ -125,7 +133,9 @@ fn dict_to_linter_config(dict: &Dictionary<Variant, Variant>) -> LinterConfig {
                     .map(|gd_str| gd_str.to_string())
                     .collect()
             }
-            Err(error) => godot_error!("Config 'disabled_rules' is invalid: {}", error),
+            Err(error) => {
+                godot_error!("Config 'disabled_rules' is invalid: {}", error);
+            }
         }
     }
     result

src/formatter.rs

@@ -3434,22 +3434,52 @@ fn process_source_reorder(context: &mut FormatterContext, node: tree_sitter::Nod
     let mut previous_classification: Option<DeclarationKind> = None;
     let mut previous_is_double_spaced = false;
     let mut previous_child_index: Option<u32> = None;
+    let mut previous_was_region_start = false;
     let mut is_first = true;
 
     let mut item_index = 0;
     while item_index < plan.items.len() {
         let item = &plan.items[item_index];
+        let child_kind = node
+            .child(item.child_index)
+            .map(GDScriptNodeKind::get_kind_from_ast_node);
+        let is_region_start = child_kind == Some(GDScriptNodeKind::RegionStart);
+        let is_region_end = child_kind == Some(GDScriptNodeKind::RegionEnd);
+        let mut spacing_classification = item.classification;
+        if is_region_start {
+            // Use the first declaration in output order to space the region
+            // like its contents. Looking at source order is not idempotent
+            // when the first declaration moves during reordering.
+            let mut next_item_index = item_index + 1;
+            while next_item_index < plan.items.len() {
+                let next_item = &plan.items[next_item_index];
+                if next_item.classification != DeclarationKind::RegionBoundary {
+                    spacing_classification = next_item.classification;
+                    break;
+                }
+                if node
+                    .child(next_item.child_index)
+                    .map(GDScriptNodeKind::get_kind_from_ast_node)
+                    == Some(GDScriptNodeKind::RegionEnd)
+                {
+                    break;
+                }
+                next_item_index += 1;
+            }
+        }
         let current_needs_two_blank = matches!(
-            item.classification,
+            spacing_classification,
             DeclarationKind::Method | DeclarationKind::InnerClass
         );
         let is_in_source_order = match previous_child_index {
             Some(index) => index < item.child_index,
             None => false,
         };
-        if !is_first {
+        if !is_first && (is_region_end || previous_was_region_start) {
+            context.render_elements.push(RenderElement::HardLine);
+        } else if !is_first {
             if let Some(ref previous_child) = previous_classification {
-                if previous_child == &item.classification
+                if previous_child == &spacing_classification
                     && !previous_is_double_spaced
                     && !current_needs_two_blank
                 {
@@ -3471,7 +3501,7 @@ fn process_source_reorder(context: &mut FormatterContext, node: tree_sitter::Nod
                         | DeclarationKind::Extends
                         | DeclarationKind::Docstring
                 ) && matches!(
-                    item.classification,
+                    spacing_classification,
                     DeclarationKind::ClassAnnotation
                         | DeclarationKind::ClassName
                         | DeclarationKind::Extends
@@ -3485,8 +3515,13 @@ fn process_source_reorder(context: &mut FormatterContext, node: tree_sitter::Nod
             }
         }
         is_first = false;
-        previous_classification = Some(item.classification);
-        previous_is_double_spaced = current_needs_two_blank;
+        if item.classification != DeclarationKind::RegionBoundary
+            || previous_classification.is_none()
+        {
+            previous_classification = Some(item.classification);
+            previous_is_double_spaced = current_needs_two_blank;
+        }
+        previous_was_region_start = is_region_start;
         previous_child_index = Some(item.child_index);
 
         // Output source children attached before the declaration.

src/reorder.rs

@@ -5,6 +5,7 @@
 //!
 //! Comments and annotations that precede a declaration in source order are
 //! bundled with it so they move together when reordered.
+//! Region markers stay in source order and separate independently sorted runs.
 
 use crate::node_kind::GDScriptNodeKind;
 use crate::shared_utils::should_annotation_be_inline;
@@ -17,17 +18,20 @@ pub struct ReorderPlan<'a> {
 
 #[derive(Debug, Clone)]
 pub struct ReorderItem<'a> {
+    /// Declarations cannot move across a region marker. Each marker also has
+    /// its own partition so it stays between the surrounding runs.
+    region_partition: u32,
     /// Index of this child in the parent node (for direct children).
     pub child_index: u32,
     /// If Some(i), this item refers to the i-th child OF the node at
     /// `child_index`. Used for split inline extends: the extends_statement
     /// is a child of class_name_statement, not a sibling.
     pub sub_child: Option<u32>,
     /// Indices of source children that precede and move with this declaration,
-    /// such as comments, annotations, and region starts.
+    /// such as comments and annotations.
     pub child_indices_attached_before_declaration: Vec<u32>,
     /// Indices of source children that follow and move with this declaration,
-    /// such as end-of-line comments and region ends.
+    /// such as end-of-line comments.
     pub child_indices_attached_after_declaration: Vec<u32>,
     /// Whether this declaration or its leading comments had a blank line before
     /// it in the source. This preserves blanks lines input by the user in their
@@ -64,6 +68,7 @@ pub enum DeclarationKind {
     OnReadyVariable, // 10
     Method,          // 11: functions (sub-sorted by MethodType)
     InnerClass,      // 12
+    RegionBoundary,  // 13: fixed marker between independently sorted runs
     Unknown = 255,   // 255
 }
 
@@ -140,9 +145,12 @@ pub fn build_reorder_plan<'a>(parent: Node<'a>, content: &'a str) -> ReorderPlan
 
     // Pass 1: classify each child.
     // These source children move with a nearby declaration. They include
-    // comments, annotations, and region markers.
+    // comments and annotations. Region markers are standalone items.
     let mut is_child_attached_to_declaration = vec![false; child_count as usize];
-    let mut is_region_end = vec![false; child_count as usize];
+    let mut region_partitions = vec![0; child_count as usize];
+    let mut region_partition = 0;
+    let mut has_pending_annotation = false;
+    let mut keep_annotated_declaration_first = false;
 
     // We use this to track if we visited a line with an annotation before
     // encountering a declaration.
@@ -166,14 +174,43 @@ pub fn build_reorder_plan<'a>(parent: Node<'a>, content: &'a str) -> ReorderPlan
             continue;
         };
         let kind = GDScriptNodeKind::get_kind_from_ast_node(child);
-        if kind == GDScriptNodeKind::Comment || kind == GDScriptNodeKind::RegionStart {
+        if matches!(
+            kind,
+            GDScriptNodeKind::RegionStart | GDScriptNodeKind::RegionEnd
+        ) {
+            region_partition += 1;
+            region_partitions[child_index as usize] = region_partition;
+            items.push(ReorderItem {
+                region_partition,
+                child_index,
+                sub_child: None,
+                child_indices_attached_before_declaration: Vec::new(),
+                child_indices_attached_after_declaration: Vec::new(),
+                has_blank_line_before: false,
+                classification: DeclarationKind::RegionBoundary,
+                name: "",
+                is_pseudo_private: false,
+                method_type: None,
+                split_extends: false,
+            });
+            region_partition += 1;
+            // An annotation can precede a region marker but still apply to
+            // the first declaration inside. Do not sort a different target
+            // between that annotation and its declaration.
+            keep_annotated_declaration_first |= has_pending_annotation;
+            child_index += 1;
+            continue;
+        }
+        region_partitions[child_index as usize] = region_partition;
+        if kind == GDScriptNodeKind::Comment {
             is_child_attached_to_declaration[child_index as usize] = true;
         } else if kind == GDScriptNodeKind::Annotation {
             // Make sure to keep tool and icon at the top of the script, above
             // class_name and extends. Otherwise it's a syntax error.
             let annotation_name = get_node_text(child, content);
             if annotation_name.starts_with("@tool") || annotation_name.starts_with("@icon") {
                 items.push(ReorderItem {
+                    region_partition,
                     child_index,
                     sub_child: None,
                     child_indices_attached_before_declaration: Vec::new(),
@@ -186,6 +223,7 @@ pub fn build_reorder_plan<'a>(parent: Node<'a>, content: &'a str) -> ReorderPlan
                     split_extends: false,
                 });
             } else {
+                has_pending_annotation = true;
                 is_child_attached_to_declaration[child_index as usize] = true;
                 if let Some(annotation_identifier) = get_annotation_identifier(child, content) {
                     if annotation_identifier == "onready" {
@@ -195,15 +233,13 @@ pub fn build_reorder_plan<'a>(parent: Node<'a>, content: &'a str) -> ReorderPlan
                     }
                 }
             }
-        } else if kind == GDScriptNodeKind::RegionEnd {
-            is_child_attached_to_declaration[child_index as usize] = true;
-            is_region_end[child_index as usize] = true;
         } else if kind == GDScriptNodeKind::SemiColon {
             // skip; handled by builder spacing
         } else {
             let child_classification = classify_child(child, content, &visited_annotations);
             let is_private = child_classification.name.starts_with('_');
             items.push(ReorderItem {
+                region_partition,
                 child_index,
                 sub_child: None,
                 child_indices_attached_before_declaration: Vec::new(),
@@ -219,6 +255,7 @@ pub fn build_reorder_plan<'a>(parent: Node<'a>, content: &'a str) -> ReorderPlan
             if child_classification.split_extends {
                 if let Some(extends_index) = find_extends_child_index(child) {
                     items.push(ReorderItem {
+                        region_partition,
                         child_index,
                         sub_child: Some(extends_index),
                         child_indices_attached_before_declaration: Vec::new(),
@@ -233,6 +270,11 @@ pub fn build_reorder_plan<'a>(parent: Node<'a>, content: &'a str) -> ReorderPlan
                 }
             }
 
+            if keep_annotated_declaration_first {
+                region_partition += 1;
+                keep_annotated_declaration_first = false;
+            }
+            has_pending_annotation = false;
             visited_annotations = VisitedAnnotation::default();
         }
         child_index += 1;
@@ -247,6 +289,10 @@ pub fn build_reorder_plan<'a>(parent: Node<'a>, content: &'a str) -> ReorderPlan
     let mut item_index = 0;
     while item_index < declaration_count {
         let item = &items[item_index];
+        if item.classification == DeclarationKind::RegionBoundary {
+            item_index += 1;
+            continue;
+        }
         if !matches!(
             item.classification,
             DeclarationKind::ClassAnnotation
@@ -305,6 +351,7 @@ pub fn build_reorder_plan<'a>(parent: Node<'a>, content: &'a str) -> ReorderPlan
 
     if !docstring_indices.is_empty() {
         items.push(ReorderItem {
+            region_partition: region_partitions[docstring_indices[0] as usize],
             child_index: docstring_indices[0],
             sub_child: None,
             child_indices_attached_before_declaration: docstring_indices,
@@ -339,9 +386,7 @@ pub fn build_reorder_plan<'a>(parent: Node<'a>, content: &'a str) -> ReorderPlan
         let mut child_indices_attached_before_declaration = Vec::new();
         let mut current_child_index = first_possible_attachment_child_index;
         while current_child_index < declaration_child_index {
-            if is_child_attached_to_declaration[current_child_index as usize]
-                && !is_region_end[current_child_index as usize]
-            {
+            if is_child_attached_to_declaration[current_child_index as usize] {
                 child_indices_attached_before_declaration.push(current_child_index);
             }
             current_child_index += 1;
@@ -373,18 +418,15 @@ pub fn build_reorder_plan<'a>(parent: Node<'a>, content: &'a str) -> ReorderPlan
             }
         }
 
-        // Attach region ends and an inline comment after the declaration to
-        // that declaration.
+        // Attach an inline comment after the declaration to that declaration.
         let mut indices_of_children_attached_after_declaration = Vec::new();
         let mut next_child_index = declaration_child_index + 1;
         if let Some(next_declaration_child_index) = next_declaration_child_index {
             let current_declaration = parent
                 .child(declaration_child_index)
                 .expect("declaration child index came from this parent");
             while next_child_index < next_declaration_child_index {
-                if is_region_end[next_child_index as usize] {
-                    indices_of_children_attached_after_declaration.push(next_child_index);
-                } else if is_child_attached_to_declaration[next_child_index as usize] {
+                if is_child_attached_to_declaration[next_child_index as usize] {
                     if let Some(following_attached_child) = parent.child(next_child_index) {
                         // This checks mainly for inline comments after the
                         // declaration. They appear as siblings in the AST even
@@ -646,6 +688,11 @@ fn get_builtin_virtual_priority(method_name: &str) -> u8 {
 }
 
 fn compare_reorder_items(left: &ReorderItem, right: &ReorderItem) -> std::cmp::Ordering {
+    let ordering_partition = left.region_partition.cmp(&right.region_partition);
+    if ordering_partition != std::cmp::Ordering::Equal {
+        return ordering_partition;
+    }
+
     let ordering_declaration_kind = (left.classification as u8).cmp(&(right.classification as u8));
     if ordering_declaration_kind != std::cmp::Ordering::Equal {
         return ordering_declaration_kind;