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

Add IReadOnlySet<T> interface #32488

Merged
merged 13 commits into from Mar 4, 2020
Merged

Add IReadOnlySet<T> interface #32488

merged 13 commits into from Mar 4, 2020

Conversation

@Jlalond
Copy link
Collaborator

Jlalond commented Feb 18, 2020

#2293

@stephentoub stephentoub changed the title Readonly set Add IReadOnlySet<T> interface Feb 18, 2020
@Jlalond Jlalond force-pushed the Jlalond:readonly-set branch 2 times, most recently from 0e5ce15 to 91bfff5 Feb 18, 2020
@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Feb 18, 2020

@stephentoub Sorry about the naming/rebasing, I dropped when I should've squashed and got myself into trouble. The dangers of not drinking coffee early in the morning

@Jlalond Jlalond force-pushed the Jlalond:readonly-set branch 2 times, most recently from 513ff7c to c722bdb Feb 20, 2020
@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Feb 20, 2020

@danmosemsft / @jkotas It's not mentioned in the issue, but should we have IImmutableSet<T> inherit from IReadOnlySet<T> instead of having Immutable hashset/sortedset inherit from IReadOnlySet individually?

@maryamariyan

This comment has been minimized.

Copy link
Member

maryamariyan commented Feb 24, 2020

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Feb 25, 2020

@maryamariyan So I should add to the dev remarks? Even if I haven't actually added a method to the ref/impl

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Feb 25, 2020

@eiriktsarpalis @layomia You are the area-System.Collections owners according to https://github.com/dotnet/runtime/blob/master/docs/area-owners.md. Could you please provide guidance to @Jlalond with this PR?

@jkotas jkotas requested review from eiriktsarpalis and layomia Feb 25, 2020
@carlossanlop

This comment has been minimized.

Copy link
Member

carlossanlop commented Feb 25, 2020

So I should add to the dev remarks? Even if I haven't actually added a method to the ref/impl

@Jlalond You are adding the interface only in the src file. Shouldn't new interfaces be added to ref files as well so they can be exposed publicly?

Yes, please add triple slash comments on top of the interface's type and methods.

We have new official guidance, and not having documentation for new APIs is considered a PR merge blocker.

@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Feb 25, 2020

@carlossanlop I understand that I needed to add this to ref and impl and I did that for hashset and sortedset, as well as their respective ref/impl in Immutable. Is there a ref for private core lib? Because if so, it all makes sense to me now as to why I'm struggling to get this to build. I will add the dev comments for certain, but I wanted to clear up where I'm not implementing in a ref vs an impl

@carlossanlop

This comment has been minimized.

Copy link
Member

carlossanlop commented Feb 25, 2020

Is there a ref for private core lib? Because if so, it all makes sense to me now as to why I'm struggling to get this to build.

@jkotas @eiriktsarpalis @layomia do you know in which ref file this interface should be declared?

@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Feb 25, 2020

