Skip to content

refactor: apply for-comprehension pattern across collections and argparse#3448

Closed
bobzhang wants to merge 4 commits intomainfrom
hongbo/comprehensions-combined
Closed

refactor: apply for-comprehension pattern across collections and argparse#3448
bobzhang wants to merge 4 commits intomainfrom
hongbo/comprehensions-combined

Conversation

@bobzhang
Copy link
Copy Markdown
Contributor

@bobzhang bobzhang commented Apr 21, 2026

Summary

Follow-up to #3444 consolidating three stacked batches (originally #3445, #3446, #3447 — superseded by this PR) plus an additional aggressive sweep of simple unconditional-map loops. Converts imperative `let xs = []` / `Array::new(...)` + `for ... { xs.push(...) }` patterns to MoonBit array comprehensions.

Sites converted (18 files)

argparse:

  • `help_render.mbt`: `positional_entries`, `subcommand_entries`, `group_members` (filter-map)
  • `parser.mbt`: `group_usage_expr` (`is Some(idx)` binding)
  • `parser_lookup.mbt`: `split_long` (simple map)
  • `parser_positionals.mbt`: `positional_args` (pattern filter)

core collections:

  • `deque/deque.mbt`: `ToJson` impl
  • `hashset/hashset.mbt`: `to_array` (`is Some({ key, .. })` binding), `ToJson` impl
  • `list/list.mbt`: `to_array`, `ToJson` impl
  • `priority_queue/priority_queue.mbt`: `ToJson` impl
  • `set/linked_hash_set.mbt`: `ToJson` impl
  • `sorted_map/map.mbt`: `to_array`
  • `sorted_map/deprecated.mbt`: `keys`, `values` (two-binding `for k, _ in self` / `for _, v in self`)

immut collections:

  • `immut/hashmap/HAMT.mbt`: `to_array` (two-binding)
  • `immut/priority_queue/types.mbt`: `ToJson` impl
  • `immut/sorted_map/map.mbt`: `to_array` (two-binding)
  • `immut/sorted_set/immutable_set.mbt`: `ToJson` impl
  • `immut/vector/vector.mbt`: `ToJson` impl

misc:

  • `bytes/internal/regex_engine/translate.mbt`: `Alternation` arm (`is (cr, pref2)` destructure)
  • `env/env_native.mbt`: `get_cli_args_internal`

Patterns used

  • Unconditional map: `[for x in iter => f(x)]`
  • Filter-map: `[for x in iter if cond => f(x)]`
  • Is-pattern binding: `[for x in iter if f(x) is Some(y) => g(y)]`
  • Two-binding for Iter2: `[for k, v in map => (k, v)]`

Discovered limitation

MoonBit rejects calls to raising functions inside comprehensions:

`error: calling function with error is not allowed inside list comprehension`

This blocks conversion of `Array::filter`, `Array::filter_map`, `ArrayView::filter`, `ArrayView::filter_map`, and similar functions whose predicate/mapper closures are typed `(T) -> ... raise?`. Those are intentionally left as imperative loops.

Sites deliberately skipped

  • `Array::new(capacity=...)` hot paths where the explicit capacity hint is load-bearing (`Deque::to_array`, `Vector::to_array`, `Array::extract_if`, `Array::split`).
  • `parser_globals_merge` two-optional-loop concat — cleaner as-is.
  • Closure-based `each_intervals` callbacks in regex symbol_map.

Test plan

  • `moon check` clean
  • `moon test` passes all 6333 tests
  • `moon fmt --check` clean
  • `codex review --uncommitted` confirms behavior-preserving (no correctness, API, or performance regressions)

Supersedes #3445, #3446, #3447.

🤖 Generated with Claude Code


Open in Devin Review

bobzhang and others added 4 commits April 21, 2026 08:19
Apply the same for-comprehension pattern used in required_option_usage
to other places where an imperative loop accumulates into an array:

- argparse/help_render.mbt: positional_entries, subcommand_entries, group_members
- priority_queue: ToJson implementation
- bytes regex_engine translate: Alternation arm

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Second follow-up to #3444, extending the same pattern to:

- argparse/parser.mbt: group_usage_expr
- argparse/parser_lookup.mbt: split_long
- env/env_native.mbt: get_cli_args_internal
- hashset/hashset.mbt: ToJson impl
- set/linked_hash_set.mbt: ToJson impl

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Third follow-up to #3444, applying the pattern more aggressively:

- argparse/parser_positionals.mbt: positional_args
- hashset/hashset.mbt: HashSet::to_array (drops explicit capacity hint)
- immut/sorted_set/immutable_set.mbt: ToJson impl
- list/list.mbt: ToJson impl
- sorted_map/deprecated.mbt: keys, values (uses Iter2 two-binding for)
- sorted_map/map.mbt: to_array

Array comprehensions do not allow calling functions that can raise, so
Array::filter, Array::filter_map, and their ArrayView counterparts are
left untouched.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Additional sites found after a broader sweep:

- deque/deque.mbt: ToJson impl
- immut/hashmap/HAMT.mbt: HashMap::to_array (two-binding)
- immut/priority_queue/types.mbt: ToJson impl
- immut/sorted_map/map.mbt: SortedMap::to_array (two-binding)
- immut/vector/vector.mbt: ToJson impl
- list/list.mbt: List::to_array

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coveralls
Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 3877

Coverage decreased (-0.008%) to 94.906%

Details

  • Coverage decreased (-0.008%) from the base build.
  • Patch coverage: 30 of 30 lines across 16 files are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 15507
Covered Lines: 14717
Line Coverage: 94.91%
Coverage Strength: 220574.09 hits per line

💛 - Coveralls

Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

Open in Devin Review

@bobzhang bobzhang closed this Apr 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants