An each over a fresh String Array whose answer is read stores each mutated element back

Makefile

@@ -1385,7 +1385,7 @@ link-names-test: $(SPINEL) $(SP_RT_LIB) $(SP_RT_MT_LIB)
 
 reject-test: $(SPINEL)
 	@ok=1; tmp=$$(mktemp -d /tmp/spinel-reject.XXXXXX); \
-	for t in test/reject/string_thread_arg.rb test/reject/string_fiber_arg.rb test/reject/string_thread_global_arg.rb test/reject/string_global_hash_element_mutation.rb test/reject/string_thread_ivar_arg.rb test/reject/string_thread_method_param_arg.rb test/reject/string_fiber_method_param_arg.rb test/reject/string_thread_block_param_arg.rb test/reject/string_thread_arg_in_loop.rb; do \
+	for t in test/reject/string_thread_arg.rb test/reject/string_fiber_arg.rb test/reject/string_thread_global_arg.rb test/reject/string_global_hash_element_mutation.rb test/reject/string_thread_ivar_arg.rb test/reject/string_thread_method_param_arg.rb test/reject/string_fiber_method_param_arg.rb test/reject/string_thread_block_param_arg.rb test/reject/string_thread_arg_in_loop.rb test/reject/string_split_local_each_mutation.rb test/reject/string_split_select_mutation.rb test/reject/string_split_map_bang_read_before.rb; do \
 	  if $(SPINEL) "$$t" -c --no-line-map -o "$$tmp/sk.c" >"$$tmp/sk.out" 2>&1; then \
 	    echo "reject-test: FAIL ($$t compiled)"; ok=0; \
 	  else grep -q "is not yet shared by reference" "$$tmp/sk.out" || \

src/analyze.c

@@ -18490,6 +18490,136 @@ static void an_mark_handle_returns(Compiler *c) {
     if (poly) c->strbuf_box[n] = 0;
   }
 }
