Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Extended Collection Types #2

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 169 additions & 0 deletions docs/extended-collection-types.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
# Extended Collection Types

| Lead Author(s) | Implemented | GitHub Links |
|---|---|---|
| ike709 | :x: No | TBD |

## Overview

Currently, the only collection type in BYOND is the `list()` type, which doubles as both a dynamically resizable list and also as a dictionary. This RFC proposes exposing several additional collection types directly from C# to DM.

Note that typed collections (e.g. an array of text strings) will be implemented as a future RFC; this RFC primarily focuses on *which* collection types to add in the first place.

## Motivation

The implementation of BYOND lists is highly inefficient in OpenDream as we can't just back it with a C# list or a C# dictionary. Furthermore, other collection types would offer performant convenience features for developers that they do not need to implement themselves, such as performant hashsets or stacks.

## Design

The current `list()` type will remain unchanged for BYOND compatibility, with the other types being added *in addition to* these legacy lists.

The following collection types will essentially be exposed directly from C# to DM through a minimal wrapper API:

- **Array**: A fixed-length collection of elements.
- **Dictionary**: A collection of key-value pairs. Equivalent to existing associative lists, and will serve as the internal implementation of BYOND's planned `alist()` feature.
- **HashSet**: A collection that contains no duplicate elements.
- **ODList**: A dynamically resizable collection that is purely a list with none of the assoc-list functionality that BYOND lists have.
- **Queue**: A first-in first-out (FIFO) collection.
- **Stack**: A last-in first-out (LIFO) collection.

Generally speaking, the helper procs from `list()` will also operate on other collections where applicable (e.g. `list.Join()` and `array.Join()` would be equivalent). A few collections will implement additional helpers as noted below.

**Note:** Initializing OpenDream's collection types with a fixed size, for example `var/array/foo[10]`, will initialize it with a `capacity` of 10 but *will not* insert any `null` elements; the actual contents of the collection will be empty. This is **breaking behavior** from BYOND lists such as `var/list/L[10]`, which inserts 10 `null` elements into the list. BYOND lists in OpenDream will preserve this behavior for compatibility. Otherwise, see the "Collection Capacity API" subsection below for more information.

This comment was marked as outdated.

Comment on lines +31 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, would it be reasonable to have something like

var/array/arr[2, 3]
arr = { { 1, 2, 3 }, { 4, 5, 6 } }

(basically int[,] multiDimensionalArray = new int[2, 3];)
if it's decided to keep the var/vec/foo[10] syntax around

Copy link

Choose a reason for hiding this comment

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

How is it possible for a "fixed-length" collection of elements (the array) to have a capacity of 10 but contain no elements? I believe it should be populated with 10 nulls.

Choose a reason for hiding this comment

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

How is it possible for a "fixed-length" collection of elements (the array) to have a capacity of 10 but contain no elements? I believe it should be populated with 10 nulls.

This has to do with memory allocation, and the size of the memory allocated for the collection vs its actual contents. Capacity of 10 means it has space set aside for 10 elements, but may have any number of elements between 0 and 10.

This is what any modern language does, and DM itself likely does it internally without telling the user about it (allowing the user to see it means they can give the VM an advisory on how much memory they expect to need, and it can allocate it all ahead of time.)

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

yeah but my point is that an array can not be resized, so it doesn't make sense for arrays specifically to have a capacity (at least, a capacity differing from its size)

Copy link

Choose a reason for hiding this comment

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

Thinking about this more, I really don't see how it makes sense for other containers either. Do any other languages exist with this kind of syntax for specifying the initial capacity of a container?

