Merge #146

146: `#[itest(skip)]` + `#[itest(focus)]` r=Bromeon a=Bromeon Follow-up to #142. Two new test features: * `#[itest(skip)]` ignores the current test, and shows the number of skipped tests in a statistic at the end of the run. * This is better than empty tests with `// TODO` or commented-out tests, as it reminds you of the technical debt and gives an impression of its extents. * If at least one test is annotated with `#[itest(focus)]`, then _only_ "focused" tests are run. * Extremely helpful during debugging sessions, or when one is working on a very particular feature. * If Godot is invoked with `-- --disallow-focus`, focus runs will always fail (for CI, to avoid accidental disabling of tests). This is not yet implemented for GDScript. Might be worth it, or might not. Apart from that, this PR massively simplifies the internal APIs to parse `#[attribute(key, key2="value")]` attributes. Co-authored-by: Jan Haller <bromeon@gmail.com>
godot-rust · Mar 6, 2023 · 7d42ebb · 7d42ebb
2 parents 55c4d3a + 1a4a47a
commit 7d42ebb
Show file tree

Hide file tree

Showing 19 changed files with 392 additions and 241 deletions.
diff --git a/.github/composite/godot-install/action.yml b/.github/composite/godot-install/action.yml
@@ -10,7 +10,7 @@ inputs:
     required: true
     description: "Name of the compiled Godot artifact to download"
 
-  binary-filename:
+  godot-binary:
     required: true
     description: "Filename of the Godot executable"
 
@@ -25,7 +25,7 @@ runs:
       run: |
         runnerDir=$(echo "${{ runner.temp }}" | sed "s!\\\\!/!")
         echo "RUNNER_DIR=$runnerDir" >> $GITHUB_ENV
-        echo "GODOT4_BIN=$runnerDir/godot_bin/${{ inputs.binary-filename }}" >> $GITHUB_ENV
+        echo "GODOT4_BIN=$runnerDir/godot_bin/${{ inputs.godot-binary }}" >> $GITHUB_ENV
       shell: bash
 
 #    - name: "Check cache for installed Godot version"

diff --git a/.github/composite/godot-itest/action.yml b/.github/composite/godot-itest/action.yml
@@ -10,10 +10,15 @@ inputs:
     required: true
     description: "Name of the compiled Godot artifact to download"
 
-  binary-filename:
+  godot-binary:
     required: true
     description: "Filename of the Godot executable"
 
+  godot-args:
+    required: false
+    default: ''
+    description: "Command-line arguments passed to Godot"
+
   rust-toolchain:
     required: false
     default: 'stable'
@@ -26,8 +31,8 @@ inputs:
 
   with-llvm:
     required: false
-    description: "Set to 'true' if LLVM should be installed"
     default: ''
+    description: "Set to 'true' if LLVM should be installed"
 
 
 runs:
@@ -39,7 +44,7 @@ runs:
       uses: ./.github/composite/godot-install
       with:
         artifact-name: ${{ inputs.artifact-name }}
-        binary-filename: ${{ inputs.binary-filename }}
+        godot-binary: ${{ inputs.godot-binary }}
 
     # The chmod seems still necessary, although applied before uploading artifact. Possibly modes are not preserved.
     # The `| xargs` pattern trims the output, since printed version may contain extra newline, which causes problems in env vars.
@@ -94,7 +99,7 @@ runs:
       run: |
         cd itest/godot
         echo "OUTCOME=itest" >> $GITHUB_ENV
