Fix #3119. Don't expose ListValue<T> lists to users

[sug44]

Use Collections.Generic.List<T> instead of ListValue<T>. Return a ListValue copy of these lists to user code.

Changes:
FileContent:binary
Lexicon:keys
Lexicon:values
Part:children
Stage:resources
Stage:resourceslex
String:split
Structure:suffixnames
Timewarp:ratelist
Timewarp:railsratelist
Timewarp:physicsratelist
Vessel:parts
Vessel:dockingports
Vessel:decouplers (separators)
Vessel:resources
allnodes bound variable
allwaypoints function

Stage:resources, Stage:resourceslex, Vessel:parts, Vessel:dockingports, Vessel:decouplers now return copies instead of references to internal lists. All lists returned by these suffixes don't get modified over time so the only way to tell that they are copies now is to use the equality operator. Stage:resources and Stage:resourceslex returning a reference was a bug.

[JonnyOThan]

I'm a little concerned about performance for this one...is there maybe a need for a "readonly list" that these can return, that is easy to construct a copy from if you need it?

[sug44]

Yes I considered it too. Basically its a tradeoff for consistency over some performance. I want to avoid people having to stumble on these few suffixes that return readonly references and not copies you can do anything with.

[darthgently]

Agree with JonnyOThan. I think this is a documentation fix not a code fix. The docs just need to make clear they are read only (if they don’t already). What is the use case for manipulating these lists that can’t be done with a deep copy when needed? They are used far more often to merely read the current list so not worth the performance hit imho

[nuggreat]

It seams to me the better solution from a performance stand point (assuming we want these lists to be modifiable in the first place) would be to preform the cast to a generic list when someone preforms a modification on one of the read only lists as apposed to on the initial list construction, which should only modify the suffixes: :ADD(), :INSERT(), :REMOVE(), :CLEAR(). and :COPY(). This would also cover any existing addons that create/return there own lists ListValue<T> structures which modifying how just kOS returns lists would not.

It is also a rare operation that some one wants to modify these lists as I only ever encountered the one person asking after the problem which is what prompted me to make the issue in the first place. And it is a far more common operation for someone to do as part of main loops

FOR p IN SHIP:PARTS {
  ...
}

or

IF SHIP:PARTS < something {
  ...
}

so avoiding performance impact on these would be good.

[JonnyOThan]

Yeah copy-on-write is another method, but more complex to implement and maintain. No objections though if someone wants to try it.

[JonnyOThan]

So is the ListValue<T> -> ListValue conversion still useful? I'm also a little wary of removing typesafety.

[sug44]

It would be perfect to:

• Not expose ListValue<T> to users
• Not create extra copies of lists for performance
• Keep typesafety

But I don't think there is an easy way to achieve all three. Keeping typesafety seems less important

[sug44]

Basically in the first commit I was prioritizing the first and the third point, now I prioritize the first and the second point. If there was some middle ground I missed let me know

[JonnyOThan]

Why is it good to not expose ListValue<T> to users?

[JonnyOThan]

OK I read through #3119 and I think I understand...the type returned by those suffixes have a hidden restriction beyond what one would expect from the KOS List type? And since most of them are already making copies of lists, there's no performance issue.

I think I buy that argument, and I'm not too worried about a performance cost for changing dockingports and decouplers since those aren't used all that often...but introducing a copy for every access to ship:parts seems like it might be an issue.

[sug44]

I did revert some changes from the first commit and now these ship suffixes don't create a copy. Is there anything else that needs to be addressed?

[JonnyOThan]

Ah yes I see that VesselTarget.Parts isn't a copy, cool.

But maybe I'm still confused...if the return value is not a copy then users shouldn't be adding things to it anyway, right? So there's not a problem if it's a specialized list type. If it IS a copy then it should be ListValue because the user could add things to it of any type. Do I have that right? So should vessel:parts still return ListValue<Part> ? And then we don't need to lose the typesafety on the C# side.

I'm also open to the possibility that suffixes that don't return copies should return something that isn't a list at all, but maybe that's a different conversation.

[8110740497]

Si El sáb, 1 de feb. de 2025 11:42 a. m., JonnyOThan ***@***.***> escribió:

…

Maybe I'm still confused...if the return value is not a copy then users shouldn't be adding things to it anyway, right? So there's not a problem if it's a specialized list type. If it IS a copy then it should be ListValue because the user could add things to it of any type. Do I have that right? So should vessel:parts still return ListValue<Part> ? And then we don't need to lose the typesafety on the C# side. — Reply to this email directly, view it on GitHub <#3129 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BLE7DNQPJKMZKZ7U5YFIZMT2NUBQZAVCNFSM6AAAAABWE5WO3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMRZGA2DENRUHA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[sug44]

Yeah you have that right. There are 3 main problems with leaving parts suffix ListValue<Part>. One is that a :copy of it will also return a ListValue<Part> list. Two is that C# gets mad at users adding elements of other types before kOS gets mad that they are trying to add an element to a read-only list. Three is that a :typename on these lists is bugged and returns ListValue'1 adding to the confusion. I'm confident there is no bug there right now that is caused by the lack of typesafety. But I do agree that its a ticking time bomb before someone introduces it. Just need to make sure that there are no bugs from the point of creation of the list to the point where its set as read-only. Maybe the solution is to put a warning there? It's also possible that the performance hit of copying is not as bad and it's better to take it reducing complexity for both users and us.

[JonnyOThan]

Thanks for explaining that. I think losing typesafety is the least bad outcome here.

I think maybe we need a refactoring where ListValue is a base type that manages possibly heterogeneous lists, and ListValue<T> can inherit from it and enforce typesafety for use in C#. But that can be done later.