Is the [10] expected to be part of the type? I don't think that initial capacity should have anything to do with the type of the variable. I'd argue that this syntax should be entirely dropped for these new data types and moved to the right-hand-side of the variable declaration inside a default expression (somehow, I can't think of the nicest approach), where it probably belongs. This would also allow the feature to be used in places that are not variable declarations.

tbh BYOND's current behaviour makes far more sense than using this for capacity, although it's still weird.

The syntax maybe makes sense for non-reassignable array members, assuming it sets the size rather than initial capacity, but I don't think we need those.

Choose a reason for hiding this comment

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

hm. I'm no longer sure either, can you bring this up in the discord?


### Array

A fixed-length collection of elements. Almost identical to `list()` except it cannot be resized.

Comment on lines +36 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear "almost identical" also means no assoc behaviour for this yes

Example usage:
```
var/array/foo = array("a","b","c")
foo += "z" // Compile-time error, cannot resize arrays

world.log << foo.Join() // Prints "abc"

foo[1] = "z" // Valid modification
```

### Dictionary
A collection of key-value pairs. Equivalent to existing associative lists, and will serve as the internal implementation of BYOND's planned `alist()` feature.

Choose a reason for hiding this comment

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

Can you enumerate over dictionary types without using .keys or .values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great question!


Example usage:
```
var/dict/bar = array("a" = 1,"b" = 2,"c" = 3)
ike709 marked this conversation as resolved.
Show resolved Hide resolved
bar += "d" // Compile-time error, no value specified

bar["d"] = 4 // Valid modification

world.log << bar["c"] // Prints 3

world.log << bar["test"] // Runtime error, key not found

bar.contains_key("test") // Returns FALSE

Choose a reason for hiding this comment

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

Would it not be more dmish to do "test" in bar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Presumably we would support both.

Copy link

Choose a reason for hiding this comment

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

I just don't see the need for an extra function if there's already a byondism. Keep in mind .Find exists too.

Copy link
Collaborator Author

@ike709 ike709 May 6, 2024

Choose a reason for hiding this comment

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

I feel like just straight reusing Find() could be a bit ambiguous but FindKey() would be better than contains_key().

Choose a reason for hiding this comment

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

Find by itself could be confused with value searching


bar.get_value("test") // Returns null, since the key doesn't exist

world.log << bar[1] // Compile-time error, dicts can't be numerically indexed

Choose a reason for hiding this comment

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

Why are we not allowing dictionaries to have a numerical key? That's how it works with the existing lists, but this is an annoying limitation with the current system and if there is going to be a specific dictionary type, I see no reason why we have to block of numerical keys (since we can now identify between what the user really wants based on whether or not they ask for a dict or an array)

Copy link
Collaborator Author

@ike709 ike709 May 6, 2024

Choose a reason for hiding this comment

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

Dictionaries aren't ordered in C# and we're just exposing the C# type to DM.

Choose a reason for hiding this comment

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

I mean, what if we have cases where we want to actually have an numerical key. Within current byond, we have to cast the number to a string and store it in the dictionary.

An example code of something that I think would be really useful to have, that would be an improvement over current byond:

var/dict/player_locations = dict(4 = dict(8 = odlist(new /mob)), 9 = dict(4 = odlist(new /mob)))
// Find the player at 4, 8
var/odlist/players = player_locations[4][8]

Choose a reason for hiding this comment

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

image

Copy link

Choose a reason for hiding this comment

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

Even without typed collections I would expect a dictionary type to have arbitrarily-typed DreamValue keys rather than just strings.

This would give similar reasons to use dict over list as there are to use JavaScript's Map type over normal JS objects.

```

`dict.keys` and `dict.values` will return a new `array()` of the dictionary's keys and values, respectively. Modifying these will not modify the existing dictionary.

Choose a reason for hiding this comment

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

If its not going to be linked, maybe it should return a constant list instead.

Copy link
Collaborator Author

@ike709 ike709 May 6, 2024

Choose a reason for hiding this comment

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

I have no strong opinion so I'll see if anyone else weighs in. I'm not sure what value a const-list has over an array though?

Copy link

Choose a reason for hiding this comment

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

It might be better to define these as being some kind of iterator so that the implementation of keys/values can be allocation-free.


`dict.contains_key(key)` will return `TRUE` or `FALSE` depending on if the `key` is present in the dictionary.

`dict.get_value(key)` will return the value of the specified `key` or `null` if the key is not present in the dictionary.

### HashSet
Essentially identical to an OD list, except it cannot contain duplicates. Inserting an element that is already present in the hashset is not an error and instead does nothing.

Direct insertion (e.g. `hashset += "foo"`) is supported, but `hashset.add(element)` will return `TRUE` or `FALSE` depending on if the element already existed in the hashset.

Example usage:
```
var/hashset/foo = hashset("a","b","c")

foo += "c" // Valid but nothing happens since `foo` already contains "c"

foo["c"] = 5 // Compile-time error, hashsets cannot be associative

world.log << foo.add("c") // Prints FALSE and inserts nothing since `foo` already contains "c"

world.log << foo.add("d") // Prints TRUE and inserts "d" into `foo`
world.log << foo.Join() // Prints "abcd"
ike709 marked this conversation as resolved.
Show resolved Hide resolved
```

### ODList
Identical to BYOND `list()` but with no support for associative lists. This facilitates various internal optimizations over using `list()`.

Choose a reason for hiding this comment

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

Can't we just optimise lists to work well when not associative? Like I'm a little bit sure we already do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We already do as much as we can, but further improvements can be made if we can guarantee a list will never become a dict and vice-versa.


Unlike BYOND lists, ODLists also implement the "Constant Collections" and "Collection Capacity API" defined in subsequent sections.

Example usage:
```
var/odlist/foo = odlist("a","b","c")

Choose a reason for hiding this comment

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

odlist seems rather awkward of a name. something like dynarray or collection seems a little bit more fitting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

collection is too generic and dynarray is cringe.

The rationale here is that odlist prevents namespace pollution and codebases can wrap it in a macro if they want to rename it.

Choose a reason for hiding this comment

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

Couldnt think of a better name; but that seems good albeit smidge weird.

Copy link

Choose a reason for hiding this comment

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

vec


foo += "d" // Valid modification, just like a BYOND list

foo["c"] = 5 // Compile-time error, ODLists cannot be associative

world.log << foo.Join() // Prints "abcd"
```

### Queue
Similar to lists, except elements are first-in first-out. Most of the list API will remain, except for inserting and removing elements. These will use `queue.enqueue(element)` and `queue.dequeue()`.

Example usage:
```
var/queue/foo = queue("a","b","c")

foo += "d" // Compile-time error, must use foo.enqueue("d")

world.log << foo[2] // Prints "b", indexing is supported

world.log << foo.dequeue() // Removes "a" from `foo` and prints "a"
```

A `queue.to_array()` helper will also be provided, and returns the queue's elements as a shallow-copied `array()`.

Choose a reason for hiding this comment

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

Would this be implicitly done if you do

for (var/thing in foo)

or would you be forced to do

for (var/thing in foo.to_array())

Choose a reason for hiding this comment

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

I like it being a method on queue, although it could potentially be more 'DMy' if it was something like array(queue) since all the current conversion procs in DM are global procs that take the thing to convert as a parameter. I think this shoulsd have some discussion around it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the method on queue as well. Would be nice to have it that way for the other conversion procs as well. A stupid compromise can just be having both but that's yucky.


### Stack
Similar to queues, except elements are last-in first-out. Most of the list API will remain, except for inserting and removing elements. These will use `stack.push(element)` and `stack.pop()`.

Example usage:
```
var/stack/foo = stack("a","b","c")

foo += "d" // Compile-time error, must use foo.push("d")

world.log << foo[2] // Prints "b", indexing is supported

foo.push("d")
world.log << foo.pop() // Removes "d" from `foo` and prints "d"
```

A `stack.to_array()` helper will also be provided, and returns the stack's elements as a shallow-copied `array()`.

## Constant Collections

All OpenDream collections can now be declared as constant, which prevents any future modifications to the collection contents and will enable future internal optimizations.
ike709 marked this conversation as resolved.
Show resolved Hide resolved

Example usage:
```
var/const/array/foo = array("a","b","c")
foo[1] = "z" // Compile-time error, cannot modify constant collection
Comment on lines +149 to +150
Copy link

@PowerfulBacon PowerfulBacon May 6, 2024

Choose a reason for hiding this comment

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

I think this needs some explanation of how it is going to work with relation to casting.
My assumption is:

  • Any non-constant array can be cast into a constant array (or provided as a parameter to a function that takes constant arrays)
  • Any constant array cannot be cast into a non-constant array

If the above was the case, then how would the following work?

var/array/test = array("a", "b")
var/const/array/constant_test = test
test[1] = "c"
world.log << constant_test[1]

Would constant arrays have to be compile time constant (which would seriously limit their use imo), would they allocate new memory on assign (which could be very inefficient), would they accept that they can change if the underlying array they are based on was changed, or would they do something else thats more complex (something more advanced like trying to figure out at compile time if something is being modified later down the line, or allocating new memory if and only if the array that it was based on has the opportunity to be modified)?

```

Note that `var/const/list/L` is not valid in BYOND and usages of constant collections will require a wrapper macro.

## Collection Capacity API

Collections (other than the existing BYOND `list()` and the fixed-length `array()` types) will expose the C# `Capacity` property and `EnsureCapacity()` functionality. These operate similarly to `list.len`, except capacity refers to the number of elements the collection's internal data structure can hold without allocating more memory. Ergo a list could have 3 elements and a capacity of 5, and no memory allocation will occur until a 6th element is inserted.

Accessing `collection.capacity` (and similar for other collection types) will return the currect value of the C# `Capacity` property. Setting it will set it in C#, which may runtime if the new capacity is smaller than the current length of the collection.

Calling `collection.ensure_capacity(size)` will call the C# method `EnsureCapacity(size)`. This method ensures that the capacity of this collection is at least the specified capacity. If the current capacity is less than capacity, it is increased to at least the specified capacity.

## BYOND List Deprecation Pragma

A new `DeprecatedByondList` pragma will be implemented. This emits on any usage of BYOND `list()` collections, and will be disabled by default. This pragma exists to assist with guiding migration to OpenDream collections.

## Considerations & Drawbacks

The primary drawback is that these collections types provide no performance benefit to codebases that do not fully migrate to OpenDream, and maintaining BYOND compability relies heavily on `#ifdef OPENDREAM` wrapper macros. That being said, there is still the opportunity for OpenDream to catch misuse of these collections as part of CI workflows.