Rust: address review

Author: Paolo Tranquilli

Cargo.lock

@@ -30,4 +30,3 @@ codeql-extractor = { path = "../../shared/tree-sitter-extractor" }
 rust-extractor-macros = { path = "macros" }
 itertools = "0.13.0"
 glob = "0.3.1"
-gag = "1.0.0"

rust/extractor/Cargo.toml

@@ -6,8 +6,8 @@ use quote::{format_ident, quote};
 pub fn extractor_cli_config(_attr: TokenStream, item: TokenStream) -> TokenStream {
     let ast = syn::parse_macro_input!(item as syn::ItemStruct);
     let name = &ast.ident;
-    let new_name = format_ident!("Cli{}", name);
-    let fields: Vec<_> = ast
+    let cli_name = format_ident!("Cli{}", name);
+    let cli_fields = ast
         .fields
         .iter()
         .map(|f| {
@@ -39,17 +39,42 @@ pub fn extractor_cli_config(_attr: TokenStream, item: TokenStream) -> TokenStrea
                 }
             }
         })
-        .collect();
+        .collect::<Vec<_>>();
+    let debug_fields = ast
+        .fields
+        .iter()
+        .map(|f| {
+            let id = f.ident.as_ref().unwrap();
+            if id == &format_ident!("inputs") {
+                quote! {
+                    .field("number of inputs", &self.#id.len())
+                }
+            } else {
+                quote! {
+                    .field(stringify!(#id), &self.#id)
+                }
+            }
+        })
+        .collect::<Vec<_>>();
+
     let gen = quote! {
         #[serde_with::apply(_ => #[serde(default)])]
-        #[derive(Debug, Deserialize, Default)]
+        #[derive(Deserialize, Default)]
         #ast
 
+        impl Debug for #name {
+            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                f.debug_struct("configuration:")
+                    #(#debug_fields)*
+                    .finish()
+            }
+        }
+
         #[serde_with::skip_serializing_none]
         #[derive(clap::Parser, Serialize)]
         #[command(about, long_about = None)]
-        struct #new_name {
-            #(#fields)*
+        struct #cli_name {
+            #(#cli_fields)*
         }
     };
     gen.into()

rust/extractor/macros/src/lib.rs

@@ -10,6 +10,7 @@ use itertools::Itertools;
 use num_traits::Zero;
 use rust_extractor_macros::extractor_cli_config;
 use serde::{Deserialize, Serialize};
+use std::fmt::Debug;
 use std::ops::Not;
 use std::path::PathBuf;
 
