Runtime: Proposal: Specialize IA.Builder.ToImmutableArray, IHS.Builder.ToImmutableHashSet

[jamesqo]

A while ago I was working on a project and whenever I wanted an ImmutableArray I would just call ToImmutableArray() on the enumerable. I got so used to doing this, in fact, that I did it for ImmutableArray.Builders without realizing there was a ToImmutable() method for a long time.

Update 8/13/17

Instead of making the new facades instance methods, they will be extension methods. Rationale is given in this comment.

public static class ImmutableArray
{
    public static ImmutableArray<T> ToImmutableArray<T>(this ImmutableArray<T>.Builder source);
}

public static class ImmutableDictionary
{
    public static ImmutableDictionary<TKey, TValue> ToImmutableDictionary<TKey, TValue>(this ImmutableDictionary<TKey, TValue>.Builder source);
}

public static class ImmutableHashSet
{
    public static ImmutableHashSet<T> ToImmutableHashSet<T>(this ImmutableHashSet<T>.Builder source);
}

public static class ImmutableList
{
    public static ImmutableList<T> ToImmutableList<T>(this ImmutableList<T>.Builder source);
}

public static class ImmutableSortedDictionary
{
    public static ImmutableSortedDictionary<TKey, TValue> ToImmutableSortedDictionary<TKey, TValue>(this ImmutableSortedDictionary<TKey, TValue>.Builder source);
}

public static class ImmutableSortedSet
{
    public static ImmutableSortedSet<T> ToImmutableSortedSet<T>(this ImmutableSortedSet<T>.Builder source);
}

Update 6/16/17

We can add IA<T>.Builder.ToIA(), IHS<T>.Builder.ToIHS(), etc. instance methods that are simply wrappers for ToImmutable(), preventing people who make silly mistakes like me from degrading the performance of their app.

public struct ImmutableArray<T>
{
    public class Builder { public ImmutableArray<T> ToImmutableArray(); }
}

public class ImmutableHashSet<T>
{
    public class Builder { public ImmutableHashSet<T> ToImmutableHashSet(); }
}

// ...

Original content

It might be a good idea to add ToImmutableArray() to ImmutableArray.Builder and mark it [Obsolete], for the sake of enforcing consistency. Similar for all of the other builder types. The [Obsolete] should not have error: true because we don't want to break existing code, but it will advise the user to use ToImmutable().

public struct ImmutableArray<T>
{
    public class Builder { [EditorBrowsable(Never)] [Obsolete("Use ToImmutable() instead.")] public ImmutableArray<T> ToImmutableArray(); }
}

public class ImmutableHashSet<T>
{
    public class Builder { [EditorBrowsable(Never)] [Obsolete("Use ToImmutable() instead.")] public ImmutableHashSet<T> ToImmutableHashSet(); }
}

// ...

/cc @AArnott, what do you think?

[AArnott]

I agree calling ToImmutable() is preferable in these cases. But I don't recall just how impactful it is. We could do type checks in the ToImmutableArray() extension method to make it basically equivalent (if we don't already, I don't recall).
Another option is to define some analyzers that catch this and other common mistakes that only analyzers can catch (like calling ImmutableList.Add but doing nothing with the result as if it were a mutable collection).

To your suggestion though, does defining an instance method and hiding it effectively hide the extension method from completion lists in the editor?

[jamesqo]

But I don't recall just how impactful it is. We could do type checks in the ToImmutableArray() extension method to make it basically equivalent

It seems silly to have to optimize for this when the usage is so easy to fix. And no, it's not equivalent to ToImmutable() yet-- we check for ImmutableArray<T>, then IImmutableArray, then we go down this branch and check for some well-known types like List<T>, T[], and ImmutableArray<T> again here. None of those match, so we end up foreach-ing through the builder.

Another option is to define some analyzers that catch this and other common mistakes that only analyzers can catch

True, although impact would be limited to people who use analyzers.

To your suggestion though, does defining an instance method and hiding it effectively hide the extension method from completion lists in the editor?

I just tested. Unfortunately, no (maybe because the extension method is generic, while the instance one in itself is not). However, the instance method is still called, so a warning would be raised.

[AArnott]

I just tested. Unfortunately, no (maybe because the extension method is generic, while the instance one in itself is not). However, the instance method is still called, so a warning would be raised.

If you tested it where the collection library was in your solution, you should be aware that the [EditorBrowsable(Never)] attribute doesn't hide items from completion lists when the API is reachable within a project or project reference. Only assembly references seem to honor that attribute.

Why emit a warning though? If we're going to define the method, why not just have it call ToImmutable() and call it a day? Sounds like it's all the goodness we want for the user, but without hassling them about changing the method they call.

[jamesqo]

@AArnott

Why emit a warning though? If we're going to define the method, why not just have it call ToImmutable() and call it a day? Sounds like it's all the goodness we want for the user, but without hassling them about changing the method they call.

It's for the purpose of consistency-- it's confusing when the same thing is done in multiple ways.

