fix: address variant alias review follow-ups

Author: GiggleLiu

problemreductions-cli/src/commands/graph.rs

@@ -57,9 +57,13 @@ pub fn list(out: &OutputConfig) -> Result<()> {
                     .unwrap_or_default();
             let mut parts: Vec<String> = Vec::new();
             if i == 0 {
-                parts.extend(problem_aliases.iter().map(|s| s.to_string()));
+                for alias in &problem_aliases {
+                    push_alias_part(&mut parts, alias);
+                }
+            }
+            for alias in &variant_aliases {
+                push_alias_part(&mut parts, alias);
             }
-            parts.extend(variant_aliases.iter().map(|s| s.to_string()));
 
             rows_data.push(VariantRow {
                 display,
@@ -719,6 +723,12 @@ pub fn export(out: &OutputConfig) -> Result<()> {
     out.emit_with_default_name("reduction_graph.json", &text, &json)
 }
 
+fn push_alias_part(parts: &mut Vec<String>, alias: &str) {
+    if !parts.iter().any(|part| part.eq_ignore_ascii_case(alias)) {
+        parts.push(alias.to_string());
+    }
+}
+
 fn parse_direction(s: &str) -> Result<TraversalFlow> {
     match s {
         "out" => Ok(TraversalFlow::Outgoing),
@@ -809,3 +819,20 @@ fn render_tree(graph: &ReductionGraph, nodes: &[NeighborTree], text: &mut String
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::push_alias_part;
+
+    #[test]
+    fn push_alias_part_deduplicates_case_insensitively_in_order() {
+        let mut parts = Vec::new();
+        push_alias_part(&mut parts, "KSAT");
+        push_alias_part(&mut parts, "3SAT");
+        push_alias_part(&mut parts, "ksat");
+        push_alias_part(&mut parts, "2SAT");
+        push_alias_part(&mut parts, "3sat");
+
+        assert_eq!(parts, vec!["KSAT", "3SAT", "2SAT"]);
+    }
+}

problemreductions-cli/src/problem_name.rs

@@ -14,8 +14,12 @@ pub struct ProblemSpec {
 ///
 /// Searches both variant-level aliases (e.g., `"3SAT"` → `KSatisfiability`) and
 /// problem-level aliases (e.g., `"MIS"` → `MaximumIndependentSet`). When a
-/// variant-level alias is matched, only the canonical name is returned here;
-/// use [`parse_problem_spec`] to also recover the variant tokens.
+/// variant-level alias is matched, only the canonical name is returned here.
+/// The older pass-through behavior where `3SAT` resolved to `"3SAT"` has been
+/// intentionally replaced by `3SAT` resolving to `"KSatisfiability"` so aliases
+/// behave consistently. Callers that need variant semantics such as
+/// `3SAT` → `KSatisfiability { k = K3 }` should use [`parse_problem_spec`] or
+/// [`resolve_problem_ref`].
 pub fn resolve_alias(input: &str) -> String {
     if input.eq_ignore_ascii_case("UndirectedFlowLowerBounds") {
         return "UndirectedFlowLowerBounds".to_string();
@@ -418,6 +422,58 @@ mod tests {
         assert_eq!(spec.variant_values, vec!["K2"]);
     }
 
+    #[test]
+    fn test_resolve_problem_ref_variant_alias_3sat() {
+        let graph = problemreductions::rules::ReductionGraph::new();
+        let expected = ProblemRef {
+            name: "KSatisfiability".to_string(),
+            variant: BTreeMap::from([("k".to_string(), "K3".to_string())]),
+        };
+
+        assert_eq!(resolve_problem_ref("3SAT", &graph).unwrap(), expected);
+    }
+
+    #[test]
+    fn test_resolve_problem_ref_variant_alias_2sat() {
+        let graph = problemreductions::rules::ReductionGraph::new();
+        let expected = ProblemRef {
+            name: "KSatisfiability".to_string(),
+            variant: BTreeMap::from([("k".to_string(), "K2".to_string())]),
+        };
+
+        assert_eq!(resolve_problem_ref("2SAT", &graph).unwrap(), expected);
+    }
+
+    #[test]
+    fn test_resolve_problem_ref_3sat_k2_rejects_duplicate_dimension() {
+        let spec = parse_problem_spec("3SAT/K2").unwrap();
+        assert_eq!(spec.name, "KSatisfiability");
+        assert_eq!(spec.variant_values, vec!["K3", "K2"]);
+
+        let graph = problemreductions::rules::ReductionGraph::new();
+        let err = resolve_problem_ref("3SAT/K2", &graph).unwrap_err();
+        assert!(
+            err.to_string().contains("specified more than once"),
+            "expected duplicate-dimension error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn test_resolve_problem_ref_3sat_simple_graph_rejects_unknown_token() {
+        let spec = parse_problem_spec("3SAT/SimpleGraph").unwrap();
+        assert_eq!(spec.name, "KSatisfiability");
+        assert_eq!(spec.variant_values, vec!["K3", "SimpleGraph"]);
+
+        let graph = problemreductions::rules::ReductionGraph::new();
+        let err = resolve_problem_ref("3SAT/SimpleGraph", &graph).unwrap_err();
+        assert!(
+            err.to_string()
+                .to_lowercase()
+                .contains("unknown variant token"),
+            "expected unknown-token error, got: {err}"
+        );
+    }
+
     #[test]
     fn test_parse_problem_spec_max2sat_problem_level() {
         // MAX2SAT is a problem-level alias on Maximum2Satisfiability (standalone problem,

problemreductions-macros/src/lib.rs

@@ -468,20 +468,32 @@ impl syn::parse::Parse for DeclareVariantsInput {
 
             // Optional: `aliases ["X", "Y", ...]`
             let aliases = if input.peek(syn::Ident) {
-                let ident: syn::Ident = input.fork().parse()?;
+                let fork = input.fork();
+                let ident: syn::Ident = fork.parse()?;
                 if ident == "aliases" {
                     input.parse::<syn::Ident>()?;
                     let content;
                     syn::bracketed!(content in input);
                     let mut out = Vec::new();
                     while !content.is_empty() {
                         let lit: syn::LitStr = content.parse()?;
+                        if lit.value().trim().is_empty() {
+                            return Err(syn::Error::new(
+                                lit.span(),
+                                "variant alias must not be empty or whitespace-only",
+                            ));
+                        }
                         out.push(lit);
                         if content.peek(syn::Token![,]) {
                             content.parse::<syn::Token![,]>()?;
                         }
                     }
                     out
+                } else if fork.peek(syn::token::Bracket) {
+                    return Err(syn::Error::new(
+                        ident.span(),
+                        format!("expected 'aliases', found '{ident}'"),
+                    ));
                 } else {
                     Vec::new()
                 }
@@ -795,6 +807,44 @@ mod tests {
         );
     }
 
+    #[test]
+    fn declare_variants_rejects_empty_alias_literal() {
+        let err =
+            match syn::parse_str::<DeclareVariantsInput>("default Foo => \"1\" aliases [\"\"]") {
+                Ok(_) => panic!("empty alias literal should be rejected"),
+                Err(err) => err,
+            };
+        assert!(
+            err.to_string().contains("empty or whitespace-only"),
+            "expected empty-alias error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn declare_variants_rejects_whitespace_only_alias_literal() {
+        let err = match syn::parse_str::<DeclareVariantsInput>(
+            "default Foo => \"1\" aliases [\"  \\t\"]",
+        ) {
+            Ok(_) => panic!("whitespace-only alias literal should be rejected"),
+            Err(err) => err,
+        };
+        assert!(
+            err.to_string().contains("empty or whitespace-only"),
+            "expected whitespace-only alias error, got: {err}"
+        );
+    }
+
+    #[test]
+    fn declare_variants_rejects_unknown_alias_keyword_before_bracket() {
+        let err = match syn::parse_str::<DeclareVariantsInput>(
+            "default Foo => \"1\" nicknames [\"Foo\"]",
+        ) {
+            Ok(_) => panic!("unknown aliases keyword should be rejected"),
+            Err(err) => err,
+        };
+        assert_eq!(err.to_string(), "expected 'aliases', found 'nicknames'");
+    }
+
     #[test]
     fn declare_variants_generates_aggregate_value_and_witness_dispatch() {
         let input: DeclareVariantsInput = syn::parse_quote! {

src/registry/mod.rs

@@ -59,7 +59,9 @@ pub use schema::{
     collect_schemas, declared_size_fields, FieldInfoJson, ProblemSchemaEntry, ProblemSchemaJson,
     ProblemSizeFieldEntry, VariantDimension,
 };
-pub use variant::{find_variant_by_alias, find_variant_entry, VariantEntry};
+pub use variant::{
+    find_variant_by_alias, find_variant_entry, validate_variant_aliases, VariantEntry,
+};
 
 use std::any::Any;
 use std::collections::BTreeMap;

src/registry/variant.rs

@@ -81,6 +81,98 @@ pub fn find_variant_by_alias(
     Some((entry, entry.variant_map()))
 }
 
+/// Validate all variant-level aliases registered in inventory.
+///
+/// This is intended for explicit test-time or startup invocation. It rejects
+/// duplicate variant-level aliases, aliases that collide with canonical
+/// problem names or problem-level aliases, and empty aliases for manually
+/// constructed [`VariantEntry`] values that bypass `declare_variants!`.
+pub fn validate_variant_aliases() -> Result<(), Vec<String>> {
+    let mut conflicts = Vec::new();
+    let mut problem_names: BTreeMap<String, Vec<String>> = BTreeMap::new();
+
+    for problem in super::problem_type::problem_types() {
+        problem_names
+            .entry(problem.canonical_name.to_lowercase())
+            .or_default()
+            .push(format!(
+                "canonical problem name `{}`",
+                problem.canonical_name
+            ));
+
+        for alias in problem.aliases {
+            problem_names
+                .entry(alias.to_lowercase())
+                .or_default()
+                .push(format!(
+                    "problem-level alias `{alias}` for `{}`",
+                    problem.canonical_name
+                ));
+        }
+    }
+
+    let mut variant_aliases: BTreeMap<String, Vec<(String, String)>> = BTreeMap::new();
+    for entry in inventory::iter::<VariantEntry> {
+        let target = variant_label(entry);
+        for alias in entry.aliases {
+            if alias.trim().is_empty() {
+                conflicts.push(format!(
+                    "variant-level alias on {target} is empty or whitespace-only"
+                ));
+                continue;
+            }
+
+            let lower = alias.to_lowercase();
+            if let Some(collisions) = problem_names.get(&lower) {
+                for collision in collisions {
+                    conflicts.push(format!(
+                        "variant-level alias `{alias}` on {target} conflicts with {collision}"
+                    ));
+                }
+            }
+
+            variant_aliases
+                .entry(lower)
+                .or_default()
+                .push((alias.to_string(), target.clone()));
+        }
+    }
+
+    for (lower, registrations) in variant_aliases {
+        if registrations.len() > 1 {
+            let details = registrations
+                .iter()
+                .map(|(alias, target)| format!("`{alias}` on {target}"))
+                .collect::<Vec<_>>()
+                .join("; ");
+            conflicts.push(format!(
+                "duplicate variant-level alias `{lower}` (case-insensitive): {details}"
+            ));
+        }
+    }
+
+    if conflicts.is_empty() {
+        Ok(())
+    } else {
+        conflicts.sort();
+        Err(conflicts)
+    }
+}
+
+fn variant_label(entry: &VariantEntry) -> String {
+    let variant = entry.variant();
+    if variant.is_empty() {
+        return entry.name.to_string();
+    }
+
+    let parts = variant
+        .iter()
+        .map(|(key, value)| format!("{key}={value}"))
+        .collect::<Vec<_>>()
+        .join(", ");
+    format!("{} {{{parts}}}", entry.name)
+}
+
 impl std::fmt::Debug for VariantEntry {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("VariantEntry")
@@ -92,3 +184,7 @@ impl std::fmt::Debug for VariantEntry {
 }
 
 inventory::collect!(VariantEntry);
+
+#[cfg(test)]
+#[path = "../unit_tests/registry/variant.rs"]
+mod tests;

src/unit_tests/registry/variant.rs

@@ -0,0 +1,8 @@
+use crate::registry::variant::validate_variant_aliases;
+
+#[test]
+fn variant_alias_inventory_is_valid() {
+    if let Err(conflicts) = validate_variant_aliases() {
+        panic!("variant alias validation failed:\n{}", conflicts.join("\n"));
+    }
+}