@@ -36,7 +37,6 @@ pub struct Config {
     pub scratch_dir: PathBuf,
     pub trap_dir: PathBuf,
     pub source_archive_dir: PathBuf,
-    pub log_dir: PathBuf,
     pub extract_dependencies: bool,
     pub verbose: u8,
     pub compression: Compression,
@@ -55,7 +55,7 @@ impl Config {
             .merge(Env::prefixed("CODEQL_EXTRACTOR_RUST_"))
             .merge(Env::prefixed("CODEQL_EXTRACTOR_RUST_OPTION_"))
             .merge(Serialized::defaults(cli_args));
-        if matches!(figment.find_value("qltest"), Ok(Value::Bool(_, true))) {
+        if let Ok(Value::Bool(_, true)) = figment.find_value("qltest") {
             let cwd = std::env::current_dir()?;
             let mut option_files = cwd
                 .ancestors()

rust/extractor/src/config.rs

@@ -4,8 +4,6 @@ use log::info;
 use ra_ap_ide_db::line_index::{LineCol, LineIndex};
 use ra_ap_project_model::ProjectManifest;
 use rust_analyzer::{ParseResult, RustAnalyzer};
-use std::fs::File;
-use std::io::{BufRead, BufReader};
 use std::{
     collections::HashMap,
     path::{Path, PathBuf},
@@ -69,15 +67,16 @@ fn extract(
     });
 }
 
-fn run_extractor(mut cfg: config::Config) -> anyhow::Result<()> {
+fn main() -> anyhow::Result<()> {
+    let mut cfg = config::Config::extract().context("failed to load configuration")?;
     stderrlog::new()
         .module(module_path!())
-        .verbosity(cfg.verbose as usize)
+        .verbosity(2 + cfg.verbose as usize)
         .init()?;
     if cfg.qltest {
         qltest::prepare(&mut cfg)?;
     }
-    info!("configuration: {cfg:#?}\n");
+    info!("{cfg:#?}\n");
 
     let traps = trap::TrapFileProvider::new(&cfg).context("failed to set up trap files")?;
     let archiver = archive::Archiver {
@@ -125,19 +124,3 @@ fn run_extractor(mut cfg: config::Config) -> anyhow::Result<()> {
 
     Ok(())
 }
-
-fn main() -> anyhow::Result<()> {
-    let cfg = config::Config::extract().context("failed to load configuration")?;
-    let qltest = cfg.qltest;
-    let qltest_log = cfg.log_dir.join("qltest.log");
-    let result = std::panic::catch_unwind(|| run_extractor(cfg));
-    if qltest && matches!(result, Err(_) | Ok(Err(_))) {
-        // in case of failure, print out the full log
-        let log = File::open(qltest_log).context("opening qltest.log")?;
-        let reader = BufReader::new(log);
-        for line in reader.lines() {
-            println!("{}", line.context("reading qltest.log")?);
-        }
-    }
-    result.unwrap()
-}

rust/extractor/src/main.rs

@@ -3,6 +3,7 @@ use anyhow::Context;
 use glob::glob;
 use itertools::Itertools;
 use log::info;
+use std::ffi::OsStr;
 use std::fs;
 use std::process::Command;
 
@@ -13,8 +14,9 @@ fn dump_lib() -> anyhow::Result<()> {
         .context("fetching test sources")?;
     let lib = paths
         .iter()
-        .filter(|p| !["lib.rs", "main.rs"].contains(&p.file_name().unwrap().to_str().unwrap()))
-        .map(|p| format!("mod {};", p.file_stem().unwrap().to_str().unwrap()))
+        .map(|p| p.file_stem().expect("results of glob must have a name"))
+        .filter(|&p| !["main", "lib"].map(OsStr::new).contains(&p))
+        .map(|p| format!("mod {};", p.to_string_lossy()))
         .join("\n");
     fs::write("lib.rs", lib).context("writing lib.rs")
 }
@@ -49,21 +51,7 @@ fn set_sources(config: &mut Config) -> anyhow::Result<()> {
     Ok(())
 }
 
-fn redirect_output(config: &Config) -> anyhow::Result<()> {
-    let log_path = config.log_dir.join("qltest.log");
-    let log = fs::OpenOptions::new()
-        .append(true)
-        .create(true)
-        .open(&log_path)
-        .context("opening qltest.log")?;
-    Box::leak(Box::new(
-        gag::Redirect::stderr(log).context("redirecting stderr")?,
-    ));
-    Ok(())
-}
-
 pub(crate) fn prepare(config: &mut Config) -> anyhow::Result<()> {
-    redirect_output(config)?;
     dump_lib()?;
     set_sources(config)?;
     dump_cargo_manifest()?;

rust/extractor/src/qltest.rs

@@ -1,7 +1,11 @@
 @echo off
 
 set "RUST_BACKTRACE=full"
+set "QLTEST_LOG=%CODEQL_EXTRACTOR_RUST_LOG_DIR%/qltest.log"
 
-type NUL && "%CODEQL_EXTRACTOR_RUST_ROOT%/tools/%CODEQL_PLATFORM%/extractor" --qltest
+type NUL && "%CODEQL_EXTRACTOR_RUST_ROOT%/tools/%CODEQL_PLATFORM%/extractor" --qltest >"%QLTEST_LOG%"
 
-exit /b %ERRORLEVEL%
+if %ERRORLEVEL% neq 0 (
+    type "%QLTEST_LOG%"
+    exit /b 1
+)

rust/ql/test/library-tests/variables/options

@@ -3,5 +3,8 @@
 set -eu
 
 export RUST_BACKTRACE=full
-
-"$CODEQL_EXTRACTOR_RUST_ROOT/tools/$CODEQL_PLATFORM/extractor" --qltest
+QLTEST_LOG="$CODEQL_EXTRACTOR_RUST_LOG_DIR"/qltest.log
+if ! "$CODEQL_EXTRACTOR_RUST_ROOT/tools/$CODEQL_PLATFORM/extractor" --qltest &>> "$QLTEST_LOG"; then
+  cat "$QLTEST_LOG"
+  exit 1
+fi