[PR] Add Maybe[T].toArray and fix DelimiterTextParser array aliasing [daffodil]

["olabusayoT (via GitHub)" <gi...@apache.org>]

olabusayoT opened a new pull request, #1720:
URL: https://github.com/apache/daffodil/pull/1720

   Added Maybe[T].toArray, replacing seven hand-rolled .toList.toArray call sites across the runtimeDependencies migration to Array (EvElement, EvEscapeSchemes, EvFieldDFA, StringLiteralForUnparser, SpecifiedLength2, ElementUnparser x2) with a single reusable method.
   
   DelimiterTextParser.runtimeDependencies now .clone()s EncodingRuntimeData's array instead of aliasing the same mutable instance: Array is invariant and mutable, unlike the Vector this used to be, so sharing the identical instance would let a future in-place mutation (e.g. dedupe or append) silently corrupt every sibling delimiter parser's dependency list along with EncodingRuntimeData's own.
   
   DAFFODIL-3065


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add Maybe[T].toArray and fix DelimiterTextParser array aliasing [daffodil]

["stevedlawrence (via GitHub)" <gi...@apache.org>]

stevedlawrence commented on code in PR #1720:
URL: https://github.com/apache/daffodil/pull/1720#discussion_r3856578440


##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/DelimiterParsers.scala:
##########
@@ -51,7 +51,10 @@ class DelimiterTextParser(
   mustMatchNonZeroData: Boolean
 ) extends TextPrimParser {
 
-  override val runtimeDependencies = rd.encodingInfo.runtimeDependencies
+  // .clone(), not the same reference: Array is mutable, unlike the
+  // Vector this used to be, so sharing the instance would let a future
+  // in-place mutation corrupt every sibling delimiter parser's list too.
+  override val runtimeDependencies = rd.encodingInfo.runtimeDependencies.clone()

Review Comment:
   I don't think this clone is really necessary. Although runtimeDependencies are mutable Arrays, I don't think we ever actually mutate them.
   
   And even if encodingInfo.runtimeDependencies did change, it's probably more correct for this runtimeDependencies to also update, so there's an argument that cloning is actually the wrong behavior. But we don't have that behavior for other runtimeDependnecies, so I don't think we're really consistent with that behavior.
   
   If we wanted to ensure it's never mutated we could change the type to Scala 3's `IArray` (immutable array), but I'm not sure if the churn is worth it. It's not too hard to inspect all the runtimeDependnecies and verify they never change.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add Maybe[T].toArray and fix DelimiterTextParser array aliasing [daffodil]

["olabusayoT (via GitHub)" <gi...@apache.org>]

olabusayoT merged PR #1720:
URL: https://github.com/apache/daffodil/pull/1720


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org