+/* A String builtin answering a new Array of new Strings nothing else holds
+   (`s.split(",")`, `s.scan(re)`, `s.lines`, `s.chars`). */
+static int sb_fresh_string_array_call(Compiler *c, int n) {
+  const NodeTable *nt = c->nt;
+  n = an_unparen(nt, n);
+  if (n < 0 || nt_kind(nt, n) != NK_CallNode || nt_ref(nt, n, "block") >= 0) return 0;
+  const char *mn = nt_str(nt, n, "name");
+  int r = nt_ref(nt, n, "receiver");
+  if (!mn || r < 0) return 0;
+  if (!sp_streq(mn, "split") && !sp_streq(mn, "scan") && !sp_streq(mn, "lines") && !sp_streq(mn, "chars"))
+    return 0;
+  TyKind rt = infer_type(c, r);
+  return (rt == TY_STRING || rt == TY_STRBUF) && infer_type(c, n) == TY_STR_ARRAY;
+}
+static int sb_subtree_max(const NodeTable *nt, int n) {
+  if (n < 0) return -1;
+  int m = n;
+  const SpNode *nd = &nt->nodes[n];
+  for (int i = 0; i < nd->nr; i++) { int x = sb_subtree_max(nt, nd->r[i].ref); if (x > m) m = x; }
+  for (int i = 0; i < nd->na; i++)
+    for (int j = 0; j < nd->a[i].n; j++) { int x = sb_subtree_max(nt, nd->a[i].ids[j]); if (x > m) m = x; }
+  return m;
+}
+/* map!/collect! over local `vn` (call w) mutating each element it yields:
+   the elements are replaced by the block's values, so a mutation of the
+   yielded String is seen only where the block's value carries it -- unless
+   the String is reachable some other way. It is not when the local is only
+   ever bound to a fresh String Array (sb_fresh_string_array_call) ahead of
+   w, every other read of it comes after w (none can hand an element out
+   before the replacement), and no loop around w runs it again over Strings
+   a later read may have handed out. */
+static int sb_map_bang_local_unobserved(Compiler *c, int w, int recv, const char *vn, Scope *vs) {
+  const NodeTable *nt = c->nt;
+  if (!vs || vs->body < 0 || !an_subtree_has(nt, vs->body, w)) return 0;
+  int wmax = sb_subtree_max(nt, w), writes = 0;
+  for (int u = comp_kind_first(c, NK_LocalVariableWriteNode); u >= 0; u = comp_kind_next(c, u)) {
+    if (nt_kind(nt, u) != NK_LocalVariableWriteNode || !sp_streq(nt_str(nt, u, "name"), vn)) continue;
+    if (!an_subtree_has(nt, vs->body, u)) continue;
+    if (u > w || !sb_fresh_string_array_call(c, nt_ref(nt, u, "value"))) return 0;
+    writes++;
+  }
+  if (writes == 0) return 0;
+  NodeKind other[] = { NK_LocalVariableOrWriteNode, NK_LocalVariableAndWriteNode,
+                       NK_LocalVariableOperatorWriteNode, NK_LocalVariableTargetNode };
+  for (int k = 0; k < (int)(sizeof other / sizeof other[0]); k++)
+    for (int u = comp_kind_first(c, other[k]); u >= 0; u = comp_kind_next(c, u))
+      if (nt_kind(nt, u) == other[k] && sp_streq(nt_str(nt, u, "name"), vn) &&
+          an_subtree_has(nt, vs->body, u)) return 0;
+  for (int u = comp_kind_first(c, NK_LocalVariableReadNode); u >= 0; u = comp_kind_next(c, u)) {
+    if (u == recv || nt_kind(nt, u) != NK_LocalVariableReadNode || !sp_streq(nt_str(nt, u, "name"), vn)) continue;
+    if (u <= wmax && an_subtree_has(nt, vs->body, u)) return 0;
+  }
+  NodeKind loops[] = { NK_WhileNode, NK_UntilNode, NK_ForNode };
+  for (int k = 0; k < 3; k++)
+    for (int u = comp_kind_first(c, loops[k]); u >= 0; u = comp_kind_next(c, u))
+      if (nt_kind(nt, u) == loops[k] && an_subtree_has(nt, u, w)) return 0;
+  /* w runs where the local lives, not in a block that may run it again */
+  return comp_scope_of(c, w) == vs;
+}
+/* Does the subtree at n hold a node that leaves a block iteration early
+   (next, redo, retry) or writes local `pn`? */
+static int sb_block_body_blocks_rewrite(const NodeTable *nt, int n, const char *pn) {
+  if (n < 0) return 0;
+  NodeKind k = nt_kind(nt, n);
+  if (k == NK_NextNode || k == NK_RedoNode || k == NK_RetryNode) return 1;
+  if ((k == NK_LocalVariableWriteNode || k == NK_LocalVariableOrWriteNode ||
+       k == NK_LocalVariableAndWriteNode || k == NK_LocalVariableOperatorWriteNode ||
+       k == NK_LocalVariableTargetNode) && sp_streq(nt_str(nt, n, "name"), pn)) return 1;
+  const SpNode *nd = &nt->nodes[n];
+  for (int i = 0; i < nd->nr; i++)
+    if (sb_block_body_blocks_rewrite(nt, nd->r[i].ref, pn)) return 1;
+  for (int i = 0; i < nd->na; i++)
+    for (int j = 0; j < nd->a[i].n; j++)
+      if (sb_block_body_blocks_rewrite(nt, nd->a[i].ids[j], pn)) return 1;
+  return 0;
+}
+/* `s.scan(re).each { |x| x.gsub!(...) }.join`: each answers the fresh Array
+   it walked, and its answer is read, so the in-place mutation of each
+   element must be seen there -- but a String Array's element binds the
+   parameter as a copy. Nothing else holds that Array or its Strings, so
+   storing the parameter's final value back over its element is the same
+   program: the block becomes `{ |x| ...; x }` under map!, which answers the
+   Array too. Only a block that runs to its end every time (no next, redo or
+   retry) and never rebinds the parameter; a break answers the break value
+   either way and leaves the Array unread. */
+static int desugar_fresh_array_each_writeback(Compiler *c) {
+  NodeTable *nt = (NodeTable *)c->nt;
+  int n0 = nt->count, changed = 0;
+  for (int w = comp_kind_first(c, NK_CallNode); w >= 0 && w < n0; w = comp_kind_next(c, w)) {
+    if (nt_kind(nt, w) != NK_CallNode || !sp_streq(nt_str(nt, w, "name"), "each")) continue;
+    int blk = nt_ref(nt, w, "block"), recv = nt_ref(nt, w, "receiver");
+    if (blk < 0 || nt_kind(nt, blk) != NK_BlockNode || nt_ref(nt, w, "arguments") >= 0) continue;
+    if (comp_value_dropped(c, w) || !sb_fresh_string_array_call(c, recv)) continue;
+    int bp = nt_ref(nt, blk, "parameters");
+    if (bp < 0 || nt_kind(nt, bp) != NK_BlockParametersNode) continue;
+    int pn = nt_ref(nt, bp, "parameters");
+    if (pn < 0) continue;
+    int nreq = 0, nopt = 0, npost = 0, nkw = 0;
+    const int *req = nt_arr(nt, pn, "requireds", &nreq);
+    nt_arr(nt, pn, "optionals", &nopt); nt_arr(nt, pn, "posts", &npost); nt_arr(nt, pn, "keywords", &nkw);
+    if (nreq != 1 || nopt || npost || nkw || nt_ref(nt, pn, "rest") >= 0 ||
+        nt_ref(nt, pn, "keyword_rest") >= 0 || nt_ref(nt, pn, "block") >= 0 ||
+        nt_kind(nt, req[0]) != NK_RequiredParameterNode) continue;
+    const char *pname = nt_str(nt, req[0], "name");
+    Scope *bs = comp_scope_of(c, blk);
+    int body = nt_ref(nt, blk, "body");
+    if (!pname || !bs || body < 0 || nt_kind(nt, body) != NK_StatementsNode) continue;
+    if (strbuf_mut_kind(c, pname, bs) != 1 || sb_block_body_blocks_rewrite(nt, body, pname)) continue;
+    int bn = 0; const int *bb = nt_arr(nt, body, "body", &bn);
+    int base = nt->count;
+    int rd = nt_new_node(nt, "LocalVariableReadNode");
+    if (rd < 0) continue;
+    nt_node_set_str(nt, rd, "name", pname);
+    nt_node_set_int(nt, rd, "depth", 0);
+    int *nb = malloc(sizeof(int) * (size_t)(bn + 1));
+    if (!nb) { fprintf(stderr, "spinel: out of memory\n"); exit(1); }
+    memcpy(nb, bb, sizeof(int) * (size_t)bn);
+    nb[bn] = rd;
+    int st = nt_new_node(nt, "StatementsNode");
+    if (st < 0) { free(nb); continue; }
+    nt_node_set_arr(nt, st, "body", nb, bn + 1);
+    free(nb);
+    nt_node_set_ref(nt, blk, "body", st);
+    nt_set_str(nt, w, "name", "map!");
+    comp_grow_node_arrays(c);
+    for (int j = base; j < nt->count; j++) c->nscope[j] = c->nscope[blk];
+    changed = 1;
+  }
+  return changed;
+}
 static int promote_shared_stored_strings(Compiler *c) {
   int changed = 0;
   /* --share-strings: the one rule decides first (#6765) */
@@ -19173,6 +19303,20 @@ static int promote_shared_stored_strings(Compiler *c) {
       for (int e = 0; e < en && !var; e++) var = nt_kind(nt, el[e]) == NK_LocalVariableReadNode;
       if (!var) continue;
     }
+    /* A fresh String Array a call answers (`s.split(",").map! { |t| t.strip!
+       ... }`) that nothing else holds: a mutation of the String an iterator
+       yields is seen only through the block's own value when the
+       iterator's answer is dropped, or is the block's values (map, and
+       map!/collect!, which store them over the elements). An iterator
+       answering the Array or some of its elements (each, select, find) is
+       read again where its answer is used, and stays refused here
+       (desugar_fresh_array_each_writeback takes each's). The parameter then
+       binds the element as a copy, as for any read. */
+    if ((bpv4->type == TY_STRING || bpv4->type == TY_STRBUF) && sb_fresh_string_array_call(c, recv4) &&
+        (is_map_bang_alias(itn) || comp_value_dropped(c, w) ||
+         bop_share_named(BOP_ANY_ARRAY, itn) == BSH_ITER_MAP ||
+         bop_share_named(BOP_ANY_ARRAY, itn) == BSH_ITER_MAP_BANG))
+      continue;
     if ((bpv4->type == TY_STRING || bpv4->type == TY_STRBUF) &&
         (nt_kind(nt, recv4) == NK_InstanceVariableReadNode || nt_kind(nt, recv4) == NK_CallNode) &&
         (infer_type(c, recv4) == TY_STR_ARRAY || infer_type(c, recv4) == TY_POLY_ARRAY)) {
@@ -19226,6 +19370,12 @@ static int promote_shared_stored_strings(Compiler *c) {
        (settled or literal) element type so an array-of-arrays each+<<
        never promotes its param */
     if (contt4 != TY_STR_ARRAY && contt4 != TY_POLY_ARRAY) continue;
+    /* map!/collect! over a local only ever bound to a fresh String Array,
+       read nowhere before the replacement: the block's value is what each
+       element becomes, and the mutated String is reachable only through it */
+    if (!lit4 && contt4 == TY_STR_ARRAY && is_map_bang_alias(itn) &&
+        sb_map_bang_local_unobserved(c, w, recv4, contn4, conts4))
+      continue;
     if (!lit4 && contt4 == TY_STR_ARRAY &&
         an_local_string_array_has_untracked_call_store(c, contn4, conts4)) {
       static const char sa_msg[] =
@@ -34649,6 +34799,7 @@ static void an_phase_infer_fixpoint(Compiler *c) {
     ch |= desugar_enum_iter_splat_args(c);     /* enum.map(*a, &b) -> enum.map(&b) */
     ch |= desugar_enum_pair_lone_param(c);     /* a.each_with_index.map { |x| } -> { |x, i| } */
     ch |= desugar_builtin_iter_block_shapes(c);  /* [1].each { |c, a = 10| } -> { |v| c = v; a = 10 } */
+    ch |= desugar_fresh_array_each_writeback(c); /* s.scan(re).each { |x| x.gsub!(..) }.join -> map! { |x| ..; x } */
     ch |= desugar_multi_yield_map_param(c);    /* multi-yield each: map's |x| takes the 1st */
     ch |= desugar_ewi_pack_values(c);          /* multi-yield each: each_with_index packs */
     ch |= desugar_enum_walk_calls(c);          /* enum.map { break } -> __enumw_map(enum) { } */

test/fresh_string_array_element_mutation.rb

@@ -0,0 +1,45 @@
+# A String yielded out of a fresh Array (split, scan) and mutated in place
+# where nothing can read the mutation back through that Array: the Array is
+# a call's temporary whose iterator answer is dropped or is the block's
+# values, or a map!/collect! stores each block value over its element.
+# These were refused ("not yet shared by reference") although a copy gives
+# the same answer. Actionview's highlight and split_paragraphs and rack's
+# parse_http_accept_header are written so.
+
+# highlight: each answers the scanned Array and join reads it, so each
+# mutated element is stored back (each { |x| ...; } runs as map! { |x| ...; x })
+def highlight(text)
+  text.scan(/<[^>]*|[^<]+/).each do |segment|
+    if !segment.start_with?("<")
+      segment.gsub!(/b/, "B")
+    end
+  end.join
+end
+p highlight("ab<i>cb</i>")
+
+# split_paragraphs: map! keeps the block's value
+def split_paragraphs(text)
+  text.to_str.gsub(/\r\n?/, "\n").split(/\n\n+/).map! { |t| t.gsub!(/([^\n]\n)(?=[^\n])/, '\1<br />') || t }
+end
+p split_paragraphs("a\nb\n\nc")
+
+# parse_http_accept_header: a local bound to split and read only after map!
+def accept(header)
+  parts = header.to_s.split(",")
+  parts.map! { |part|
+    part.strip!
+    next if part.empty?
+    part.downcase
+  }
+  parts.compact!
+  parts
+end
+p accept("text/html, ,Application/JSON ")
+
+s = " a , b ,c "
+s.split(",").each { |x| x.strip!; print x, ";" }
+puts
+p s.split(",").map { |x| x.strip!; x.upcase }
+p s.split(",").each { |x| x.strip! }
+p "x\ny\n".lines.each(&:chomp!)
+p s

test/fresh_string_array_element_mutation.rb.expected

@@ -0,0 +1,8 @@
+"aB<i>cB</i>"
+["a\n<br />b", "c"]
+["text/html", "application/json"]
+a;b;c;
+["A", "B", "C"]
+["a", "b", "c"]
+["x", "y"]
+" a , b ,c "

test/reject/string_split_local_each_mutation.rb

@@ -0,0 +1,7 @@
+# A local bound to split's Array whose Strings an each block strips in
+# place, and that is read afterwards: each keeps the elements, so the
+# mutation must be seen through the Array. Still refused, not silently
+# applied to a copy.
+a = " x , y ".split(",")
+a.each(&:strip!)
+p a

test/reject/string_split_map_bang_read_before.rb

@@ -0,0 +1,7 @@
+# map! over a local whose element was read out before the replacement: the
+# String it handed out is the one map!'s block mutates in place. Refused,
+# not silently applied to a copy.
+parts = " a , b ".split(",")
+first = parts[0]
+parts.map! { |x| x.strip! || x }
+p first, parts

test/reject/string_split_select_mutation.rb

@@ -0,0 +1,4 @@
+# select answers some of the very Strings its block mutated in place, so
+# the mutation is read back through its answer: refused, not silently
+# applied to a copy.
+p " a , b ".split(",").select { |x| x.strip!; true }