If you tested it where the collection library was in your solution, you should be aware that the [EditorBrowsable(Never)] attribute doesn't hide items from completion lists when the API is reachable within a project or project reference. Only assembly references seem to honor that attribute.

I actually found out about that while I was testing it. Yes, I tested it outside the solution.

[jnm2]

It's for the purpose of consistency-- it's confusing when the same thing is done in multiple ways.

Yes.

[AArnott]

But we have instance methods that match extension methods names all over the place for purposes of specialization. ImmutableArray<T> is full of them. I would think ToImmutableArray() is just another one of those examples.

[karelz]

Obsolete is too big hammer in general - I would suggest to use this https://github.com/dotnet/corefx/wiki/ApiCompat instead.

[jamesqo]

@AArnott Although I would prefer being consistent, I don't have a very strong opinion on this so I'm willing to go with your solution here. Plus as @karelz says (in the wiki) this will cause warnings for existing codebases, which are often treated as errors and can induce breaking changes.

@karelz Just a question-- do you really think it's a good idea to let anyone edit the wiki? This repo has 12k stars so it's visited by quite a lot of people. It isn't too hard to imagine some troll just stopping by and deleting everything/posting random content, which could go unnoticed for some time.

[karelz]

@jamesqo yes, I think it is a good idea. Trolls can be kicked out of GitHub / our org. And there is history, so we can review changes :) ... I prefer easier collaboration over fear from not-yet-observed malicious behavior.

[AArnott]

@jamesqo: yes IMO just defining an instance method that has the desired behavior seems like the way to go.

[jamesqo]

@AArnott OK. Have updated the proposal

[safern]

@ianhays and me are okay with the updated propsal, so marking it as api-ready-for-review.

[jamesqo]

@safern That's great. One thought I had since writing this, when you write

public static class CExtensions { public static void M<T>(this C<T>c) { } }

public class C<T> { public void M() { } }

new C().// cursor is here

Both M() and M<>() will show up in the editor completion list. Also, changing extension method -> instance method calls could be breaking since they might throw an NRE instead of an ANE. So I'm changing the proposal so that the new APIs are extension methods:

public static class IA
{
    public static IA<T> ToIA<T>(this IA<T>.Builder source);
}

public static class IHS
{
    public static IHS<T> ToIHS<T>(this IHS<T>.Builder source);
}

...

[safern]

Sounds reasonable to me. Could you please update the original proposal?

[AArnott]

But the existing ToImmutableArray method is an extension method. Will defining one for a more derived type take precedence in c#? In other languages?

[jamesqo]

@AArnott Sorry for the late response.

But the existing ToImmutableArray method is an extension method. Will defining one for a more derived type take precedence in c#? In other languages?

Of course it will. System.Collections.Immutable already does this trick where it defines overloads of LINQ methods specific to ImmutableArray, and those are picked up on instead of the ones in Enumerable, correct?

[jamesqo]

@safern I updated the original proposal-- this can be re-marked as ready-for-review.

[karelz]

@jamesqo can you please expend the names to all added methods? It will be easier to understand the proposed API surface (I couldn't make heads and tales of it in 10s). Thanks!

[jamesqo]

@karelz I was a bit lazy; the type names are so long, and at the time I was editing on my phone. I've updated the proposal now that I'm on my laptop.

[safern]

Thanks @jamesqo just labeled it as api-ready-for-review :)

[karelz]

API approved as described in top post.
For future, please put there full formal proposal - we had to fish out motivation and reasons from the thread :)

[karelz]

Should be fairly straightforward (adding new public API is the most complex part). Anyone wants to pick it up?

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/W_r6oG7nnK4?t=99 (10 min duration)

[danmosemsft]

@jamesqo any interest in doing this one?

[jamesqo]

@danmosemsft Can't right now, mark it up-for-grabs please.

[davidkaya]

@danmosemsft I will take this one.

[danmosemsft]

@davidkaya you are welcome. Let us know if you have questions. There are plenty of past PRs that add API and help in our Documentation also.

[AArnott]

Adding extension methods to solve this problem will be ineffective for folks have using System.Linq; in their using list but don't have using System.Collections.Immutable; in their using list. That's why I'd favor instance methods over more extension methods.

Both M() and M<>() will show up in the editor completion list.

Is this still a problem in VS2017? If so, where is the bug on the language service to track this?
Given it would look odd for completion lists to display both ToImmutable and ToImmutableArray for a builder, how about we use [System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)] on the new instance methods? This would seem to prevent the double-display at least, and possibly hide ToImmutableArray entirely. But if it still displays the extension method, it would end up calling the instance method, which is what we want.

Also, changing extension method -> instance method calls could be breaking since they might throw an NRE instead of an ANE. So I'm changing the proposal so that the new APIs are extension methods:

Either exception should be considered a genuine bug in the program. And it's not a binary breaking change either. It would only hit folks who recompile their programs against the new API. I wouldn't tend to lean toward maintaining source compatibility for folks who are intentionally doing something that might throw ANE and catch it later as part of their execution flow.