Suggest list<TYPE> over array<TYPE> and TYPE[]

[provokateurin]

The array type can be dangerous in some situations where you definitely only want a list.

[provokateurin]

@nickvergessen do you think we should also suggest list<...> over ...[] for readability purposes?

[nickvergessen]

for readability purposes?

Making people change their working production code is not the best idea.

Also just for the record because the title is irritating. Not every array is a list. But every list is an array. In a lot of cases docs are basically lying and claim …[] while they did array_filter etc, and the array is actually not a list anymore but can have gaps in the keys. I guess Psalm could/should cover that, but seems it does not yet?

[provokateurin]

If there are a gaps in the keys then the array should be specified as array<int, ...> explicitly.

[provokateurin]

Making people change their working production code is not the best idea.

I'd make it a warning for now, but I think at some point this should be enforced to prevent bugs.

[nickvergessen]

If there are a gaps in the keys then the array should be specified as array<int, ...> explicitly.

Yes, but that is not what most apps have at the moment. And if they would just add list<…> and update their baseline it#s still broken and not what is documented.

[come-nc]

Syntax type[] means array, not list, at least for psalm.

[provokateurin]

But array<type> should be the same as list<type>, no? Both ensure the array is not associative and the index it continuous.

[come-nc]

No, array<string> means array<array-key,string> to my knowledge.

Reading https://psalm.dev/docs/annotating_code/type_syntax/array_types/ at least it’s clear type[] means array<array-key,type> to psalm.

[provokateurin]

After reading that I feel like we might want to discourage type[] as one usually wants list<type> but the [] syntax also allows string indexes (which seems counterintuitive to me and openapi-extractor treats it like an array in JSON and never as a possible object).

[come-nc]

After reading that I feel like we might want to discourage type[] as one usually wants list<type> but the [] syntax also allows string indexes (which seems counterintuitive to me and openapi-extractor treats it like an array in JSON and never as a possible object).

I’m hesitant.
In general it is good practice to return list<type> but accept type[] as entry data to be more flexible.
But using list everywhere means useless calls to array_values as soon as we touch the array. While type[] basically means I return something you can iterate on, just ignore the keys.

So yeah I don’t know. For ocs API yes it makes sense to encourage list I suppose, and maybe refuse type[] to force explicit description of what is returned.
But the current type limitation already in place are driving me crazy, I want to accept any level of nested array with int|string keys and string values, and it’s proving challenging.

[provokateurin]

means I return something you can iterate on, just ignore the keys

The problem I see is that this could result in different JSON outputs. It could either be an array or an object, but currently we just assume it is always an array. Now this doesn't seem to be true so it should be fixed, but that would get super ugly and I've never encountered a problem with it so far.

But the current type limitation already in place are driving me crazy, I want to accept any level of nested array with int|string keys and string values, and it’s proving challenging.

So basically a recursive object? Are you a facing a limitation of openapi-extractor or psalm? To my knowledge psalm doesn't allow any self-referencing/recursive types, so I don't know how to achieve what you describe. Please let me know if I can help you.

[come-nc]

The problem I see is that this could result in different JSON outputs. It could either be an array or an object, but currently we just assume it is always an array. Now this doesn't seem to be true so it should be fixed, but that would get super ugly and I've never encountered a problem with it so far.

Yeah I meant for PHP usage, not for the JSON API. For the API I think it makes sense to use and enforce list.

So basically a recursive object? Are you a facing a limitation of openapi-extractor or psalm? To my knowledge psalm doesn't allow any self-referencing/recursive types, so I don't know how to achieve what you describe. Please let me know if I can help you.

Yes. I think it is a psalm limitation.
Is it possible to describe this in openapi schema?

[provokateurin]

Yeah OpenAPI/JSON schema can describe recursive types, but there is no way we can generate them due to the psalm limitation.

[provokateurin]

We have to forbid anything that is equivalent to array<array-key, TYPE> because array-key can be string or int while only the former is actually valid JSON (also see #142).
This means the TYPE[] syntax needs to be forbidden as well as the array<TYPE> syntax as they both expand to array<array-key, TYPE> which can be invalid JSON.
This would also prevent a lot of the problem when it comes to handling PHP arrays and converting them to JSON (e.g. nextcloud/server#48311).
The alternative to these is using list<TYPE> or array<string, TYPE> when necessary, nothing else is allowed.

@come-nc @nickvergessen does this make sense to you?

I fear this will have quite some impact and necessary changes (so it should be a warning at first), but it is necessary to fix these problems to ensure the specifications match any data that could possibly returned.

[nickvergessen]

This means the TYPE[] syntax needs to be forbidden as well as the array syntax as they both expand to array<array-key, TYPE> which can be invalid JSON.

We need to properly communicate this. Most people that use TYPE[] actually mean array<int, TYPE>, but yeah there is also issues with various server functions that claim to return [] but its actually an array and the index keys can have gaps.

I fear this will have quite some impact and necessary changes

necessary doc changes, most functionality should be "fine"

[provokateurin]

Functionality yes, but I already started adapting the server to these changes and it causes a lot of changes deeper in the code where the typing is already wrong.
So I hope to make some slow progress on this on server and I'll make a draft PR for everyone else to already try it out and see what needs to be changed to prepare for the update.

[nickvergessen]

we could have a commit/branch in this repo, then people can checkout the commit locally and run it

[provokateurin]

Yeah that's what I meant with "make a draft PR". I wasn't specific enough on which repo this would be.

[provokateurin]

See #168