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

Extensions for immutable collection builders (#21055) #31071

Merged
merged 6 commits into from Oct 1, 2018

Conversation

@davidkaya
Copy link
Collaborator

commented Jul 14, 2018

Added extensions for immutable builders, which should be prefered over the extensions on IEnumerable because of performance benefits.

Fixes #21055.

davidkaya added 4 commits Jul 14, 2018
Added extensions for immutable builders, which should be prefered over the extensions on IEnumerable because of performance benefits.
@davidkaya davidkaya changed the title Extensions for immutable builders (#21055) Extensions for immutable collection builders (#21055) Jul 14, 2018
@Wraith2

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2018

No null checks?

@karelz karelz added this to the 3.0 milestone Jul 19, 2018
@joshfree joshfree requested review from AArnott and safern Jul 20, 2018
@AArnott
AArnott approved these changes Aug 8, 2018
Copy link
Contributor

left a comment

Thanks for preparing this change. My only concern with this approach is what I just added to the discussion over here.

/// </summary>
/// <param name="builder">The builder to create the immutable array from.</param>
/// <returns>An immutable array.</returns>
[Pure]

This comment has been minimized.

Copy link
@AArnott

AArnott Aug 8, 2018

Contributor

Given that [Pure] has no runtime impact, why isn't the attribute included in the ref assemblies so that analyzers can do their work based on it?

This comment has been minimized.

Copy link
@Wraith2

Wraith2 Aug 8, 2018

Collaborator

Looking at it (and not the author) I'd guess that it's because none of the other definitions in the ref assembly have them and he followed the existing pattern on the assumption that someone in the past had good reason to do this.

Adding them in ref would require a dependency on System.Diagnostics.Contracts which would seem an odd thing to do.

This comment has been minimized.

Copy link
@AArnott

AArnott Aug 8, 2018

Contributor

Agreed: this PR itself looks consistent with precedent. So my question was perhaps more directed at the authors of the ref assemblies in general. @danmosemsft @csharpfritz perhaps?

This comment has been minimized.

Copy link
@csharpfritz

csharpfritz Aug 15, 2018

I think I was included incorrectly on this...

This comment has been minimized.

Copy link
@danmosemsft

danmosemsft Aug 15, 2018

Member

[Pure] is part of Contracts, right? We don't use or enforce those and generally have been removing contract annotations. I believe we discussed this before in the context of immutable collections in fact and @AArnott did you indicate you didn't believe in them either? In which case I guess these can all be removed unless they are somehow still useful to .NET Framework consumers even though they're not on the ref assembly and probably not consistent either.

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 15, 2018

Member

It seems to me we likely want an analyzer proving it adds value first.

Right. Based on the past experiments with Code Contracts, it is very hard to get a return on investment with this. The annotations do catch a few bugs, but they require a prohibitive amount of manual work to do that.

This comment has been minimized.

Copy link
@danmosemsft

danmosemsft Aug 15, 2018

Member

Annotations aside, if the compiler included an analyzer with a hard coded list of most commonly misused pure functions (even just those on String) it would provide non zero value with ?no false positives. 😃

This comment has been minimized.

Copy link
@AArnott

AArnott Aug 16, 2018

Contributor

Based on the past experiments with Code Contracts, it is very hard to get a return on investment with this.

Code Contracts preceded roslyn analyzers, and required custom build steps, VS extensions, and/or post-compilation IL-rewriting . Roslyn analyzers are proven to be effective and popular. Let's not let Code Contracts' failure deter us from tapping Analyzers for solving this simple issue.

This comment has been minimized.

Copy link
@Grauenwolf

Grauenwolf Jan 23, 2019

The Pure attribute is checked by Rule CA1806 in Microsoft.CodeQuality.Analyzers.

This comment has been minimized.

Copy link
@AArnott

AArnott Jan 23, 2019

Contributor

Thanks for redrawing attention to this, @Grauenwolf. I've filed a separate issue to track the issue: #34778

@karelz

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

@safern @ianhays is the PR ready for merge?

@karelz

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

@safern ping?

@safern
safern approved these changes Sep 6, 2018
Copy link
Member

left a comment

LGTM.

@safern

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@davidkaya would you mind fixing the merge conflicts?

@davidkaya

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 28, 2018

@safern Should be fine now.

@safern safern merged commit 9f404b4 into dotnet:master Oct 1, 2018
14 checks passed
14 checks passed
CROSS Check Build finished.
Details
Linux arm Release Build Build finished.
Details
Linux arm64 Release Build Build finished.
Details
Linux x64 Release Build Build finished.
Details
Linux-musl x64 Debug Build Build finished.
Details
NETFX x86 Release Build Build finished.
Details
OSX x64 Debug Build Build finished.
Details
Packaging All Configurations x64 Debug Build Build finished.
Details
UWP CoreCLR x64 Debug Build Build finished.
Details
UWP NETNative x86 Release Build Build finished.
Details
WIP ready for review
Details
Windows x64 Debug Build Build finished.
Details
Windows x86 Release Build Build finished.
Details
license/cla All CLA requirements met.
Details
@safern

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

@safern Should be fine now.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.