Repository navigation
Suggest list<TYPE> over array<TYPE> and TYPE[] #33
Description
The array type can be dangerous in some situations where you definitely only want a list.
{{ message }}
list<TYPE> over array<TYPE> and TYPE[] #33The array type can be dangerous in some situations where you definitely only want a list.
Activity
provokateurin commentedon Dec 6, 2023
@nickvergessen do you think we should also suggest
list<...>over...[]for readability purposes?nickvergessen commentedon Dec 6, 2023
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 commentedon Dec 6, 2023
If there are a gaps in the keys then the array should be specified as
array<int, ...>explicitly.provokateurin commentedon Dec 6, 2023
I'd make it a warning for now, but I think at some point this should be enforced to prevent bugs.
nickvergessen commentedon Dec 6, 2023
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 commentedon May 30, 2024
Syntax
type[]meansarray, notlist, at least for psalm.provokateurin commentedon May 30, 2024
But
array<type>should be the same aslist<type>, no? Both ensure the array is not associative and the index it continuous.come-nc commentedon Jun 3, 2024
No,
array<string>meansarray<array-key,string>to my knowledge.Reading https://psalm.dev/docs/annotating_code/type_syntax/array_types/ at least it’s clear
type[]meansarray<array-key,type>to psalm.provokateurin commentedon Jun 4, 2024
After reading that I feel like we might want to discourage
type[]as one usually wantslist<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 commentedon Jun 4, 2024
I’m hesitant.
In general it is good practice to return
list<type>but accepttype[]as entry data to be more flexible.But using
listeverywhere means useless calls toarray_valuesas soon as we touch the array. Whiletype[]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
listI suppose, and maybe refusetype[]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 commentedon Jun 4, 2024
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.
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 commentedon Jun 4, 2024
Yeah I meant for PHP usage, not for the JSON API. For the API I think it makes sense to use and enforce
list.Yes. I think it is a psalm limitation.
Is it possible to describe this in openapi schema?
provokateurin commentedon Jun 4, 2024
Yeah OpenAPI/JSON schema can describe recursive types, but there is no way we can generate them due to the psalm limitation.
provokateurin commentedon Sep 24, 2024
We have to forbid anything that is equivalent to
array<array-key, TYPE>becausearray-keycan bestringorintwhile only the former is actually valid JSON (also see #142).This means the
TYPE[]syntax needs to be forbidden as well as thearray<TYPE>syntax as they both expand toarray<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>orarray<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.
[-]Suggest `list<...>` over `array<...>`[/-][+]Suggest `list<TYPE>` over `array<TYPE>` and `TYPE[]`[/+]nickvergessen commentedon Sep 25, 2024
We need to properly communicate this. Most people that use
TYPE[]actually meanarray<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.necessary doc changes, most functionality should be "fine"
provokateurin commentedon Sep 25, 2024
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 commentedon Sep 25, 2024
we could have a commit/branch in this repo, then people can checkout the commit locally and run it
provokateurin commentedon Sep 25, 2024
Yeah that's what I meant with "make a draft PR". I wasn't specific enough on which repo this would be.
provokateurin commentedon Sep 26, 2024
See #168