runtime: Ref assemblies should not drop [Pure] attribute from APIs

The Pure attribute is checked by Rule CA1806 in Microsoft.CodeQuality.Analyzers. The [Pure] attribute helps prevent many bugs, particular around APIs such as the immutable collections where folks frequently make such mistakes as this:

    public static void FOo()
    {
        var list = ImmutableList.Create<int>();
        list.Add(3);
    }

If ImmutableList<T>.Add(T) had the [Pure] attribute on it, the CA1806 analyzer mentioned above would produce an error in the above snippet, thereby helping the developer avoid a bug.

In fact the Add method does have a [Pure] attribute on it:

https://github.com/dotnet/corefx/blob/abc7e38a7a6bb0eed6a972127438685d7e9cbc98/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.cs#L222-L223

But the ref assembly drops these attributes, rendering all the effort we went to in tagging these pure methods to no effect.

In the below example, the call to ImmutableList<int>.Add does not produce a warning, but the next call to the sample’s own Add method does produce a warning, because the [Pure] attribute is actually there.

    public static void FOo()
    {
        var list = ImmutableList.Create<int>();
        list.Add(3);
        Add(3, 5);
    }

    [Pure]
    public static int Add(int a, int b) => a + b;

Can whatever build process that generates these ref assemblies be fixed to preserve this attribute?

_Originally posted by @AArnott in https://github.com/dotnet/corefx/pull/31071_

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 3
Comments: 23 (22 by maintainers)

Most upvoted comments

Can we get that changed? I don’t see any reason to strip away the PureAttribute without warning the developer that they need to add a hidden compiler flag. That is just going to lead to confusion.

Grauenwolf on Jan 23, 2019

that’s a shortcoming of other assemblies, IMO

I’m not going to debate on the value of Contracts here.

Have you confirmed that indeed no other assembly uses this attribute?

Yes. The only other place its used is internally in System.Runtime.WindowsRuntime, so it has no impact on that reference assembly.

is propagating this “fix” as simple as adding the Define to its project?

Yes, and this is actually consistent with what you’d need to do in a customer project if you wanted to ship the usage of the Pure attribute. If a create a new project with “[Pure]” on APIs and compile normally the attribute doesn’t perist in the assembly. If I add a define of CONTRACTS_FULL then it does.

ericstj on Apr 17, 2020

@ericstj If no other assembly has [Pure] attributes in it, that’s a shortcoming of other assemblies, IMO, as there are certainly other APIs that are side-effect free and should be attributed as such. Have you confirmed that indeed no other assembly uses this attribute? If/when we add this attribute in other appropriate places, is propagating this “fix” as simple as adding the Define to its project?

AArnott on Apr 17, 2020

@safern @ericstj do we want to fix this for 3.0? sounds not irrelevant.

ViktorHofer on Jun 23, 2019

That is not true. We use tools to generate source code and that source code is checked in and directly compiled by CSC.

I meant that it isn’t generated by the C# ref assembly feature (which is how most people should be generating reference assemblies).

tannergooding on Jan 24, 2019

Given that the C# ref assembly feature does preserve the attributes (where applicable), this should probably be moved to dotnet/arcade so we can update GenFacades.

I think the removal of [Conditional("CONTRACTS_FULL")] from the PureAttribute class is likely a separate proposal (which would likely live in CoreFX).

CC. @ericstj, @ViktorHofer

tannergooding on Jan 24, 2019

@tannergooding I just confirmed that csc does emit [Pure] attributes in ref assemblies.

But the PureAttribute class itself has [Conditional("CONTRACTS_FULL")] applied, so unless that constant is defined, the attribute will be dropped from the ref and the impl assembly alike.

AArnott on Jan 23, 2019