diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e36e983..ed900c37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Changelog +* improve `inject_global_value` to support structured data. Add the `env_json` property to inject JSON encoded data and the `default_value` property to inject data when the provided environment variable is not defined ([#277](https://github.com/seaofvoices/darklua/pull/277)) * add rule to remove method call syntax (`remove_method_call`) ([#276](https://github.com/seaofvoices/darklua/pull/276)) * fix `remove_unused_variable` rule to correctly handle trailing unassigned (but used!) variables ([#275](https://github.com/seaofvoices/darklua/pull/275)) * add rule to convert Luau numbers (`convert_luau_number`) ([#274](https://github.com/seaofvoices/darklua/pull/274)) diff --git a/Cargo.lock b/Cargo.lock index 6d2674bd..a40e8dca 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -421,6 +421,7 @@ dependencies = [ "log", "notify", "notify-debouncer-full", + "num-traits", "paste", "pathdiff", "petgraph", diff --git a/Cargo.toml b/Cargo.toml index 96e2fc16..0e3f605e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,6 +35,7 @@ full_moon = { version = "1.0.0", features = ["roblox"] } indexmap = "2.7.0" json5 = "0.4.1" log = "0.4.22" +num-traits = "0.2.19" pathdiff = "0.2.3" petgraph = "0.6.5" regex = "1.11.1" diff --git a/site/content/rules/inject_global_value.md b/site/content/rules/inject_global_value.md index afc33701..c76a9339 100644 --- a/site/content/rules/inject_global_value.md +++ b/site/content/rules/inject_global_value.md @@ -7,23 +7,46 @@ parameters: type: string description: The name of the global variable - name: value - type: boolean, number or string + type: any description: The value to inject default: nil - name: env added_in: "0.7.0" type: string description: An environment variable to read the value from + - name: env_json + added_in: "unreleased" + type: string + description: An environment variable to read the json-encoded value from + - name: default_value + added_in: "unreleased" + type: any + description: The default value when using an environment variable that is not defined examples: - rules: "[{ rule: 'inject_global_value', identifier: 'CONSTANT', value: 'Hello' }, { rule: 'inject_global_value', identifier: 'AMOUNT', value: 11 }]" content: | if _G.AMOUNT > 10 or _G.CONSTANT ~= nil then --[[ ... ]] end + - rules: "[{ rule: 'inject_global_value', identifier: 'DEBUG', value: true }]" + content: | + if _G.DEBUG then + print('Debug information') + end --- This rule will find a global variable and replace it with a given value. The value can be defined in the rule configuration or taken from an environment variable. +To inject a static value, use the `value` property. + +```json5 +{ + rule: "inject_global_value", + identifier: "GLOBAL", + value: true, +} +``` + If `value` is not specified, the `env` property can be defined to read an environment variable that will be read into a string. ```json5 @@ -34,4 +57,25 @@ If `value` is not specified, the `env` property can be defined to read an enviro } ``` +Alternatively, the `env_json` property allows you to read a JSON-encoded value (`json5` is supported) from an environment variable. This is useful for injecting any data like booleans or structured data like arrays or objects. + +```json5 +{ + rule: "inject_global_value", + identifier: "SETTINGS", + env_json: "APP_SETTINGS", +} +``` + +When using the `env` or `env_json` property, the `default_value` property can be used to provide a fallback value when the environment variable is not defined. This prevents the rule from using `nil` as the default value. + +```json5 +{ + rule: "inject_global_value", + identifier: "FEATURE_FLAG", + env: "ENABLE_FEATURE", + default_value: false, +} +``` + This rule can be used in combination with the `remove_unused_if_branch`, `compute_expression`, and other rules, to eliminate dead branches. In addition to making your code smaller, it should make it faster (depending on how hot the code path is) since it is eliminating branch condition evaluations at client-side runtime. diff --git a/src/rules/inject_value.rs b/src/rules/inject_value.rs index 8dbf273a..e0f28f62 100644 --- a/src/rules/inject_value.rs +++ b/src/rules/inject_value.rs @@ -1,7 +1,7 @@ -use crate::nodes::{ - Block, DecimalNumber, Expression, ParentheseExpression, Prefix, StringExpression, UnaryOperator, -}; -use crate::process::{IdentifierTracker, NodeProcessor, NodeVisitor, ScopeVisitor}; +use num_traits::ToPrimitive; + +use crate::nodes::{Block, Expression, ParentheseExpression, Prefix, StringExpression}; +use crate::process::{to_expression, IdentifierTracker, NodeProcessor, NodeVisitor, ScopeVisitor}; use crate::rules::{ Context, FlawlessRule, RuleConfiguration, RuleConfigurationError, RuleProperties, RulePropertyValue, @@ -84,38 +84,58 @@ impl NodeProcessor for ValueInjection { pub const INJECT_GLOBAL_VALUE_RULE_NAME: &str = "inject_global_value"; /// A rule to replace global variables with values. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq)] pub struct InjectGlobalValue { identifier: String, value: Expression, + original_properties: RuleProperties, +} + +fn properties_with_value(value: impl Into) -> RuleProperties { + let mut properties = RuleProperties::new(); + properties.insert("value".to_owned(), value.into()); + properties } impl InjectGlobalValue { - pub fn nil>(identifier: S) -> Self { + pub fn nil(identifier: impl Into) -> Self { Self { identifier: identifier.into(), value: Expression::nil(), + original_properties: properties_with_value(RulePropertyValue::None), } } - pub fn boolean>(identifier: S, value: bool) -> Self { + pub fn boolean(identifier: impl Into, value: bool) -> Self { Self { identifier: identifier.into(), value: Expression::from(value), + original_properties: properties_with_value(value), } } - pub fn string, S2: Into>(identifier: S, value: S2) -> Self { + pub fn string(identifier: impl Into, value: impl Into) -> Self { + let value = value.into(); + let original_properties = properties_with_value(&value); Self { identifier: identifier.into(), value: StringExpression::from_value(value).into(), + original_properties, } } - pub fn number>(identifier: S, value: f64) -> Self { + pub fn number(identifier: impl Into, value: f64) -> Self { Self { identifier: identifier.into(), value: Expression::from(value), + original_properties: if let Some(integer) = value + .to_usize() + .filter(|integer| integer.to_f64() == Some(value)) + { + properties_with_value(integer) + } else { + properties_with_value(value) + }, } } } @@ -125,6 +145,7 @@ impl Default for InjectGlobalValue { Self { identifier: "".to_owned(), value: Expression::nil(), + original_properties: RuleProperties::new(), } } } @@ -139,34 +160,63 @@ impl FlawlessRule for InjectGlobalValue { impl RuleConfiguration for InjectGlobalValue { fn configure(&mut self, properties: RuleProperties) -> Result<(), RuleConfigurationError> { verify_required_properties(&properties, &["identifier"])?; - verify_property_collisions(&properties, &["value", "env"])?; + verify_property_collisions(&properties, &["value", "env", "env_json"])?; + verify_property_collisions(&properties, &["value", "default_value"])?; + + let mut default_value_expected = None; + let mut has_default_value = false; + + self.original_properties = properties.clone(); for (key, value) in properties { match key.as_str() { "identifier" => { self.identifier = value.expect_string(&key)?; } - "value" => match value { - RulePropertyValue::None => {} - RulePropertyValue::String(value) => { - self.value = StringExpression::from_value(value).into(); - } - RulePropertyValue::Boolean(value) => { - self.value = Expression::from(value); - } - RulePropertyValue::Usize(value) => { - self.value = DecimalNumber::new(value as f64).into(); + "value" => { + if let Some(value) = value.into_expression() { + self.value = value + } else { + return Err(RuleConfigurationError::UnexpectedValueType(key)); } - RulePropertyValue::Float(value) => { - self.value = Expression::from(value); + } + "default_value" => { + has_default_value = true; + if let Some(value) = value.into_expression() { + self.value = value + } else { + return Err(RuleConfigurationError::UnexpectedValueType(key)); } - _ => return Err(RuleConfigurationError::UnexpectedValueType(key)), - }, - "env" => { + } + "env" | "env_json" => { let variable_name = value.expect_string(&key)?; if let Some(os_value) = env::var_os(&variable_name) { if let Some(value) = os_value.to_str() { - self.value = StringExpression::from_value(value).into(); + self.value = if key.as_str() == "env_json" { + let json_value = json5::from_str::(value).map_err(|err| { + RuleConfigurationError::UnexpectedValue { + property: key.clone(), + message: format!( + "invalid json data assigned to the `{}` environment variable: {}", + &variable_name, + err + ), + } + })?; + + to_expression(&json_value).map_err(|err| { + RuleConfigurationError::UnexpectedValue { + property: key, + message: format!( + "unable to convert json data assigned to the `{}` environment variable to a lua expression: {}", + &variable_name, + err + ), + } + })? + } else { + StringExpression::from_value(value).into() + }; } else { return Err(RuleConfigurationError::UnexpectedValue { property: key, @@ -177,17 +227,23 @@ impl RuleConfiguration for InjectGlobalValue { }); } } else { - log::warn!( - "environment variable `{}` is not defined. The rule `{}` will use `nil`", - variable_name, - INJECT_GLOBAL_VALUE_RULE_NAME, - ); + default_value_expected = Some(variable_name); }; } _ => return Err(RuleConfigurationError::UnexpectedProperty(key)), } } + if !has_default_value { + if let Some(variable_name) = default_value_expected { + log::warn!( + "environment variable `{}` is not defined. The rule `{}` will use `nil`", + &variable_name, + INJECT_GLOBAL_VALUE_RULE_NAME, + ); + } + } + Ok(()) } @@ -196,43 +252,13 @@ impl RuleConfiguration for InjectGlobalValue { } fn serialize_to_properties(&self) -> RuleProperties { - let mut rules = RuleProperties::new(); + let mut rules = self.original_properties.clone(); + rules.insert( "identifier".to_owned(), RulePropertyValue::String(self.identifier.clone()), ); - let property_value = match &self.value { - Expression::True(_) => RulePropertyValue::Boolean(true), - Expression::False(_) => RulePropertyValue::Boolean(false), - Expression::Nil(_) => RulePropertyValue::None, - Expression::Number(number) => { - let value = number.compute_value(); - if value.trunc() == value && value >= 0.0 && value < usize::MAX as f64 { - RulePropertyValue::Usize(value as usize) - } else { - RulePropertyValue::Float(value) - } - } - Expression::String(string) => RulePropertyValue::from(string.get_value()), - Expression::Unary(unary) => { - if matches!(unary.operator(), UnaryOperator::Minus) { - if let Expression::Number(number) = unary.get_expression() { - RulePropertyValue::Float(-number.compute_value()) - } else { - unreachable!( - "unexpected expression for unary minus {:?}", - unary.get_expression() - ); - } - } else { - unreachable!("unexpected unary operator {:?}", unary.operator()); - } - } - _ => unreachable!("unexpected expression {:?}", self.value), - }; - rules.insert("value".to_owned(), property_value); - rules } } @@ -252,7 +278,7 @@ mod test { }"#, ); - assert!(result.is_err()); + insta::assert_snapshot!(result.unwrap_err().to_string(), @"missing required field 'identifier'"); } #[test] @@ -260,19 +286,34 @@ mod test { let result = json5::from_str::>( r#"{ rule: 'inject_global_value', + identifier: 'DEV', value: false, env: "VAR", }"#, ); - assert!(result.is_err()); + insta::assert_snapshot!(result.unwrap_err().to_string(), @"the fields `value` and `env` cannot be defined together"); + } + + #[test] + fn configure_with_value_and_default_value_properties_should_error() { + let result = json5::from_str::>( + r#"{ + rule: 'inject_global_value', + identifier: 'DEV', + value: false, + default_value: true, + }"#, + ); + + insta::assert_snapshot!(result.unwrap_err().to_string(), @"the fields `value` and `default_value` cannot be defined together"); } #[test] fn deserialize_from_string_notation_should_error() { let result = json5::from_str::>("'inject_global_value'"); - assert!(result.is_err()); + insta::assert_snapshot!(result.unwrap_err().to_string(), @"missing required field 'identifier'"); } #[test] @@ -323,4 +364,68 @@ mod test { assert_json_snapshot!("inject_float_value_as_var", rule); } + + #[test] + fn serialization_round_trip_with_mixed_array() { + let rule: Box = json5::from_str( + r#"{ + rule: 'inject_global_value', + identifier: 'foo', + value: ["hello", true, 1, 0.5, -1.35], + }"#, + ) + .unwrap(); + + assert_json_snapshot!(rule, @r###" + { + "rule": "inject_global_value", + "identifier": "foo", + "value": [ + "hello", + true, + 1, + 0.5, + -1.35 + ] + } + "###); + } + + #[test] + fn serialization_round_trip_with_object_value() { + let rule: Box = json5::from_str( + r#"{ + rule: 'inject_global_value', + identifier: 'foo', + value: { + f0: 'world', + f1: true, + f2: 1, + f3: 0.5, + f4: -1.35, + f5: [1, 2, 3], + }, + }"#, + ) + .unwrap(); + + assert_json_snapshot!(rule, @r###" + { + "rule": "inject_global_value", + "identifier": "foo", + "value": { + "f0": "world", + "f1": true, + "f2": 1, + "f3": 0.5, + "f4": -1.35, + "f5": [ + 1, + 2, + 3 + ] + } + } + "###); + } } diff --git a/src/rules/mod.rs b/src/rules/mod.rs index f7869339..8a8cb73e 100644 --- a/src/rules/mod.rs +++ b/src/rules/mod.rs @@ -493,15 +493,16 @@ fn verify_property_collisions( properties: &RuleProperties, names: &[&str], ) -> Result<(), RuleConfigurationError> { - let mut exists = false; + let mut exists: Option<&str> = None; for name in names.iter() { if properties.contains_key(*name) { - if exists { - return Err(RuleConfigurationError::PropertyCollision( - names.iter().map(ToString::to_string).collect(), - )); + if let Some(existing_name) = &exists { + return Err(RuleConfigurationError::PropertyCollision(vec![ + existing_name.to_string(), + name.to_string(), + ])); } else { - exists = true; + exists = Some(*name); } } } diff --git a/src/rules/rule_property.rs b/src/rules/rule_property.rs index 6bb202a5..27b1939f 100644 --- a/src/rules/rule_property.rs +++ b/src/rules/rule_property.rs @@ -3,13 +3,18 @@ use std::collections::HashMap; use regex::Regex; use serde::{Deserialize, Serialize}; +use crate::{ + nodes::{DecimalNumber, Expression, StringExpression, TableEntry, TableExpression}, + process::to_expression, +}; + use super::{require::PathRequireMode, RequireMode, RobloxRequireMode, RuleConfigurationError}; pub type RuleProperties = HashMap; /// In order to be able to weakly-type the properties of any rule, this enum makes it possible to /// easily use serde to gather the value associated with a property. -#[derive(Debug, Serialize, Deserialize, PartialEq)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(untagged, rename_all = "snake_case")] pub enum RulePropertyValue { Boolean(bool), @@ -19,6 +24,10 @@ pub enum RulePropertyValue { StringList(Vec), RequireMode(RequireMode), None, + #[doc(hidden)] + Map(serde_json::Map), + #[doc(hidden)] + Array(Vec), } impl RulePropertyValue { @@ -82,6 +91,30 @@ impl RulePropertyValue { _ => Err(RuleConfigurationError::RequireModeExpected(key.to_owned())), } } + + pub(crate) fn into_expression(self) -> Option { + match self { + Self::None => Some(Expression::nil()), + Self::String(value) => Some(StringExpression::from_value(value).into()), + Self::Boolean(value) => Some(Expression::from(value)), + Self::Usize(value) => Some(DecimalNumber::new(value as f64).into()), + Self::Float(value) => Some(Expression::from(value)), + Self::StringList(value) => Some( + TableExpression::new( + value + .into_iter() + .map(|element| { + TableEntry::from_value(StringExpression::from_value(element)) + }) + .collect(), + ) + .into(), + ), + Self::RequireMode(require_mode) => to_expression(&require_mode).ok(), + Self::Map(value) => to_expression(&value).ok(), + Self::Array(value) => to_expression(&value).ok(), + } + } } impl From for RulePropertyValue { @@ -216,4 +249,166 @@ mod test { let bool: Option = None; assert_eq!(RulePropertyValue::from(bool), RulePropertyValue::None); } + + mod parse { + use super::*; + + fn parse_rule_property(json: &str, expect_property: RulePropertyValue) { + let parsed: RulePropertyValue = serde_json::from_str(json).unwrap(); + assert_eq!(parsed, expect_property); + } + + #[test] + fn parse_boolean_true() { + parse_rule_property("true", RulePropertyValue::Boolean(true)); + } + + #[test] + fn parse_boolean_false() { + parse_rule_property("false", RulePropertyValue::Boolean(false)); + } + + #[test] + fn parse_string() { + parse_rule_property( + r#""hello world""#, + RulePropertyValue::String("hello world".to_owned()), + ); + } + + #[test] + fn parse_empty_string() { + parse_rule_property(r#""""#, RulePropertyValue::String("".to_owned())); + } + + #[test] + fn parse_string_with_escapes() { + parse_rule_property( + r#""hello\nworld\twith\"quotes\"""#, + RulePropertyValue::String("hello\nworld\twith\"quotes\"".to_owned()), + ); + } + + #[test] + fn parse_usize_zero() { + parse_rule_property("0", RulePropertyValue::Usize(0)); + } + + #[test] + fn parse_usize_positive() { + parse_rule_property("42", RulePropertyValue::Usize(42)); + } + + #[test] + fn parse_float_zero() { + parse_rule_property("0.0", RulePropertyValue::Float(0.0)); + } + + #[test] + fn parse_float_positive() { + parse_rule_property("3.14159", RulePropertyValue::Float(3.14159)); + } + + #[test] + fn parse_float_negative() { + parse_rule_property("-2.718", RulePropertyValue::Float(-2.718)); + } + + #[test] + fn parse_float_scientific_notation() { + parse_rule_property("1.23e-4", RulePropertyValue::Float(1.23e-4)); + } + + #[test] + fn parse_string_list_empty() { + parse_rule_property("[]", RulePropertyValue::StringList(vec![])); + } + + #[test] + fn parse_string_list_single() { + parse_rule_property( + r#"["hello"]"#, + RulePropertyValue::StringList(vec!["hello".to_owned()]), + ); + } + + #[test] + fn parse_string_list_multiple() { + parse_rule_property( + r#"["hello", "world", "test"]"#, + RulePropertyValue::StringList(vec![ + "hello".to_owned(), + "world".to_owned(), + "test".to_owned(), + ]), + ); + } + + #[test] + fn parse_string_list_with_empty_strings() { + parse_rule_property( + r#"["", "hello", ""]"#, + RulePropertyValue::StringList(vec![ + "".to_owned(), + "hello".to_owned(), + "".to_owned(), + ]), + ); + } + + #[test] + fn parse_require_mode_path_string() { + parse_rule_property(r#""path""#, RulePropertyValue::String("path".to_owned())); + } + + #[test] + fn parse_require_mode_roblox_string() { + parse_rule_property( + r#""roblox""#, + RulePropertyValue::String("roblox".to_owned()), + ); + } + + #[test] + fn parse_require_mode_path_object() { + parse_rule_property( + r#"{"name": "path"}"#, + RulePropertyValue::RequireMode(RequireMode::Path(PathRequireMode::default())), + ); + } + + #[test] + fn parse_require_mode_path_object_with_options() { + parse_rule_property( + r#"{"name": "path", "module_folder_name": "index"}"#, + RulePropertyValue::RequireMode(RequireMode::Path(PathRequireMode::new("index"))), + ); + } + + #[test] + fn parse_require_mode_roblox_object() { + parse_rule_property( + r#"{"name": "roblox"}"#, + RulePropertyValue::RequireMode(RequireMode::Roblox(RobloxRequireMode::default())), + ); + } + + #[test] + fn parse_require_mode_roblox_object_with_options() { + parse_rule_property( + r#"{"name": "roblox", "rojo_sourcemap": "./sourcemap.json"}"#, + RulePropertyValue::RequireMode(RequireMode::Roblox( + serde_json::from_str::( + r#"{"rojo_sourcemap": "./sourcemap.json"}"#, + ) + .unwrap(), + )), + ); + } + + #[test] + fn parse_null_as_none() { + parse_rule_property("null", RulePropertyValue::None); + } + } } diff --git a/tests/rule_tests/inject_value.rs b/tests/rule_tests/inject_value.rs index ae51e68e..8a6f131e 100644 --- a/tests/rule_tests/inject_value.rs +++ b/tests/rule_tests/inject_value.rs @@ -52,6 +52,48 @@ test_rule_without_effects!( does_not_inline_if_global_table_is_redefined("local _G return _G.foo"), ); +test_rule!( + inject_global_empty_array, + json5::from_str::>( + r#"{ + rule: 'inject_global_value', + identifier: 'VALUE', + value: [], + }"#, + ) + .unwrap(), + inject_value("return VALUE") => "return {}", + inject_value_from_global_table("return _G.VALUE") => "return {}", +); + +test_rule!( + inject_global_mixed_array, + json5::from_str::>( + r#"{ + rule: 'inject_global_value', + identifier: 'VALUE', + value: [1, "hello", true], + }"#, + ) + .unwrap(), + inject_value("return VALUE") => "return {1, 'hello', true}", + inject_value_from_global_table("return _G.VALUE") => "return {1, 'hello', true}", +); + +test_rule!( + inject_global_object, + json5::from_str::>( + r#"{ + rule: 'inject_global_value', + identifier: 'VALUE', + value: {a: 1, b: "hello", c: true}, + }"#, + ) + .unwrap(), + inject_value("return VALUE") => "return { a = 1, b = 'hello', c = true }", + inject_value_from_global_table("return _G.VALUE") => "return { a = 1, b = 'hello', c = true }", +); + #[test] fn deserialize_from_object_notation_without_value() { json5::from_str::>( @@ -183,6 +225,19 @@ test_rule!( inject_negative_integer("return _G.num") => "return 1E49", ); +test_rule!( + inject_global_variable_default_value, + json5::from_str::>( + r#"{ + rule: 'inject_global_value', + identifier: 'num', + env: 'NUM', + default_value: 10, + }"#, + ).unwrap(), + inject_negative_integer("return _G.num") => "return 10", +); + #[test] fn deserialize_number_value_too_large() { let err = json5::from_str::>(