Description
Replace eval(parse()) patterns with safer and more performant alternatives. The eval(parse()) pattern is considered dangerous, slow, and hard to debug. Modern R provides better ways to handle dynamic evaluation.
Current Implementation Issues
R/MappedData.R - Multiple eval(parse()) occurrences
This is the most critical performance issue in the codebase. The pattern appears 10+ times:
Lines 336-342: Dynamic aesthetic assignment
eval(parse(text = paste0(
"ggplot2::aes(",
aestheticName, " = ", columnName,
")"
)))
- Very slow: parsing and evaluation on every call
- Security risk: potential code injection
- Hard to debug: errors show parsed text, not original code
Other occurrences in MappedData.R:
- Lines 386-392: Similar pattern
- Lines 405-411: Same inefficient pattern
- Lines 542-548: Repeated pattern
- Lines 601-607: Repeated pattern
R/MappedDataTimeProfile.R - Similar patterns
Similar eval(parse()) patterns for dynamic aesthetic creation.
Why eval(parse()) is Problematic
- Performance: Text parsing is expensive
- Security: Can execute arbitrary code
- Debugging: Error messages are cryptic
- Maintainability: Code is harder to understand
- Type safety: No compile-time checking
Suggested Alternatives
Option 1: Use rlang (Recommended)
The package already uses rlang for tidy evaluation. Use rlang::sym() and rlang::expr():
Before:
eval(parse(text = paste0("ggplot2::aes(", aestheticName, " = ", columnName, ")")))
After:
aes_list <- list()
aes_list[[aestheticName]] <- rlang::sym(columnName)
do.call(ggplot2::aes, aes_list)
Or using tidy evaluation:
ggplot2::aes(!!rlang::sym(aestheticName) := !!rlang::sym(columnName))
Option 2: Use aes_string() (Deprecated but safer)
Note: aes_string() is deprecated in newer ggplot2, but still safer than eval(parse()):
ggplot2::aes_string(setNames(columnName, aestheticName))
Option 3: Build aesthetics as list
Build aesthetics programmatically:
aes_mapping <- list()
aes_mapping[[aestheticName]] <- as.name(columnName)
do.call(ggplot2::aes, aes_mapping)
Recommended Approach
For this codebase, use Option 1 with rlang since:
- Package already imports
rlang
- Most modern and maintainable
- Best performance
- Type-safe
- Works with current ggplot2 versions
Expected Benefits
- Significant performance improvement (5-10x faster for aesthetic creation)
- Safer code (no code injection risk)
- Better error messages
- More maintainable code
- Type checking and IDE support
Implementation Notes
Files to Modify
R/MappedData.R (10+ occurrences - highest priority)
R/MappedDataTimeProfile.R
Testing
- Run all existing plot tests
- Test each plot type with various aesthetic mappings
- Verify error messages are helpful
- Test edge cases (special characters in column names, etc.)
- Add performance benchmarks to measure improvement
References
Description
Replace
eval(parse())patterns with safer and more performant alternatives. Theeval(parse())pattern is considered dangerous, slow, and hard to debug. Modern R provides better ways to handle dynamic evaluation.Current Implementation Issues
R/MappedData.R- Multiple eval(parse()) occurrencesThis is the most critical performance issue in the codebase. The pattern appears 10+ times:
Lines 336-342: Dynamic aesthetic assignment
Other occurrences in MappedData.R:
R/MappedDataTimeProfile.R- Similar patternsSimilar
eval(parse())patterns for dynamic aesthetic creation.Why eval(parse()) is Problematic
Suggested Alternatives
Option 1: Use rlang (Recommended)
The package already uses
rlangfor tidy evaluation. Userlang::sym()andrlang::expr():Before:
After:
Or using tidy evaluation:
Option 2: Use aes_string() (Deprecated but safer)
Note:
aes_string()is deprecated in newer ggplot2, but still safer thaneval(parse()):Option 3: Build aesthetics as list
Build aesthetics programmatically:
Recommended Approach
For this codebase, use Option 1 with rlang since:
rlangExpected Benefits
Implementation Notes
Files to Modify
R/MappedData.R(10+ occurrences - highest priority)R/MappedDataTimeProfile.RTesting
References