diff --git a/src/execute/mod.rs b/src/execute/mod.rs index bbbfd958..7f8c68e8 100644 --- a/src/execute/mod.rs +++ b/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 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")); + } } diff --git a/src/validate.rs b/src/validate.rs index 79c6c36e..1a134ef1 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -176,9 +176,28 @@ pub fn validate(query: &str) -> Result { // 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() + ); + } }