Fix global mappings and aesthetic aliases in validation (#306)

* Merge global mappings during validation

* Centralise aesthetic aliases

* cargo fmt

Author: georgestagg

src/execute/mod.rs

@@ -178,17 +178,14 @@ fn merge_global_mappings_into_layers(specs: &mut [Plot], layer_schemas: &[Schema
             let schema_columns: HashSet<&str> = schema.iter().map(|c| c.name.as_str()).collect();
 
             // 1. First merge explicit global aesthetics (layer overrides global)
-            // Note: "color"/"colour" are accepted even though not in supported,
-            // because split_color_aesthetic will convert them to fill/stroke later
             // Note: facet aesthetics (panel, row, column) are also accepted,
             // as they apply to all layers regardless of geom support
             // Note: Use all_names (not supported) so that Delayed aesthetics like
             // pos2 on histogram can be targeted by explicit global mappings, matching
             // the behavior of layer-level MAPPING
             for (aesthetic, value) in &spec.global_mappings.aesthetics {
-                let is_color_alias = matches!(aesthetic.as_str(), "color" | "colour");
                 let is_facet_aesthetic = crate::plot::scale::is_facet_aesthetic(aesthetic.as_str());
-                if all_names.contains(&aesthetic.as_str()) || is_color_alias || is_facet_aesthetic {
+                if all_names.contains(&aesthetic.as_str()) || is_facet_aesthetic {
                     layer
                         .mappings
                         .aesthetics
@@ -226,69 +223,59 @@ fn merge_global_mappings_into_layers(specs: &mut [Plot], layer_schemas: &[Schema
     }
 }
 
-/// Let 'color' aesthetics fill defaults for the 'stroke' and 'fill' aesthetics.
-/// Also splits 'color' scale to 'fill' and 'stroke' scales.
-/// Removes 'color' from both mappings and scales after splitting to avoid
-/// non-deterministic behavior from HashMap iteration order.
-fn split_color_aesthetic(spec: &mut Plot) {
-    // 1. Split color SCALE to fill/stroke scales
-    if let Some(color_scale_idx) = spec.scales.iter().position(|s| s.aesthetic == "color") {
-        let color_scale = spec.scales[color_scale_idx].clone();
-
-        // Add fill scale if not already present
-        if !spec.scales.iter().any(|s| s.aesthetic == "fill") {
-            let mut fill_scale = color_scale.clone();
-            fill_scale.aesthetic = "fill".to_string();
-            spec.scales.push(fill_scale);
-        }
-
-        // Add stroke scale if not already present
-        if !spec.scales.iter().any(|s| s.aesthetic == "stroke") {
-            let mut stroke_scale = color_scale.clone();
-            stroke_scale.aesthetic = "stroke".to_string();
-            spec.scales.push(stroke_scale);
-        }
-
-        // Remove the color scale
-        spec.scales.remove(color_scale_idx);
-    }
-
-    // 2. Split color mapping to fill/stroke in layers, then remove color
-    for layer in &mut spec.layers {
-        if let Some(color_value) = layer.mappings.aesthetics.get("color").cloned() {
-            let aesthetics = layer.geom.aesthetics();
-
-            for &aes in &["stroke", "fill"] {
-                if aesthetics.is_supported(aes) {
-                    layer
-                        .mappings
-                        .aesthetics
-                        .entry(aes.to_string())
-                        .or_insert(color_value.clone());
+/// Resolve aesthetic aliases in a plot specification.
+///
+/// For each alias defined in [`AESTHETIC_ALIASES`], splits the alias in scales,
+/// layer mappings, and layer parameters into the concrete target aesthetics
+/// (only where the geom supports them). Removes the alias after splitting to
+/// avoid non-deterministic behavior from HashMap iteration order.
+fn resolve_aesthetic_aliases(spec: &mut Plot) {
+    use crate::plot::layer::geom::types::AESTHETIC_ALIASES;
+
+    for &(alias, targets) in AESTHETIC_ALIASES {
+        // 1. Split alias SCALE to target scales
+        if let Some(idx) = spec.scales.iter().position(|s| s.aesthetic == alias) {
+            let alias_scale = spec.scales[idx].clone();
+            for &target in targets {
+                if !spec.scales.iter().any(|s| s.aesthetic == target) {
+                    let mut target_scale = alias_scale.clone();
+                    target_scale.aesthetic = target.to_string();
+                    spec.scales.push(target_scale);
                 }
             }
-
-            // Remove color after splitting
-            layer.mappings.aesthetics.remove("color");
+            spec.scales.remove(idx);
         }
-    }
 
-    // 3. Split color parameter (SETTING) to fill/stroke in layers
-    for layer in &mut spec.layers {
-        if let Some(color_value) = layer.parameters.get("color").cloned() {
+        // 2. Split alias mapping and parameters in each layer
+        for layer in &mut spec.layers {
             let aesthetics = layer.geom.aesthetics();
 
-            for &aes in &["stroke", "fill"] {
-                if aesthetics.is_supported(aes) {
-                    layer
-                        .parameters
-                        .entry(aes.to_string())
-                        .or_insert(color_value.clone());
+            // Split mapping
+            if let Some(value) = layer.mappings.aesthetics.get(alias).cloned() {
+                for &target in targets {
+                    if aesthetics.is_supported(target) {
+                        layer
+                            .mappings
+                            .aesthetics
+                            .entry(target.to_string())
+                            .or_insert(value.clone());
+                    }
                 }
+                layer.mappings.aesthetics.remove(alias);
             }
 
-            // Remove color after splitting
-            layer.parameters.remove("color");
+            // Split parameter (SETTING)
+            if let Some(value) = layer.parameters.get(alias).cloned() {
+                for &target in targets {
+                    if aesthetics.is_supported(target) {
+                        layer
+                            .parameters
+                            .entry(target.to_string())
+                            .or_insert(value.clone());
+                    }
+                }
+                layer.parameters.remove(alias);
+            }
         }
     }
 }
@@ -1024,10 +1011,10 @@ pub fn prepare_data_with_reader(query: &str, reader: &dyn Reader) -> Result<Prep
     // because transformation happens in builder.rs right after parsing
     merge_global_mappings_into_layers(&mut specs, &layer_schemas);
 
-    // Split 'color' aesthetic to 'fill' and 'stroke' early in the pipeline
-    // This must happen before validation so fill/stroke are validated (not color)
+    // Resolve aesthetic aliases (e.g., 'color' → 'fill'/'stroke') early in the pipeline
+    // This must happen before validation so concrete aesthetics are validated
     for spec in &mut specs {
-        split_color_aesthetic(spec);
+        resolve_aesthetic_aliases(spec);
     }
 
     // Add literal (constant) columns to type info programmatically

src/plot/layer/geom/types.rs

@@ -18,6 +18,16 @@ pub const POSITION_VALUES: &[&str] = &["identity", "stack", "dodge", "jitter"];
 /// Closed interval side values for binned data
 pub const CLOSED_VALUES: &[&str] = &["left", "right"];
 
+/// Aesthetic aliases: user-facing names that resolve to concrete aesthetics.
+///
+/// An alias is considered supported if any of its target aesthetics are supported
+/// by a geom. For example, `color` resolves to `stroke` and/or `fill` — so any geom
+/// that supports either `stroke` or `fill` also accepts `color`.
+///
+/// Note: Spelling variants (`colour`/`col` → `color`) are handled separately at parse
+/// time by `normalise_aes_name()` in `parser/builder.rs`.
+pub const AESTHETIC_ALIASES: &[(&str, &[&str])] = &[("color", &["stroke", "fill"])];
+
 /// Default aesthetic values for a geom type
 ///
 /// This struct describes which aesthetics a geom supports, requires, and their default values.
@@ -32,21 +42,37 @@ pub struct DefaultAesthetics {
 }
 
 impl DefaultAesthetics {
-    /// Get all aesthetic names (including Delayed)
+    /// Get all aesthetic names (including Delayed and aliases)
     pub fn names(&self) -> Vec<&'static str> {
-        self.defaults.iter().map(|(name, _)| *name).collect()
+        let mut result: Vec<&'static str> = self.defaults.iter().map(|(name, _)| *name).collect();
+        // Include alias names if any of their targets are in the defaults
+        for &(alias, targets) in AESTHETIC_ALIASES {
+            if targets.iter().any(|t| result.contains(t)) {
+                result.push(alias);
+            }
+        }
+        result
     }
 
     /// Get supported aesthetic names (excludes Delayed, for MAPPING validation)
     ///
-    /// Returns the literal names from defaults. For bidirectional position checking,
-    /// use `is_supported()` which handles pos1/pos2 equivalence.
+    /// Returns the literal names from defaults plus any aliases whose targets are
+    /// supported. For bidirectional position checking, use `is_supported()` which
+    /// handles pos1/pos2 equivalence.
     pub fn supported(&self) -> Vec<&'static str> {
-        self.defaults
+        let mut result: Vec<&'static str> = self
+            .defaults
             .iter()
             .filter(|(_, value)| !matches!(value, DefaultAestheticValue::Delayed))
             .map(|(name, _)| *name)
-            .collect()
+            .collect();
+        // Include alias names if any of their targets are supported
+        for &(alias, targets) in AESTHETIC_ALIASES {
+            if targets.iter().any(|t| result.contains(t)) {
+                result.push(alias);
+            }
+        }
+        result
     }
 
     /// Get required aesthetic names (those marked as Required)
@@ -66,7 +92,8 @@ impl DefaultAesthetics {
     /// Check if an aesthetic is supported (not Delayed)
     ///
     /// Position aesthetics are bidirectional: if pos1* is supported, pos2* is also
-    /// considered supported (and vice versa).
+    /// considered supported (and vice versa). Aliases (e.g., `color`) are supported
+    /// if any of their target aesthetics are supported.
     pub fn is_supported(&self, name: &str) -> bool {
         // Check for direct match first
         let direct_match = self
@@ -86,6 +113,13 @@ impl DefaultAesthetics {
             });
         }
 
+        // Check if name is an alias that resolves to a supported aesthetic
+        for &(alias, targets) in AESTHETIC_ALIASES {
+            if alias == name {
+                return targets.iter().any(|t| self.is_supported(t));
+            }
+        }
+
         false
     }
 
@@ -184,18 +218,20 @@ mod tests {
         assert_eq!(aes.get("yend"), Some(&DefaultAestheticValue::Delayed));
         assert_eq!(aes.get("nonexistent"), None);
 
-        // Test names() - includes all aesthetics
+        // Test names() - includes all aesthetics + aliases
         let names = aes.names();
-        assert_eq!(names.len(), 6);
+        assert_eq!(names.len(), 7); // 6 defaults + "color" alias (has stroke+fill)
         assert!(names.contains(&"x"));
         assert!(names.contains(&"yend"));
+        assert!(names.contains(&"color")); // alias resolved from stroke+fill
 
-        // Test supported() - excludes Delayed
+        // Test supported() - excludes Delayed, includes aliases
         let supported = aes.supported();
-        assert_eq!(supported.len(), 5);
+        assert_eq!(supported.len(), 6); // 5 non-delayed + "color" alias
         assert!(supported.contains(&"x"));
         assert!(supported.contains(&"size"));
         assert!(supported.contains(&"fill"));
+        assert!(supported.contains(&"color")); // alias
         assert!(!supported.contains(&"yend")); // Delayed excluded
 
         // Test required() - only Required variants
@@ -208,6 +244,7 @@ mod tests {
         // Test is_supported() - efficient membership check
         assert!(aes.is_supported("x"));
         assert!(aes.is_supported("size"));
+        assert!(aes.is_supported("color")); // alias: has stroke+fill
         assert!(!aes.is_supported("yend")); // Delayed not supported
         assert!(!aes.is_supported("nonexistent"));
 
@@ -222,4 +259,42 @@ mod tests {
         assert!(!aes.is_required("size"));
         assert!(!aes.is_required("yend"));
     }
+
+    #[test]
+    fn test_color_alias_requires_stroke_or_fill() {
+        // Geom with neither stroke nor fill: color alias should NOT be supported
+        let aes = DefaultAesthetics {
+            defaults: &[
+                ("pos1", DefaultAestheticValue::Required),
+                ("pos2", DefaultAestheticValue::Required),
+                ("size", DefaultAestheticValue::Number(3.0)),
+            ],
+        };
+
+        assert!(!aes.is_supported("color"));
+        assert!(!aes.supported().contains(&"color"));
+        assert!(!aes.names().contains(&"color"));
+
+        // Geom with only stroke: color alias should be supported
+        let aes_stroke = DefaultAesthetics {
+            defaults: &[
+                ("pos1", DefaultAestheticValue::Required),
+                ("stroke", DefaultAestheticValue::String("black")),
+            ],
+        };
+
+        assert!(aes_stroke.is_supported("color"));
+        assert!(aes_stroke.supported().contains(&"color"));
+
+        // Geom with only fill: color alias should be supported
+        let aes_fill = DefaultAesthetics {
+            defaults: &[
+                ("pos1", DefaultAestheticValue::Required),
+                ("fill", DefaultAestheticValue::String("black")),
+            ],
+        };
+
+        assert!(aes_fill.is_supported("color"));
+        assert!(aes_fill.supported().contains(&"color"));
+    }
 }

src/validate.rs

@@ -176,9 +176,28 @@ pub fn validate(query: &str) -> Result<Validated> {
 
             // Check required aesthetics
             // Note: Without schema data, we can only check if mappings exist,
-            // not if the columns are valid. We skip this check for wildcards.
-            if !layer.mappings.wildcard {
-                if let Err(e) = layer.validate_mapping(&plot.aesthetic_context, false) {
+            // not if the columns are valid. We skip this check for wildcards
+            // (either layer or global).
+            let is_annotation = matches!(
+                layer.source,
+                Some(crate::plot::types::DataSource::Annotation)
+            );
+            let has_wildcard =
+                layer.mappings.wildcard || (!is_annotation && plot.global_mappings.wildcard);
+            if !has_wildcard {
+                // Merge global mappings into a temporary copy for validation
+                // (mirrors execution-time merge, layer takes precedence)
+                let mut merged = layer.clone();
+                if !is_annotation {
+                    for (aesthetic, value) in &plot.global_mappings.aesthetics {
+                        merged
+                            .mappings
+                            .aesthetics
+                            .entry(aesthetic.clone())
+                            .or_insert(value.clone());
+                    }
+                }
+                if let Err(e) = merged.validate_mapping(&plot.aesthetic_context, false) {
                     errors.push(ValidationError {
                         message: format!("{}: {}", context, e),
                         location: None,
@@ -282,4 +301,59 @@ mod tests {
         assert!(validated.valid());
         assert!(validated.errors().is_empty());
     }
+
+    #[test]
+    fn test_validate_color_aesthetic_on_line() {
+        // color should be valid on line geom (has stroke)
+        let validated = validate(
+            "SELECT 1 as x, 2 as y VISUALISE DRAW line MAPPING x AS x, y AS y, region AS color",
+        )
+        .unwrap();
+        assert!(
+            validated.valid(),
+            "color should be accepted on line geom: {:?}",
+            validated.errors()
+        );
+    }
+
+    #[test]
+    fn test_validate_color_aesthetic_on_point() {
+        // color should be valid on point geom (has stroke + fill)
+        let validated = validate(
+            "SELECT 1 as x, 2 as y VISUALISE DRAW point MAPPING x AS x, y AS y, cat AS color",
+        )
+        .unwrap();
+        assert!(
+            validated.valid(),
+            "color should be accepted on point geom: {:?}",
+            validated.errors()
+        );
+    }
+
+    #[test]
+    fn test_validate_colour_spelling() {
+        // British spelling 'colour' should work (normalized by parser to 'color')
+        let validated = validate(
+            "SELECT 1 as x, 2 as y VISUALISE DRAW line MAPPING x AS x, y AS y, region AS colour",
+        )
+        .unwrap();
+        assert!(
+            validated.valid(),
+            "colour (British) should be accepted: {:?}",
+            validated.errors()
+        );
+    }
+
+    #[test]
+    fn test_validate_global_color_mapping() {
+        // Global color mapping should validate correctly
+        let validated =
+            validate("SELECT 1 as x, 2 as y VISUALISE x AS x, y AS y, region AS color DRAW line")
+                .unwrap();
+        assert!(
+            validated.valid(),
+            "global color mapping should be accepted: {:?}",
+            validated.errors()
+        );
+    }
 }