-        $GODOT4_BIN --headless 2>&1 | tee "${{ runner.temp }}/log.txt" | tee >(grep "SCRIPT ERROR:" -q && {
+        $GODOT4_BIN --headless ${{ inputs.godot-args }} 2>&1 | tee "${{ runner.temp }}/log.txt" | tee >(grep "SCRIPT ERROR:" -q && {
         	printf "\n -- Godot engine encountered error, abort...\n";
         	pkill godot
             echo "OUTCOME=godot-runtime" >> $GITHUB_ENV

diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml
@@ -57,7 +57,7 @@ jobs:
         uses: ./.github/composite/godot-install
         with:
           artifact-name: godot-linux
-          binary-filename: godot.linuxbsd.editor.dev.x86_64
+          godot-binary: godot.linuxbsd.editor.dev.x86_64
 
       - name: "Check clippy"
         run: |
@@ -126,7 +126,7 @@ jobs:
         uses: ./.github/composite/godot-install
         with:
           artifact-name: godot-${{ matrix.name }}
-          binary-filename: ${{ matrix.godot-binary }}
+          godot-binary: ${{ matrix.godot-binary }}
 
       - name: "Compile tests"
         run: cargo test $GDEXT_FEATURES --no-run
@@ -168,15 +168,19 @@ jobs:
           # Additionally, the Godot source is patched to make dlclose() a no-op, as unloading dynamic libraries loses stacktrace and
           # cause false positives like println!. See https://github.com/google/sanitizers/issues/89.
           # The gcc version can possibly be removed later, as it is slower and needs a larger artifact than the clang one.
+
+          # --disallow-focus: fail if #[itest(focus)] is encountered, to prevent running only a few tests for full CI
           - name: linux-memcheck-gcc
             os: ubuntu-20.04
             rust-toolchain: stable
             godot-binary: godot.linuxbsd.editor.dev.x86_64.san
+            godot-args: -- --disallow-focus
 
           - name: linux-memcheck-clang
             os: ubuntu-20.04
             rust-toolchain: stable
             godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
+            godot-args: -- --disallow-focus
 
     steps:
       - uses: actions/checkout@v3
@@ -185,7 +189,8 @@ jobs:
         uses: ./.github/composite/godot-itest
         with:
           artifact-name: godot-${{ matrix.name }}
-          binary-filename: ${{ matrix.godot-binary }}
+          godot-binary: ${{ matrix.godot-binary }}
+          godot-args: ${{ matrix.godot-args }}
           with-llvm: ${{ matrix.name == 'macos' }}
 
 

diff --git a/.github/workflows/minimal-ci.yml b/.github/workflows/minimal-ci.yml
@@ -57,7 +57,7 @@ jobs:
         uses: ./.github/composite/godot-install
         with:
           artifact-name: godot-linux
-          binary-filename: godot.linuxbsd.editor.dev.x86_64
+          godot-binary: godot.linuxbsd.editor.dev.x86_64
 
       - name: "Check clippy"
         run: |
@@ -78,7 +78,7 @@ jobs:
         uses: ./.github/composite/godot-install
         with:
           artifact-name: godot-linux
-          binary-filename: godot.linuxbsd.editor.dev.x86_64
+          godot-binary: godot.linuxbsd.editor.dev.x86_64
 
       - name: "Compile tests"
         run: cargo test $GDEXT_FEATURES --no-run
@@ -98,7 +98,7 @@ jobs:
         uses: ./.github/composite/godot-itest
         with:
           artifact-name: godot-linux
-          binary-filename: godot.linuxbsd.editor.dev.x86_64
+          godot-binary: godot.linuxbsd.editor.dev.x86_64
           #godot_ver: ${{ matrix.godot }}
 
 

diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs
@@ -151,6 +151,7 @@ pub fn default_call_error() -> GDExtensionCallError {
 
 #[doc(hidden)]
 #[inline]
+#[track_caller] // panic message points to call site
 pub fn panic_on_call_error(err: &GDExtensionCallError) {
     let actual = err.error;
 

diff --git a/godot-macros/src/derive_godot_class.rs b/godot-macros/src/derive_godot_class.rs
@@ -4,16 +4,13 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use crate::util::{bail, ensure_kv_empty, ident, parse_kv_group, path_is_single, KvMap, KvValue};
-use crate::{util, ParseResult};
-use proc_macro2::{Ident, Punct, Span, TokenStream};
-use quote::spanned::Spanned;
+use crate::util::{bail, ident, KvParser};
+use crate::ParseResult;
+use proc_macro2::{Ident, Punct, TokenStream};
 use quote::{format_ident, quote};
-use venial::{Attribute, NamedField, Struct, StructFields, TyExpr};
-
-pub fn transform(input: TokenStream) -> ParseResult<TokenStream> {
-    let decl = venial::parse_declaration(input)?;
+use venial::{Declaration, NamedField, Struct, StructFields, TyExpr};
 
+pub fn transform(decl: Declaration) -> ParseResult<TokenStream> {
     let class = decl
         .as_struct()
         .ok_or_else(|| venial::Error::new("Not a valid struct"))?;
@@ -68,43 +65,24 @@ pub fn transform(input: TokenStream) -> ParseResult<TokenStream> {
 
 /// Returns the name of the base and the default mode
 fn parse_struct_attributes(class: &Struct) -> ParseResult<ClassAttributes> {
-    let mut base = ident("RefCounted");
-    //let mut new_mode = GodotConstructMode::AutoGenerated;
+    let mut base_ty = ident("RefCounted");
     let mut has_generated_init = false;
 
     // #[func] attribute on struct
-    if let Some((span, mut map)) = parse_class_attr(&class.attributes)? {
-        //println!(">>> CLASS {class}, MAP: {map:?}", class = class.name);
-
-        if let Some(kv_value) = map.remove("base") {
-            if let KvValue::Ident(override_base) = kv_value {
-                base = override_base;
-            } else {
-                bail("Invalid value for 'base' argument", span)?;
-            }
+    if let Some(mut parser) = KvParser::parse(&class.attributes, "class")? {
+        if let Some(base) = parser.handle_ident("base")? {
+            base_ty = base;
         }
 
-        /*if let Some(kv_value) = map.remove("new") {
-            match kv_value {
-                KvValue::Ident(ident) if ident == "fn" => new_mode = GodotConstructMode::FnNew,
-                KvValue::Ident(ident) if ident == "none" => new_mode = GodotConstructMode::None,
-                _ => bail(
-                    "Invalid value for 'new' argument; must be 'fn' or 'none'",
-                    span,
-                )?,
-            }
-        }*/
-        if let Some(kv_value) = map.remove("init") {
-            match kv_value {
-                KvValue::None => has_generated_init = true,
-                _ => bail("Argument 'init' must not have a value", span)?,
-            }
+        if parser.handle_alone("init")? {
+            has_generated_init = true;
         }
-        ensure_kv_empty(map, span)?;
+
+        parser.finish()?;
     }
 
     Ok(ClassAttributes {
-        base_ty: base,
+        base_ty,
         has_generated_init,
     })
 }
@@ -130,34 +108,27 @@ fn parse_fields(class: &Struct) -> ParseResult<Fields> {
     for (field, _punct) in fields {
         let mut is_base = false;
 
-        // #[base] or #[export]
-        for attr in field.attributes.iter() {
-            if let Some(path) = attr.get_single_path_segment() {
-                if path == "base" {
-                    is_base = true;
-                    if let Some(prev_base) = base_field {
-                        bail(
-                            format!(
-                                "#[base] allowed for at most 1 field, already applied to '{}'",
-                                prev_base.name
-                            ),
-                            attr,
-                        )?;
-                    }
-                    base_field = Some(Field::new(&field))
-                } else if path == "export" {
-                    match parse_kv_group(&attr.value) {
-                        Ok(export_kv) => {
-                            let exported_field =
-                                ExportedField::new_from_kv(Field::new(&field), attr, export_kv)?;
-                            exported_fields.push(exported_field);
-                        }
-                        Err(error) => {
-                            return Err(error);
-                        }
-                    }
-                }
+        // #[base]
+        if let Some(parser) = KvParser::parse(&field.attributes, "base")? {
+            if let Some(prev_base) = base_field {
+                bail(
+                    format!(
+                        "#[base] allowed for at most 1 field, already applied to '{}'",
+                        prev_base.name
+                    ),
+                    parser.span(),
+                )?;
             }
+            is_base = true;
+            base_field = Some(Field::new(&field));
+            parser.finish()?;
+        }
+
+        // #[export]
+        if let Some(mut parser) = KvParser::parse(&field.attributes, "export")? {
+            let exported_field = ExportedField::new_from_kv(Field::new(&field), &mut parser)?;
+            exported_fields.push(exported_field);
+            parser.finish()?;
         }
 
         // Exported or Rust-only fields
@@ -173,26 +144,6 @@ fn parse_fields(class: &Struct) -> ParseResult<Fields> {
     })
 }
 
-/// Parses a `#[class(...)]` attribute
-fn parse_class_attr(attributes: &[Attribute]) -> ParseResult<Option<(Span, KvMap)>> {
-    let mut godot_attr = None;
-    for attr in attributes.iter() {
-        let path = &attr.path;
-        if path_is_single(path, "class") {
-            if godot_attr.is_some() {
-                bail(
-                    "Only one #[class] attribute per item (struct, fn, ...) allowed",
-                    attr,
-                )?;
-            }
-
-            let map = util::parse_kv_group(&attr.value)?;
-            godot_attr = Some((attr.__span(), map));
-        }
-    }
-    Ok(godot_attr)
-}
-
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // General helpers
 
@@ -229,16 +180,10 @@ struct ExportedField {
 }
 
 impl ExportedField {
-    pub fn new_from_kv(
-        field: Field,
-        attr: &Attribute,
-        mut map: KvMap,
-    ) -> ParseResult<ExportedField> {
-        let getter = Self::require_key_value(&mut map, "getter", attr)?;
-        let setter = Self::require_key_value(&mut map, "setter", attr)?;
-        let variant_type = Self::require_key_value(&mut map, "variant_type", attr)?;
-
-        ensure_kv_empty(map, attr.__span())?;
+    pub fn new_from_kv(field: Field, parser: &mut KvParser) -> ParseResult<ExportedField> {
+        let getter = parser.handle_lit_required("getter")?;
+        let setter = parser.handle_lit_required("setter")?;
+        let variant_type = parser.handle_lit_required("variant_type")?;
 
         Ok(ExportedField {
             field,
@@ -247,21 +192,6 @@ impl ExportedField {
             variant_type,
         })
     }
-
-    fn require_key_value(map: &mut KvMap, key: &str, attr: &Attribute) -> ParseResult<String> {
-        if let Some(value) = map.remove(key) {
-            if let KvValue::Lit(value) = value {
-                Ok(value)
-            } else {
-                bail(
-                    format!("#[export] attribute {key} with a non-literal variant_type",),
-                    attr,
-                )?
-            }
-        } else {
-            bail(format!("#[export] attribute without a {key}"), attr)
-        }
-    }
 }
 
 fn make_godot_init_impl(class_name: &Ident, fields: Fields) -> TokenStream {

diff --git a/godot-macros/src/gdextension.rs b/godot-macros/src/gdextension.rs
@@ -9,15 +9,7 @@ use proc_macro2::TokenStream;
 use quote::quote;
 use venial::Declaration;
 
-pub fn transform(meta: TokenStream, input: TokenStream) -> Result<TokenStream, venial::Error> {
-    // Hack because venial doesn't support direct meta parsing yet
-    let input = quote! {
-        #[gdextension(#meta)]
-        #input
-    };
-
-    let decl = venial::parse_declaration(input)?;
-
+pub fn transform(decl: Declaration) -> Result<TokenStream, venial::Error> {
     let mut impl_decl = match decl {
         Declaration::Impl(item) => item,
         _ => return bail("#[gdextension] can only be applied to trait impls", &decl),

diff --git a/godot-macros/src/godot_api.rs b/godot-macros/src/godot_api.rs
@@ -13,8 +13,7 @@ use venial::{AttributeValue, Declaration, Error, Function, Impl, ImplMember};
 // Note: keep in sync with trait GodotExt
 const VIRTUAL_METHOD_NAMES: [&str; 3] = ["ready", "process", "physics_process"];
 
-pub fn transform(input: TokenStream) -> Result<TokenStream, Error> {
-    let input_decl = venial::parse_declaration(input)?;
+pub fn transform(input_decl: Declaration) -> Result<TokenStream, Error> {
     let decl = match input_decl {
         Declaration::Impl(decl) => decl,
         _ => bail(