Skip to content

Commit

Permalink
Reorder compile errors for #[derive(GodotClass)]
Browse files Browse the repository at this point in the history
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 {}
```
  • Loading branch information
fpdotmonkey committed Jun 22, 2024
1 parent cd31a83 commit f0856b3
Showing 1 changed file with 33 additions and 20 deletions.
53 changes: 33 additions & 20 deletions godot-macros/src/class/derive_godot_class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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 {
Expand Down

0 comments on commit f0856b3

Please sign in to comment.