@carlossanlop I think I found it in System.Runtime, sorry it took me so long to figure that out :(

@carlossanlop

This comment has been minimized.

Copy link
Member

carlossanlop commented Feb 25, 2020

No worries, @Jlalond 😸 Glad you found it.

@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Feb 25, 2020

@carlossanlop macosx runtime seemed to fail, are there any flaky tests/can you bump the build one more time?

Looks it failed twice for crypto

/// Determines whether the current set overlaps with the specified collection.
/// </summary>
/// <param name="other">The collection to compare to the current set.</param>
/// <returns>ture if the currnet set and other share at least one common eleemnt; otherwise, false.</returns>

This comment has been minimized.

Copy link
@eiriktsarpalis

eiriktsarpalis Feb 26, 2020

Member
Suggested change
/// <returns>ture if the currnet set and other share at least one common eleemnt; otherwise, false.</returns>
/// <returns><see langword="true" />if the current set and other share at least one common element; otherwise, <see langword="false" />.</returns>

This comment has been minimized.

Copy link
@carlossanlop

carlossanlop Feb 26, 2020

Member

@eiriktsarpalis I made a tiny edit to your suggestion.

This comment has been minimized.

Copy link
@Jlalond

Jlalond Feb 26, 2020

Author Collaborator

@carlossanlop should I apply these langwords to all instances of true/false while I'm at it?

This comment has been minimized.

Copy link
@carlossanlop

carlossanlop Feb 27, 2020

Member

Yes sure, that would be great, thank you. true, false and also null.

@eiriktsarpalis

This comment has been minimized.

Copy link
Member

eiriktsarpalis commented Feb 26, 2020

@danmosemsft / @jkotas It's not mentioned in the issue, but should we have IImmutableSet inherit from IReadOnlySet instead of having Immutable hashset/sortedset inherit from IReadOnlySet individually?

I think it would be a reasonable and useful addition, but it probably needs to go through api review, since it was not scoped in the original proposal. What say you @terrajobst?

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Feb 26, 2020

should we have IImmutableSet inherit from IReadOnlySet

I think it would be a reasonable and useful addition

Wouldn't that be a breaking change?

@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Feb 26, 2020

@stephentoub Can you explain why you think it's a breaking change? Is it because the old call sites would now be defined by an interface?

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Feb 27, 2020

e.g. as a source breaking change example, imagine you have this:

interface A { void Bar(); }
    
public class C : A
{
    void A.Bar() { }
}

Now I come along and add:

interface B { void Bar(); }

and change A to inherit B. C will no longer compile.

@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Feb 27, 2020

@stephentoub Makes absolute sense, as which method to call, I only ask because IImmutableSet only inherits from IReadOnlyCollection. I'm absolutely no expert on this, just wanting to learn.

To clarify what I mean, In your above example, only C is defining Bar() and now is inheriting the declaration of same function from B, is that still a source breaking change?

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Feb 27, 2020

To clarify what I mean, In your above example, only C is defining Bar() and now is inheriting the declaration of same function from B, is that still a source breaking change?

Sorry, I don't understand the question. Can you write out the code for what you're asking?

@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Feb 27, 2020

@stephentoub
IImmutableSet<T> already exposes contain, and ImmutableHashSet<T> implements both IImmutableSet<T> and ISet<T> which both share contain. This change has ImmutableHashSet<T> inheriting functions from IReadOnlySet<T> which overlaps with ISet<T>. Is having IImmutableSet<T> forwarding the behavior from IReadOnlySet<T> which IImmutableSet<T> is already implementing source breaking?

From:

interface IImmutableSet<T>
{
  bool Contains(T item);
}

To:

interface IImmutableSet<T> : IReadOnlySet<T>
{
   // contains is now defined in read only set
}
@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Feb 28, 2020

@stephentoub Awesome, thank you for explaining. So we're not adding to IImmutableSet? If so I'll get all the edits done

@terrajobst

This comment has been minimized.

Copy link
Member

terrajobst commented Feb 28, 2020

@eiriktsarpalis

I think it would be a reasonable and useful addition, but it probably needs to go through api review, since it was not scoped in the original proposal. What say you @terrajobst?

It's listed on the API review feedback already. It's unfortunate that the other immutable interfaces all extend their read-only counterparts because we can't do that for the new IReadOnlySet<T> because it's a breaking change (same reason why the mutable interfaces weren't updated to extend the read-only ones).

I think we should still implement IReadOnlySet<T> on ImmutableHashSet<T> and ImmutableSortedSet<T> though.

@Jlalond Jlalond force-pushed the Jlalond:readonly-set branch from e67d3dd to db9067e Mar 1, 2020
@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Mar 3, 2020

@eiriktsarpalis @layomia Hey guys, rebased on top of master and looks like it's building. If I could get another review I'd appreciate it!

Copy link
Member

eiriktsarpalis left a comment

LGTM, thank you for your contribution.

@Jlalond

This comment has been minimized.

Copy link
Collaborator Author

Jlalond commented Mar 3, 2020

@eiriktsarpalis No thank you! Without help of you, Joe and @carlossanlop I wouldn't have figured I needed a ref in System.Runtime!

@eiriktsarpalis eiriktsarpalis merged commit 530b556 into dotnet:master Mar 4, 2020
86 checks passed
86 checks passed
WIP Ready for review
Details
dotnet-runtime-perf Build #20200301.14 succeeded
Details
dotnet-runtime-perf (Checkout) Checkout succeeded
Details
dotnet-runtime-perf (CoreCLR Product Build Linux x64 release) CoreCLR Product Build Linux x64 release succeeded
Details
dotnet-runtime-perf (CoreCLR Product Build Windows_NT x64 release) CoreCLR Product Build Windows_NT x64 release succeeded
Details
dotnet-runtime-perf (CoreCLR Product Build Windows_NT x86 release) CoreCLR Product Build Windows_NT x86 release succeeded
Details
dotnet-runtime-perf (Libraries Build Linux x64 Release) Libraries Build Linux x64 Release succeeded
Details
dotnet-runtime-perf (Libraries Build Windows_NT x64 Release) Libraries Build Windows_NT x64 Release succeeded
Details
dotnet-runtime-perf (Libraries Build Windows_NT x86 Release) Libraries Build Windows_NT x86 Release succeeded
Details
dotnet-runtime-perf (Performance Linux x64 release netcoreapp5.0) Performance Linux x64 release netcoreapp5.0 succeeded
Details
dotnet-runtime-perf (Performance Windows_NT x64 release netcoreapp5.0) Performance Windows_NT x64 release netcoreapp5.0 succeeded
Details
dotnet-runtime-perf (Performance Windows_NT x86 release netcoreapp5.0) Performance Windows_NT x86 release netcoreapp5.0 succeeded
Details
license/cla All CLA requirements met.
Details
runtime Build #20200301.19 succeeded
Details
runtime (Checkout) Checkout succeeded
Details
runtime (CoreCLR Product Build Linux arm release) CoreCLR Product Build Linux arm release succeeded
Details
runtime (CoreCLR Product Build Linux arm64 release) CoreCLR Product Build Linux arm64 release succeeded
Details
runtime (CoreCLR Product Build Linux x64 release) CoreCLR Product Build Linux x64 release succeeded
Details
runtime (CoreCLR Product Build Linux_musl arm64 release) CoreCLR Product Build Linux_musl arm64 release succeeded
Details
runtime (CoreCLR Product Build Linux_musl x64 release) CoreCLR Product Build Linux_musl x64 release succeeded
Details
runtime (CoreCLR Product Build OSX x64 checked) CoreCLR Product Build OSX x64 checked succeeded
Details
runtime (CoreCLR Product Build OSX x64 release) CoreCLR Product Build OSX x64 release succeeded
Details
runtime (CoreCLR Product Build Windows_NT arm release) CoreCLR Product Build Windows_NT arm release succeeded
Details
runtime (CoreCLR Product Build Windows_NT arm64 release) CoreCLR Product Build Windows_NT arm64 release succeeded
Details
runtime (CoreCLR Product Build Windows_NT x64 release) CoreCLR Product Build Windows_NT x64 release succeeded
Details
runtime (CoreCLR Product Build Windows_NT x86 release) CoreCLR Product Build Windows_NT x86 release succeeded
Details
runtime (Installer Build and Test Linux_arm Debug) Installer Build and Test Linux_arm Debug succeeded
Details
runtime (Installer Build and Test Linux_arm64 Debug) Installer Build and Test Linux_arm64 Debug succeeded
Details
runtime (Installer Build and Test Linux_musl_arm64 Debug) Installer Build and Test Linux_musl_arm64 Debug succeeded
Details
runtime (Installer Build and Test Linux_musl_x64 Debug) Installer Build and Test Linux_musl_x64 Debug succeeded
Details
runtime (Installer Build and Test Linux_x64 Debug) Installer Build and Test Linux_x64 Debug succeeded
Details
runtime (Installer Build and Test OSX_x64 Debug) Installer Build and Test OSX_x64 Debug succeeded
Details
runtime (Installer Build and Test Windows_NT_arm Debug) Installer Build and Test Windows_NT_arm Debug succeeded
Details
runtime (Installer Build and Test Windows_NT_arm64 Debug) Installer Build and Test Windows_NT_arm64 Debug succeeded
Details
runtime (Installer Build and Test Windows_NT_x64 Debug) Installer Build and Test Windows_NT_x64 Debug succeeded
Details
runtime (Installer Build and Test Windows_NT_x86 Debug) Installer Build and Test Windows_NT_x86 Debug succeeded
Details
runtime (Libraries Build Linux arm Release) Libraries Build Linux arm Release succeeded
Details
runtime (Libraries Build Linux arm64 Debug) Libraries Build Linux arm64 Debug succeeded
Details
runtime (Libraries Build Linux x64 Debug) Libraries Build Linux x64 Debug succeeded
Details
runtime (Libraries Build Linux_musl arm64 Release) Libraries Build Linux_musl arm64 Release succeeded
Details
runtime (Libraries Build Linux_musl x64 Debug) Libraries Build Linux_musl x64 Debug succeeded
Details
runtime (Libraries Build OSX x64 Debug) Libraries Build OSX x64 Debug succeeded
Details
runtime (Libraries Build WebAssembly wasm Debug) Libraries Build WebAssembly wasm Debug succeeded
Details
runtime (Libraries Build Windows_NT allConfigurations x64 Debug) Libraries Build Windows_NT allConfigurations x64 Debug succeeded
Details
runtime (Libraries Build Windows_NT arm Release) Libraries Build Windows_NT arm Release succeeded
Details
runtime (Libraries Build Windows_NT arm64 Release) Libraries Build Windows_NT arm64 Release succeeded
Details
runtime (Libraries Build Windows_NT net472 x86 Release) Libraries Build Windows_NT net472 x86 Release succeeded
Details
runtime (Libraries Build Windows_NT x64 Debug) Libraries Build Windows_NT x64 Debug succeeded
Details
runtime (Libraries Build Windows_NT x86 Debug) Libraries Build Windows_NT x86 Debug succeeded
Details
runtime (Libraries Build Windows_NT x86 Release) Libraries Build Windows_NT x86 Release succeeded
Details
runtime (Libraries Test Build Linux x64 Debug) Libraries Test Build Linux x64 Debug succeeded
Details
runtime (Libraries Test Build OSX x64 Debug) Libraries Test Build OSX x64 Debug succeeded
Details
runtime (Libraries Test Build Windows_NT x64 Debug) Libraries Test Build Windows_NT x64 Debug succeeded
Details
runtime (Libraries Test Run checked coreclr OSX x64 Debug) Libraries Test Run checked coreclr OSX x64 Debug succeeded
Details
runtime (Libraries Test Run release coreclr Linux x64 Debug) Libraries Test Run release coreclr Linux x64 Debug succeeded
Details
runtime (Libraries Test Run release coreclr Linux_musl x64 Debug) Libraries Test Run release coreclr Linux_musl x64 Debug succeeded
Details
runtime (Libraries Test Run release coreclr OSX x64 Debug) Libraries Test Run release coreclr OSX x64 Debug succeeded
Details
runtime (Libraries Test Run release coreclr Windows_NT x64 Debug) Libraries Test Run release coreclr Windows_NT x64 Debug succeeded
Details
runtime (Libraries Test Run release coreclr Windows_NT x86 Debug) Libraries Test Run release coreclr Windows_NT x86 Debug succeeded
Details
runtime (Libraries Test Run release coreclr Windows_NT x86 Release) Libraries Test Run release coreclr Windows_NT x86 Release succeeded
Details
runtime (Libraries Test Run release mono Linux arm64 Debug) Libraries Test Run release mono Linux arm64 Debug succeeded
Details
runtime (Libraries Test Run release mono Linux x64 Debug) Libraries Test Run release mono Linux x64 Debug succeeded
Details
runtime (Libraries Test Run release mono OSX x64 Debug) Libraries Test Run release mono OSX x64 Debug succeeded
Details
runtime (Mono LLVM Product Build Linux x64 debug) Mono LLVM Product Build Linux x64 debug succeeded
Details
runtime (Mono LLVM Product Build Linux x64 release) Mono LLVM Product Build Linux x64 release succeeded
Details
runtime (Mono LLVM Product Build OSX x64 debug) Mono LLVM Product Build OSX x64 debug succeeded
Details
runtime (Mono LLVM Product Build OSX x64 release) Mono LLVM Product Build OSX x64 release succeeded
Details
runtime (Mono Product Build Linux arm debug) Mono Product Build Linux arm debug succeeded
Details
runtime (Mono Product Build Linux arm release) Mono Product Build Linux arm release succeeded
Details
runtime (Mono Product Build Linux arm64 debug) Mono Product Build Linux arm64 debug succeeded
Details
runtime (Mono Product Build Linux arm64 release) Mono Product Build Linux arm64 release succeeded
Details
runtime (Mono Product Build Linux x64 debug) Mono Product Build Linux x64 debug succeeded
Details
runtime (Mono Product Build Linux x64 release) Mono Product Build Linux x64 release succeeded
Details
runtime (Mono Product Build Linux_musl x64 debug) Mono Product Build Linux_musl x64 debug succeeded
Details
runtime (Mono Product Build Linux_musl x64 release) Mono Product Build Linux_musl x64 release succeeded
Details
runtime (Mono Product Build OSX x64 debug) Mono Product Build OSX x64 debug succeeded
Details
runtime (Mono Product Build OSX x64 release) Mono Product Build OSX x64 release succeeded
Details
runtime (Mono Product Build Windows_NT x64 debug) Mono Product Build Windows_NT x64 debug succeeded
Details
runtime (Mono Product Build Windows_NT x64 release) Mono Product Build Windows_NT x64 release succeeded
Details
runtime-live-build Build #20200301.16 succeeded
Details
runtime-live-build (Build Linux x64 debug RuntimeFlavor_Mono) Build Linux x64 debug RuntimeFlavor_Mono succeeded
Details
runtime-live-build (Build Linux x64 debug Runtime_Release) Build Linux x64 debug Runtime_Release succeeded
Details
runtime-live-build (Build OSX x64 release Runtime_Debug) Build OSX x64 release Runtime_Debug succeeded
Details
runtime-live-build (Build Windows_NT x64 debug Mono_Libraries) Build Windows_NT x64 debug Mono_Libraries succeeded
Details
runtime-live-build (Build Windows_NT x86 release Runtime_Debug) Build Windows_NT x86 release Runtime_Debug succeeded
Details
runtime-live-build (Checkout) Checkout succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.