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

Casting a scalar (single value) to System.Collections.ArrayList fails #11749

Open
mklement0 opened this issue Feb 1, 2020 · 7 comments
Open

Casting a scalar (single value) to System.Collections.ArrayList fails #11749

mklement0 opened this issue Feb 1, 2020 · 7 comments

Comments

@mklement0
Copy link
Contributor

@mklement0 mklement0 commented Feb 1, 2020

Note: Supporting this would be syntactic sugar, but it is already supported with System.Collections.ArrayList's generic counterpart, System.Collections.Generic.List<T>, as well as with regular arrays (e.g, Object[]) and even other colllections (e.g., System.Collections.ObjectModel.Collection<T>).

Steps to reproduce

# These tests pass and create a single-element collection from the input scalar;
# By  contrast, casting from an *array* (e.g., `[System.Collections.ArrayList] (42, 43)`) 
# works in *all* cases.
{ [array] 42 } | Should -Not -Throw
{ [System.Collections.Generic.List[int]] 42 } | Should -Not -Throw
{ [System.Collections.ObjectModel.Collection[int]] 42 } | Should -Not -Throw

# This test fails.
{ [System.Collections.ArrayList] 42 } | Should -Not -Throw

Expected behavior

All tests should pass.

Actual behavior

The last test fails, because among the types used only System.Collections.ArrayList doesn't support casting from a single value.

Cannot convert the "42" value of type "System.Int32" to type "System.Collections.ArrayList"

Environment data

PowerShell Core 7.0.0-rc.2
@SeeminglyScience

This comment has been minimized.

Copy link
Contributor

@SeeminglyScience SeeminglyScience commented Feb 1, 2020

Since ArrayList is officially not recommended does it make sense to add improvements for it?

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented Feb 1, 2020

I think not worth it

@mklement0

This comment has been minimized.

Copy link
Contributor Author

@mklement0 mklement0 commented Feb 1, 2020

Good to know about the official deprecation notice, @SeeminglyScience.

However, people will likely continue to use it (what they usually care about is efficient iterative growth, not type homogeneity, and [System.Collections.Generic.List[object]] is a keyboardful, even though it is better behaved), and the - situational - inconsistency described here will continue to cause confusion.

Therefore, if it is a reasonably quick and simple fix, my vote is to do it. It is the kind of issue that trips you up in practice, but isn't big enough to warrant documenting.


Looking at the bigger picture, this takes us back to how it would be nice to have a well-behaved list type that is a first-class citizen (#5643), and perhaps even += support for mutable collections (#5805).

@iSazonov iSazonov added the Area-Engine label Feb 1, 2020
@SeeminglyScience

This comment has been minimized.

Copy link
Contributor

@SeeminglyScience SeeminglyScience commented Feb 1, 2020

Good to know about the official deprecation notice, @SeeminglyScience.

Sorry I should have been more explicit, but it is not necessarily deprecated. Just recommended against.

However, people will likely continue to use it (what they usually care about is efficient iterative growth, not type homogeneity, and [System.Collections.Generic.List[object]] is a keyboardful, even though it is better behaved), and the - situational - inconsistency described here will continue to cause confusion.

If it is efficiency they're after, I highly doubt ArrayList is getting the same improvements that go into List<T>. Plus having to null the output of Add has some overhead and imo makes it more cumbersome than List<T>.

Mainly though, if an API is not recommended, and there is a clear alternative, making it easier to use seems counter productive.

Looking at the bigger picture, this takes us back to how it would be nice to have a well-behaved list type that is a first-class citizen (#5643), and perhaps even += support for mutable collections (#5805).

The topic of += being mapped to Add is a mixed bag imo. It would be the only case (I think) of an assignment operator not actually assigning something. Even with PowerShell's custom array addition, you're still assigning the new array. Same with C# and delegates. It's always LHS = LHS + RHS and this would change that. Is that a problem though? 🤷‍♂

Actually now that I think about it C# does it for event handlers, where it calls the add_ accessor. So maybe there's precedent.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

@mklement0 mklement0 commented Feb 1, 2020

Mainly though, if an API is not recommended, and there is a clear alternative, making it easier to use seems counter productive.

Agreed in general, but this is more of a bug fix than an improvement.

To put it differently: Leaving the inconsistency no more encourages users to abandon ArrayList than fixing would promote its (ill-advised) continued use:

[System.Collections.ArrayList] 42 not working is (a) likely to make you work around it with [System.Collections.ArrayList] @(42), and, conversely, (b) does nothing to alert you to the fact that you should be using [System.Collections.Generic.List[object]] 42 instead.

(Not having to worry about .Add() polluting your output stream is a much more compelling reason, by contrast, but the challenge in both cases is that you have to know about the alternative.)

The net effect of the current behavior is just a situational-and-therefore-perfidious inconsistency that is a nuisance in practice, hence my recommendation to fix it.

But enough about that: we now know the pros and cons, and it is clearly not a big issue in the grand scheme of things:

If consensus forms around this assessment and someone is knowledgeable enough to fix this quickly and safely, I'm hoping they will.

I do find the tangent re better built-in list support interesting, so I'll say more about that in a separate comment.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

@mklement0 mklement0 commented Feb 1, 2020

(Tangent-only alert.)

The topic of += being mapped to .Add() is a mixed bag imo. It would be the only case (I think) of an assignment operator not actually assigning something.

I agree that that would be problematic; @PetSerAl originally voiced the concern and proposed implementing a specialized list type that doesn't violate the equivalence of $a += $b and $a = $a + $b while still performing well (though reference equality would not be maintained) - see #5805 (comment).

The C# += syntax for adding event delegates is indeed an interesting anomaly.

Even with PowerShell's custom array addition, you're still assigning the new array.

Yes, but for users not concerned with reference equality that usually isn't a functional problem - but with high iteration counts becomes a performance concern.

Additionally, what's tricky about += currently is that it quietly converts a non-array collection type to an [object[]] array, which adds quiet loss of type fidelity to the performance concern:

$var = [System.Collections.Generic.List[object]] 42
$var += 43 # creates a *copy* of the list *as an array*, with the new element appended.
$var.GetType().Name   # -> 'Object[]' !!

Suggesting that += be mapped to .Add() is seductive (it was the starting point of #5805), but ultimately too problematic.

In light of that, the best that could be done with += for non-array LHS values is to create copies as the LHS type rather than as an array (which wouldn't address the performance concerns and may not always be possible).

For a potential future built-in [list] type, @PetSerAl's proposal strikes me as the best way forward.

I suggest continuing the conversation at #5805.

@bpayette

This comment has been minimized.

Copy link
Contributor

@bpayette bpayette commented Feb 2, 2020

@mklement0 This does sound like a bug. I would have expected the cast to work on anything that implements IList.

Separately, with respect to += the solution might be to use persistent data structures (which are totally not what they sound like.) They are used in extensively in Clojure and are quite performant. With persistent data structures, it should be possible to preserve the semantic while getting a significant performance gain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.