[JULES] Refactor: Unified contains function for List and Text

[logbie]

The Issue:
Duplicate logic for contains existed in both src/stdlib/list.rs and src/stdlib/text.rs. Furthermore, because both modules registered a function named contains in the same global environment, the registration in list.rs (which happened later) overwrote the one in text.rs. This caused contains to fail when used on Text, as the active implementation expected a List.

The Rational:

• Fix Bug: contains was broken for Text types.
• Reduce Redundancy: Consolidated near-duplicate logic.
• Improve Maintainability: A single source of truth for contains.

The Solution:
Refactored native_contains in src/stdlib/list.rs to act as a polymorphic function. It now checks the type of the first argument:

• If Value::List, it iterates through the list.
• If Value::Text, it performs a substring check (logic moved from text.rs).
• Removed the conflicting native_contains from src/stdlib/text.rs.

Verification:

• cargo fmt and cargo clippy passed.
• Manual verification script reproduce_contains.wfl confirmed contains works for both List and Text.
• All existing tests passed (after ensuring release build presence for integration tests).

---

PR created automatically by Jules for task 7954133854785299130 started by @logbie

Summary by CodeRabbit

• Refactor
  • Consolidated the contains operation to support both list and text containment checks with enhanced type validation and improved error handling.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

Claude finished @logbie's task —— View job

---

Code Review for Contains Function Refactor

Todo List:

• Read repository guidelines (CLAUDE.md)
• Read modified files to understand changes
• Analyze code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide comprehensive feedback

Code Review Analysis

🔍 Summary

This PR successfully addresses a critical bug where contains function registration collision caused text contains to fail. The refactor consolidates duplicate logic into a single polymorphic function, which is a solid architectural improvement.

✅ Positive Aspects

1. Bug Fix: Correctly resolves the function name collision issue where list.rs registration overwrote text.rs
2. Code Consolidation: Eliminates duplication by unifying similar logic in one location
3. Type Safety: Proper error handling with descriptive error messages for type mismatches
4. Performance: Maintains efficient implementations for both List iteration and Text substring checking

⚠️ Issues Identified

Critical Issues:

1. Missing Test Coverage

• No unit tests added to verify the polymorphic behavior
• Per CLAUDE.md guidelines: "TDD is mandatory: Write failing tests FIRST for any feature or bug fix"
• Need tests in tests/ directory to verify both List and Text contains work correctly

2. Documentation Gap

• The refactored function lacks documentation explaining its polymorphic nature
• Should document the type-specific behavior in function comments

Code Quality Issues:

1. Error Message Inconsistency src/stdlib/list.rs:124-131

• Error message format differs from other similar functions
• Consider standardizing error message patterns across the stdlib

2. String Dereferencing src/stdlib/list.rs:122

Ok(Value::Bool(text.contains(&**substring)))

• The &**substring pattern is correct but could be more readable as substring.as_ref()

🚀 Performance Considerations

Positive:

• List iteration remains O(n) as before
• Text substring check uses efficient str::contains() method

No concerns identified - performance characteristics are maintained or improved.

🔒 Security Analysis

No security concerns identified:

• Proper input validation and type checking
• No unsafe code or memory vulnerabilities
• Error handling prevents potential panics

📋 Recommendations

High Priority:

1. Add comprehensive unit tests covering:

   #[test]
   fn test_contains_list() { /* test list contains */ }
   
   #[test] 
   fn test_contains_text() { /* test text contains */ }
   
   #[test]
   fn test_contains_type_mismatch() { /* test error cases */ }

2. Add function documentation:

   /// Polymorphic contains function that works on both Lists and Text
   /// For Lists: checks if item exists in the list using equality comparison  
   /// For Text: checks if substring exists in the text
   pub fn native_contains(args: Vec<Value>) -> Result<Value, RuntimeError> {

Medium Priority:

1. Improve string dereferencing readability src/stdlib/list.rs:122:

   // Current: text.contains(&**substring)
   // Better:  text.contains(substring.as_ref())

2. Consider integration test to verify the original bug is fixed:

   • Test that both contains with list_var and item and contains with text_var and substring work in actual WFL programs

📊 Test Coverage Assessment

Current state: ⚠️ Insufficient

• Manual verification mentioned but no automated tests added
• Missing unit tests for the new polymorphic behavior
• Should include edge cases (empty lists/strings, type mismatches)

Required tests:

• List contains (existing item, non-existing item, empty list)
• Text contains (existing substring, non-existing substring, empty string)
• Error cases (wrong argument count, invalid types)

✅ Architecture Assessment

The architectural decision is sound:

• Single source of truth for contains logic ✅
• Polymorphic design aligns with WFL's dynamic typing ✅
• Follows existing patterns in the stdlib (e.g., native_length) ✅
• Maintains backward compatibility ✅

🎯 Final Recommendation

Conditional Approval - The core fix and refactor are excellent, but tests must be added before merging per repository guidelines.

This change fixes a legitimate bug and improves maintainability, but lacks the required test coverage that CLAUDE.md mandates for all features/fixes.

[coderabbitai]

📝 Walkthrough

Walkthrough

The contains function implementation was consolidated from the text module into the list module. The refactored function now supports both list and text arguments through type-based branching, with appropriate error handling for invalid argument types.

Changes

Cohort / File(s)	Summary
List contains consolidation src/stdlib/list.rs	Refactored native_contains to handle multiple input types via pattern matching: lists (iterates to check membership), text (checks substring containment), and error handling for unsupported types.
Text contains removal src/stdlib/text.rs	Removed native_contains function and its registration from register_text, relocating contains functionality to the list module.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A contains once split in two,
Now hops together, fresh and new!
Lists and strings in harmony bound,
One faithful function, round and round! 🌟

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: unifying the contains function to handle both List and Text types, eliminating duplication and fixing the bug where text.rs registration was overwritten.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor/unified-contains-7954133854785299130

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR fixes a bug where the contains function was broken for Text types due to duplicate registrations in separate modules. The refactoring consolidates the logic into a single polymorphic implementation that handles both List and Text types.

Changes:

• Removed duplicate native_contains function and registration from text.rs
• Refactored native_contains in list.rs to handle both List and Text types polymorphically
• Fixed the namespace collision that caused the Text version of contains to be overwritten

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/stdlib/text.rs	Removed duplicate native_contains function and its registration
src/stdlib/list.rs	Enhanced native_contains to polymorphically handle both List and Text types

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The double dereference &**substring is unclear and could be simplified. Consider using substring.as_ref() for better readability.

Suggested change

	Ok(Value::Bool(text.contains(&**substring)))
	Ok(Value::Bool(text.contains(substring.as_ref())))