Report error locus within instrumented code (#170)

* Add report locus test

* Give Function a higher precedence than Impl when parsing

* Let rust parse impl automatically

Letting Rust parse the impl block allows us to keep the same TokenStream as input in the autometrics macro code, even when async_trait is involved.

Keeping the original TokenStream keeps the Span information in method/function bodies so error reporting in instrumented code is not absorbed in "#[autometrics]" span.

This also simplifies the parsing code a little bit.

Author: gagbo

CHANGELOG.md

@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Update `http` to `1.0`. This fixes compatibility with `axum 0.7` (#167)
 - Explicitly set default timeout and period for the OTEL push exporter (#168)
+- Fix a regression that made all compilation errors in instrumented code appear as located in
+  the `#[autometrics]` annotation instead of the original location (#170)
 
 ## [1.0.0](https://github.com/autometrics-dev/autometrics-rs/releases/tag/v1.0.0) - 2023-12-01
 

autometrics-macros/src/lib.rs

@@ -25,11 +25,11 @@ pub fn autometrics(
 ) -> proc_macro::TokenStream {
     let args = parse_macro_input!(args as AutometricsArgs);
 
-    let (async_trait, item) = check_async_trait(item);
+    let async_trait = check_async_trait(&item);
     let item = parse_macro_input!(item as Item);
 
     let result = match item {
-        Item::Function(item) => instrument_function(&args, item, &None),
+        Item::Function(item) => instrument_function(&args, item, None),
         Item::Impl(item) => instrument_impl_block(&args, item, &async_trait),
     };
 
@@ -41,22 +41,15 @@ pub fn autometrics(
     output.into()
 }
 
-/// returns a tuple of two containing:
-/// - `async_trait` attributes that have to be re-added after our instrumentation magic has been added
-/// - `input` but without the `async_trait` attributes
-fn check_async_trait(input: proc_macro::TokenStream) -> (String, proc_macro::TokenStream) {
+/// returns the `async_trait` attributes that have to be re-added after our instrumentation magic has been added
+fn check_async_trait(input: &proc_macro::TokenStream) -> String {
     let regex = Regex::new(r#"#\[[^\]]*async_trait\]"#)
         .expect("The regex is hardcoded and thus guaranteed to be successfully parseable");
 
     let original = input.to_string();
-
     let attributes: Vec<_> = regex.find_iter(&original).map(|m| m.as_str()).collect();
-    let replaced = regex.replace_all(&original, "");
-
-    // .unwrap is safe because we only remove tokens from the existing stream, we dont add new ones
-    let ts = proc_macro::TokenStream::from_str(replaced.as_ref()).unwrap();
 
-    (attributes.join("\n"), ts)
+    attributes.join("\n")
 }
 
 #[proc_macro_derive(ResultLabels, attributes(label))]
@@ -71,7 +64,7 @@ pub fn result_labels(input: proc_macro::TokenStream) -> proc_macro::TokenStream
 fn instrument_function(
     args: &AutometricsArgs,
     item: ItemFn,
-    struct_name: &Option<String>,
+    struct_name: Option<&str>,
 ) -> Result<TokenStream> {
     let sig = item.sig;
     let block = item.block;
@@ -364,7 +357,7 @@ fn instrument_impl_block(
                     sig: method.sig,
                     block: Box::new(method.block),
                 };
-                let tokens = match instrument_function(args, item_fn, &struct_name) {
+                let tokens = match instrument_function(args, item_fn, struct_name.as_deref()) {
                     Ok(tokens) => tokens,
                     Err(err) => err.to_compile_error(),
                 };

autometrics-macros/src/parse.rs

@@ -19,12 +19,10 @@ pub(crate) enum Item {
 
 impl Parse for Item {
     fn parse(input: ParseStream) -> Result<Self> {
-        let lookahead = input.lookahead1();
-        if lookahead.peek(Token![impl]) {
-            input.parse().map(Item::Impl)
-        } else {
-            input.parse().map(Item::Function)
-        }
+        input
+            .parse()
+            .map(Item::Function)
+            .or_else(|_| input.parse().map(Item::Impl))
     }
 }
 

autometrics/tests/compilation.rs

@@ -0,0 +1,13 @@
+//! Tests relying on macros or compiler diagnostics
+
+#[test]
+fn harness() {
+    let t = trybuild::TestCases::new();
+
+    // Test the ResultLabels macro
+    t.pass("tests/compilation/result_labels/pass/*.rs");
+    t.compile_fail("tests/compilation/result_labels/fail/*.rs");
+
+    // Test that compiler reports errors in the correct location
+    t.compile_fail("tests/compilation/error_locus/fail/*.rs");
+}

autometrics/tests/compilation/error_locus/fail/report_original_line.rs

@@ -0,0 +1,15 @@
+// This test ensures that when an instrumented function has a compilation error,
+// then the error is reported at the correct line in the original code.
+use autometrics::autometrics;
+
+#[autometrics]
+fn bad_function() {
+    // This vec is not mut
+    let contents: Vec<u32> = Vec::new();
+
+    contents.push(2);
+}
+
+fn main() {
+    bad_function();
+}

autometrics/tests/compilation/error_locus/fail/report_original_line.stderr

@@ -0,0 +1,10 @@
+error[E0596]: cannot borrow `contents` as mutable, as it is not declared as mutable
+  --> tests/compilation/error_locus/fail/report_original_line.rs:10:5
+   |
+10 |     contents.push(2);
+   |     ^^^^^^^^ cannot borrow as mutable
+   |
+help: consider changing this to be mutable
+   |
+8  |     let mut contents: Vec<u32> = Vec::new();
+   |         +++

autometrics/tests/result_labels/fail/wrong_attribute.rs renamed to autometrics/tests/compilation/result_labels/fail/wrong_attribute.rs

@@ -1,5 +1,5 @@
 error: Only `label(result = "RES")` (RES can be "ok" or "error") is supported
-  --> tests/result_labels/fail/wrong_attribute.rs:11:7
+  --> tests/compilation/result_labels/fail/wrong_attribute.rs:11:7
    |
 11 |     #[label]
    |       ^^^^^

autometrics/tests/result_labels/fail/wrong_attribute.stderr renamed to autometrics/tests/compilation/result_labels/fail/wrong_attribute.stderr

@@ -1,5 +1,5 @@
 error: Only `label(result = "RES")` (RES can be "ok" or "error") is supported
-  --> tests/result_labels/fail/wrong_kv_attribute.rs:11:7
+  --> tests/compilation/result_labels/fail/wrong_kv_attribute.rs:11:7
    |
 11 |     #[label = "error"]
    |       ^^^^^^^^^^^^^^^

autometrics/tests/result_labels/fail/wrong_kv_attribute.rs renamed to autometrics/tests/compilation/result_labels/fail/wrong_kv_attribute.rs

@@ -1,5 +1,5 @@
 error: Only `result = "RES"` (RES can be "ok" or "error") is supported
-  --> tests/result_labels/fail/wrong_result_name.rs:11:13
+  --> tests/compilation/result_labels/fail/wrong_result_name.rs:11:13
    |
 11 |     #[label(unknown = "ok")]
    |             ^^^^^^^

autometrics/tests/result_labels/fail/wrong_kv_attribute.stderr renamed to autometrics/tests/compilation/result_labels/fail/wrong_kv_attribute.stderr

@@ -1,5 +1,5 @@
 error: Only "ok" or "error" are accepted as result values
-  --> tests/result_labels/fail/wrong_variant.rs:12:22
+  --> tests/compilation/result_labels/fail/wrong_variant.rs:12:22
    |
 12 |     #[label(result = "not ok")]
    |                      ^^^^^^^^

autometrics/tests/result_labels/fail/wrong_result_name.rs renamed to autometrics/tests/compilation/result_labels/fail/wrong_result_name.rs

@@ -27,7 +27,7 @@ impl TestTrait for TestStruct {
 fn main() {
     let ts = TestStruct::default();
 
-    async move {
+    let _ = async move {
         <TestStruct as TestTrait>::method().await;
         ts.self_method().await;
     };