From f0856b3dcdebab35d5b96c1567d87cdb6627807d Mon Sep 17 00:00:00 2001
From: Fletcher Porter <me@fletcherporter.com>
Date: Fri, 21 Jun 2024 21:26:10 +0300
Subject: [PATCH] Reorder compile errors for `#[derive(GodotClass)]`

This addresses an issue where relatively major compile errors would be
shown only after smaller errors were fixed.  This is problematic since
fixing a large error has a high chance of invalidating the work for the
small error.

In my eyes, the error priority goes with each of these, in order.
1. Scale - if the error requires a larger change
2. Locality - if the error requires changes away from the error site
3. Complexity - if the change for the error carries a lot of meaning

There were two main cases addressed, unsupported tuple structs and
editor plugins.  Replacing a tuple struct with a regular struct is a
very large change.  It would require rewriting the struct definition and
possibly also distant trait definitions.  Because of the large scale, I
made the lack of tuple struct support the highest priority error.
Editor plugins can error a lot because the `class` macro needs all three
of `editor_plugin`, `tool`, and `base=EditorPlugin`.  I prioritized the
message for `base` because it is complex, implementing `WithBaseField`
for the struct, and could also involve non-local changes.  `tool` is
easy to fix, so doesn't need priority.

Since this isn't easily testable, here's a couple cases that gave errors
in the wrong order before.

```rs
// fix the tuple struct, and then an error would occur on rename
\#[derive(GodotClass)]
\#[class(rename = "Not an identifier")]
struct TupleStructErrorsBeforeRename(String);

// the error is no base, then no tool, and last nonsense is invalid
\#[derive(GodotClass)]
\#[class(editor_plugin, nonsense)]
struct EditorPluginNoBaseErrorsBeforeNoTool {}
```
---
 godot-macros/src/class/derive_godot_class.rs | 53 ++++++++++++--------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/godot-macros/src/class/derive_godot_class.rs b/godot-macros/src/class/derive_godot_class.rs
index 0b65d4871..513ae7e90 100644
--- a/godot-macros/src/class/derive_godot_class.rs
+++ b/godot-macros/src/class/derive_godot_class.rs
@@ -20,8 +20,9 @@ pub fn derive_godot_class(item: venial::Item) -> ParseResult<TokenStream> {
         .as_struct()
         .ok_or_else(|| venial::Error::new("Not a valid struct"))?;
 
+    let named_fields = named_fields(class)?;
     let struct_cfg = parse_struct_attributes(class)?;
-    let fields = parse_fields(class, struct_cfg.init_strategy)?;
+    let fields = parse_fields(named_fields, struct_cfg.init_strategy)?;
 
     let class_name = &class.name;
     let class_name_str: String = struct_cfg
@@ -292,9 +293,6 @@ fn parse_struct_attributes(class: &venial::Struct) -> ParseResult<ClassAttribute
             base_ty = base;
         }
 
-        // #[class(rename = NewName)]
-        rename = parser.handle_ident("rename")?;
-
         // #[class(init)], #[class(no_init)]
         match handle_opposite_keys(&mut parser, "init", "class")? {
             Some(true) => init_strategy = InitStrategy::Generated,
@@ -314,20 +312,26 @@ fn parse_struct_attributes(class: &venial::Struct) -> ParseResult<ClassAttribute
             is_editor_plugin = true;
 
             // Requires #[class(tool, base=EditorPlugin)].
-            if !is_tool {
+	    // The base=EditorPlugin check should come first to create the best
+	    // compile errors since it's more complex to resolve.
+	    // See https://github.com/godot-rust/gdext/pull/773
+            if base_ty != ident("EditorPlugin") {
                 return bail!(
                     attr_key,
-                    "#[class(editor_plugin)] requires additional key `tool`"
+                    "#[class(editor_plugin)] requires additional key-value `base=EditorPlugin`"
                 );
             }
-            if base_ty != ident("EditorPlugin") {
+            if !is_tool {
                 return bail!(
                     attr_key,
-                    "#[class(editor_plugin)] requires additional key-value `base=EditorPlugin`"
+                    "#[class(editor_plugin)] requires additional key `tool`"
                 );
             }
         }
 
+        // #[class(rename = NewName)]
+        rename = parser.handle_ident("rename")?;
+
         // #[class(hidden)]
         // TODO consider naming this "internal"; godot-cpp uses that terminology:
         // https://github.com/godotengine/godot-cpp/blob/master/include/godot_cpp/core/class_db.hpp#L327
@@ -349,22 +353,31 @@ fn parse_struct_attributes(class: &venial::Struct) -> ParseResult<ClassAttribute
     })
 }
 
-/// Returns field names and 1 base field, if available
-fn parse_fields(class: &venial::Struct, init_strategy: InitStrategy) -> ParseResult<Fields> {
-    let mut all_fields = vec![];
-    let mut base_field = Option::<Field>::None;
-    let mut has_deprecated_base = false;
-
-    let named_fields: Vec<(venial::NamedField, Punct)> = match &class.fields {
-        venial::Fields::Unit => {
-            vec![]
-        }
+/// Fetches data for all named fields for a struct.
+///
+/// Errors if `class` is a tuple struct.
+fn named_fields(class: &venial::Struct) -> ParseResult<Vec<(venial::NamedField, Punct)>> {
+    // This is separate from parse_fields to improve compile errors.  The
+    // errors from here demand larger and more non-local changes from the API
+    // user than those from parse_struct_attributes, so this must be run first.
+    match &class.fields {
+        venial::Fields::Unit => Ok(vec![]),
         venial::Fields::Tuple(_) => bail!(
             &class.fields,
             "#[derive(GodotClass)] not supported for tuple structs",
         )?,
-        venial::Fields::Named(fields) => fields.fields.inner.clone(),
-    };
+        venial::Fields::Named(fields) => Ok(fields.fields.inner.clone()),
+    }
+}
+
+/// Returns field names and 1 base field, if available.
+fn parse_fields(
+    named_fields: Vec<(venial::NamedField, Punct)>,
+    init_strategy: InitStrategy,
+) -> ParseResult<Fields> {
+    let mut all_fields = vec![];
+    let mut base_field = Option::<Field>::None;
+    let mut has_deprecated_base = false;
 
     // Attributes on struct fields
     for (named_field, _punct) in named_fields {