Ref assemblies should not drop [Pure] attribute from APIs

[AArnott]

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 dotnet/corefx#31071

[tannergooding]

Do the reference assemblies produced by the C# compiler include these types of attributes. CC. @jaredpar?

[AArnott]

@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.

[Grauenwolf]

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.

[jaredpar]

Do the reference assemblies produced by the C# compiler include these types of attributes.

Yes we do keep (as @AArnott confirmed). Consider that if we did not then it would significantly hamper analyzers. Any analyzer based on attributes would essentially be un-usable in the face of reference assemblies.

[tannergooding]

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

[AArnott]

Is it GenFacades that needs to define the symbol or corefx? Are the assemblies generated from the corefx repo, and if so, wouldn't corefx be the repo that needs to be changed to define the symbol?

Or we could resolve this issue by removing the [Conditional] attribute. Either way, it seems like corefx is the repo to change, isn't it?

[tannergooding]

The ref assemblies in CoreFX are special and are not generated by the C# compiler, instead they are (outside a couple exceptions) generated by some special tooling in the arcade repo. The root issue here is that the tooling in arcade is doing something different than the C# compiler (specifically, it isn't counting attributes as part of the public API). There have been other cases of this in the past, and we have had to fix those cases as well (such as counting private fields of structs as part of this public API).

So, ultimately we likely need two (possibly three issues):

• An issue in dotnet/arcade tracking us updating the tooling that generates the CoreFX reference assemblies (ensuring that generated reference assemblies include attributes as part of the public API)
• An issue in dotnet/corefx tracking us consuming the new tooling and regenerating all of our reference assemblies (ensuring that the shipping reference assemblies now include attributes)
• Potentially an issue in dotnet/corefx tracking the removal of [Conditional("CONTRACTS_FULL")] from PureAttribute (which would ensure that this attribute ends up in the ref assembly)

[ericstj]

The ref assemblies in CoreFX are special and are not generated by the C# compiler, instead they are (outside a couple exceptions) generated by some special tooling in the arcade repo.

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

Did you mean GenAPI? GenAPI will persist the pure attribute if it exists. That's controlled by an exclusion list and PureAttribute isn't in it. https://github.com/dotnet/corefx/blob/master/eng/DefaultGenApiDocIds.txt

If the PureAttribute is present in the source you can run GenAPI and it will add it to the checked in source file. If the PureAttribute is in the checked in source file it will be preserved in the compiled reference assembly.

[AArnott]

If the PureAttribute is present in the source you can run GenAPI and it will add it to the checked in source file. If the PureAttribute is in the checked in source file it will be preserved in the compiled reference assembly.

I assume you mean after the [Conditional] attribute is removed, or if the csc tool is invoked to generate that ref assembly with the CONTRACTS_FULL symbol defined, right @ericsj?

[ericstj]

@AArnott you are correct. I was missing that aspect of this issue. PureAttribute would need to be persisted into the assembly that GenAPI reads, so you'd need to build with CONTRACTS_FULL once before running GenAPI.

[tannergooding]

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).

[ericstj]

This worked for me:

1. Add CONTRACTS_FULL to https://github.com/dotnet/corefx/blob/8fea941db55fd79635fde1a54058620ddac16961/src/System.Collections.Immutable/src/System.Collections.Immutable.csproj#L12
2. dotnet msbuild
3. dotnet msbuild /t:GenerateReferenceSource
4. observe the differences to the generated source file in the rfeference assembly folder (and add back the ifdefs as you wish)
5. add a dependency to the reference assembly on System.Diagnostics.Contracts so that it builds again. Add the CONTRACTS_FULL define as well.
6. Build the reference assembly.
7. Observe pureattributes in your reference assembly.

[ViktorHofer]

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

[ViktorHofer]

@ericstj what should we do here? I know that we made changes to GenAPI and our other tools, so this might already be fixed?

[AArnott]

@ViktorHofer There is still no [Pure] attribute on ImmutableList<T>.Add on the very latest preview of the package on nuget.org.

[ericstj]

Yeah, this is not fixed. I think we should just consider adding CONTRACTS_FULL as a default define in the dotnet/runtime build then updating the reference assemblies that are impacted. Any objections?

[ericstj]

So Immutable.Collections is the only assembly that actually needs this, so I'll just constrain the Define to that assembly.

[AArnott]

@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?

[ericstj]

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]

Per discussion we're actually planning to remove the Pure attribute usage: #35118 (comment)

[stephentoub]

@ericstj, should this be closed now, or is it still tracking work?

[ViktorHofer]

Triage: @ericstj will take a look

[ericstj]

We agreed to stop using Pure and removed its usage from the codebase. We could now remove the exception from our attribute lists; @Anipik feel free to clean that up. Closing this as the OP is addressed