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

Do something with IReadOnlyCollection and ICollection. We needs good abstractions hierarchy. #20399

Closed
dmitriyse opened this issue Mar 2, 2017 · 8 comments
Labels
area-System.Collections enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@dmitriyse
Copy link

Just drop IReadOnly****, mark them obsolete.
We can't do anything with backward compatibility here.
If we inherit ICollection from IReadOnlyCollection we will broke existing code.
See: http://stackoverflow.com/questions/12622539/why-doesnt-generic-icollection-implement-ireadonlycollection-in-net-4-5

But may be sometime will be good point do do this breaking change, provide tools for migration automation.

And also IReadOnly*** is not clear solution, we needs IReadable*** and IImmutable****

This is not-null reference types like problem. But it's second after nullability.
Let's start/continue to solve this problem.

@dmitriyse
Copy link
Author

dmitriyse commented Mar 2, 2017

Too late. Too late. It means never. But it's every day pain. We need to do something like not-nullable reference (see dotnet/csharplang#219) or define version of CoreFX with this breaking change. For example "DotNet Ultra" scheduled to 2025 - will be next major Dotnet rewrite, where this problem will be solved.

@danmoseley
Copy link
Member

As you recognize I don't believe we'd take any breaking change of that sort any time soon. Countless code already uses them.

@terrajobst
Copy link
Member

terrajobst commented Mar 2, 2017

Removing is certainly not an option at this point. However, we've talked with the compiler and runtime team to see if we could make the mutable interfaces inherit from the read-only interfaces. AFAIR the consensus is that we probably could, but we'll have to make sure it's not a source/IL breaking change. So far, the impact of the work didn't seem to be worth it.

@dmitriyse assuming our mutable interfaces would extend the read-only interfaces, would this be sufficient? Also note that we do have immutable interfaces in the System.Collections.Immutable namespace.

@dmitriyse
Copy link
Author

dmitriyse commented Mar 3, 2017

Ok, I understand how deep is a rabbit hole. Hypothetical "DotNet Ultra" in 2025 will be probably backward compatible to current CLR binaries. And probably we will get D# that works side by side with C# 12.0 on-top of the same CLR, but have no any inherited problems of C#(and have no old-code compile restriction). This hypothetical D# can finally get more popularity than C# and finally any current C# problems could be solved.

But next 7 years until 2025 we needs to work on C#.

But seriously:

If we will think carefully we will infer that:
Adding inheritance ICollection: IReadOnlyCollection will broke

  1. Current Binary Code (BCL) and libraries,
  2. Current CLR implementation, but not IL itself.
  3. Binary compatibility of assemblies between current and modified CLR
  4. Current sources that relays on Reflection enumeration (Not so much actually)
    For example any core that rely on self-declared properties list of ICollection will be broken since "Count" property will be only in IReadOnlyCollection.
  5. Current sources with explicit implementations (billions of code lines). Except the case where C# language team relax restriction on explicit interface implementation and allow explicit implementation of inherited members with name of derived interface:
public class MyCollection:ICollection
{
    // It's in IReadOnlyCollection, but it's ok in C# 6.0+ 
    int ICollection<int>.Count { 
        get
        { 
            return _someCount;
         }
    }
}

and finally this point requires not so big change in the compiler.


So what was broken with CoreCLR

  1. Current Binary Code (we was required to recompile all libraries, even if no source changes were required)
  2. CLR is fully reimplemented but IL mostly stay as is.
  3. Binary compatibility of assemblies between classic and new CLR
  4. Source code required adoption in a case of reflection
  5. Compiler was improved greatly to work not only on windows.

Conclusion

Breaking changes of ICollection: IReadOnlyCollections are very the same as .Net 4.5 / CoreCLR breaking changes.

The only additional required step was a compiler feature with relaxation:

public class MyCollection:ICollection
{
    // It's in IReadOnlyCollection, but it's ok in C# 6.0+ 
    int ICollection<int>.Count { 
        get
        { 
            return _someCount;
         }
    }
}

@dmitriyse
Copy link
Author

dmitriyse commented Mar 3, 2017

Unfortunately looks like with CoreCLR is too late. But I hope at least this can be planned to 2025 year.

Currentl we can only improve toolset/compiler to allow annotation of classes/argumets/properties/variables with "readable" and add compile-time analysis.

Exactly the same thing that will be done in C# 8.0 with not-null references.

They will add two mode to C# compiler,

  1. for old code written in C# < 8.0 (assign nulls to reference types not marked as not nullable is ok)
  2. for new code written in C# >= 8.0 (Not-Nullable annotation + warnings/errors from static code analysis)

P.S Toolset VS 2015 + R# + ImplicitNullability + JetBrains.Annotations.NotNull already works like C# 8.0.

@dmitriyse
Copy link
Author

dmitriyse commented Mar 3, 2017

@dmitriyse assuming our mutable interfaces would extend the read-only interfaces, would this be sufficient? Also note that we do have immutable interfaces in the System.Collections.Immutable namespace

  1. It's will be really great to force mutable interfaces to inherit "ReadOnly" interfaces. It solve many many problems. If the next two issues would be solved, than yes this is enough.

  2. Immutable interfaces are inherited from "ReadOnly"=="Readable" interfaces this is good but they require implementers to implement cloning-like modification which can be in many cases redundant.
    So we need interfaces for readonly+immutable but not clone-modifiable.
    Or we can do so with annotations for "ReadOnly" variables and static analysis.

  3. At least one interface is missing IReadOnlySet https://github.com/dotnet/corefx/issues/1973
    (Probably some another interfaces should be introduced probably inspired from C5 collections)

@dmitriyse
Copy link
Author

I added more close to reality proposals related to this thread. Possibly annotation and compile-time analysis is a wrong way.
https://github.com/dotnet/corefx/issues/16660
https://github.com/dotnet/corefx/issues/16661

@safern
Copy link
Member

safern commented Mar 13, 2017

As you recognize I don't believe we'd take any breaking change of that sort any time soon. Countless code already uses them.

Removing is certainly not an option at this point.

As stated this is a breaking change we wouldn't like to take at this point or soon. So we are closing it for now.

Thanks @dmitriyse for your proposal and feel free to bring up more :)

@safern safern closed this as completed Mar 13, 2017
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Collections enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

5 participants