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.

Sign up for GitHub

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

Implement ExperimentalAttribute #85444

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Implement ExperimentalAttribute #85444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libraries/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.Shared.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,9 @@
<type fullname="System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverageAttribute">
<attribute internal="RemoveAttributeInstances" />
</type>
<type fullname="System.Diagnostics.CodeAnalysis.ExperimentalAttribute">
<attribute internal="RemoveAttributeInstances" />
</type>
stephentoub marked this conversation as resolved.
Show resolved Hide resolved

<!-- System.Reflection -->
<type fullname="System.Reflection.AssemblyCompanyAttribute">
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\DynamicallyAccessedMembersAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\DynamicDependencyAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\ExcludeFromCodeCoverageAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\ExperimentalAttribute.cs" />
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\NullableAttributes.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\UnscopedRefAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\RequiresAssemblyFilesAttribute.cs" />
Expand Down
55 changes: 55 additions & 0 deletions ...aries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/ExperimentalAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System.Diagnostics.CodeAnalysis
{
/// <summary>
/// Indicates that an API is experimental and it may change in the future.
/// </summary>
/// <remarks>
/// This attribute allows call sites to be flagged with a diagnostic that indicates that an experimental
/// feature is used. Authors can use this attribute to ship preview features in their assemblies.
/// </remarks>
[AttributeUsage(AttributeTargets.Assembly |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joperezr @stephentoub Let's not include "Assembly" and "Module" in the list. I don't think those targets are valid for other attributes of this kind (Obsolete, Windows.Foundation.Metadata.Experimental, Windows.Foundation.Metadata.Deprecated, ...).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @terrajobst @geeknoid

Thanks for the feedback @jcouv. Aside from the fact that other attributes don't support this, is there a particular reason why you don't think we should allow Assembly or Module use? Seems to me that there are valid scenarios where people developing new assemblies might opt to use it at that level, in the past this has been done mostly via using prerelease package versions but I think that not everybody uses that and there are some scenarios where it might be better to have stable p ackages with assemblies marked as experimental in them.

Copy link
Member

@jcouv jcouv Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason for my question is precedent. The secondary reason is implementation cost (deviating from existing attributes means additional complexity and testing).
I'm not saying it's a bad idea, but since the previous design worked, I think we'd need a strong reason to revise it (why was it sufficient for those other attributes but not this one).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough. @bartonjs @terrajobst @stephentoub Do you know off the top of your head why the Obsolete attribute doesn't support Assembly or Module targets? I would have expected the same arguments should be made for both Obsolete and Experimental, since they are essentially the same attribute but just tracking the two opposing ends of an API lifecycle.

Copy link
Member Author

@RussKie RussKie Jun 20, 2023 •
edited by joperezr
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe dotnet/extensions has assembly-level applications of the attribute. E.g., https://github.com/dotnet/extensions/blob/d558f8de86adf93044bc5386bd4367b3d509ead3/Directory.Build.targets#L68-L72

Copy link
Member

@terrajobst terrajobst Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the platform compat attributes we allowed assembly/module and we use it quite a bit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with excluding them for now if we want to match ObsoleteAttribute. We can revisit then we run into limitations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geeknoid FYI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the compiler/roslyn change was merged.

AttributeTargets.Module |
AttributeTargets.Class |
AttributeTargets.Struct |
AttributeTargets.Enum |
AttributeTargets.Constructor |
AttributeTargets.Method |
AttributeTargets.Property |
AttributeTargets.Field |
AttributeTargets.Event |
AttributeTargets.Interface |
AttributeTargets.Delegate, Inherited = false)]
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
public sealed class ExperimentalAttribute : Attribute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joperezr @stephentoub I just want to confirm the expected user experience with an example. I'm re-using a message we already have for APIs marked as experimental.

Let me know if you have any feedback.

    [Fact]
    public void Test()
    {
        var src = """
C.M();

[System.Diagnostics.CodeAnalysis.Experimental("DiagID1", UrlFormat = "https://example.org/{0}")]
class C
{
    public static void M() { }
}
""";

        var comp = CreateCompilation(new[] { src, experimentalAttributeSrc });
        comp.VerifyDiagnostics(
            // 0.cs(1,1): warning DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates. (https://example.org/DiagID1)
            // C.M();
            Diagnostic("DiagID1", "C").WithArguments("C").WithLocation(1, 1)
            );

        var diag = comp.GetDiagnostics().Single();
        Assert.Equal("DiagID1", diag.Id);
        Assert.Equal(ErrorCode.WRN_Experimental, (ErrorCode)diag.Code);
        Assert.Equal("https://example.org/DiagID1", diag.Descriptor.HelpLinkUri);
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, the above looks right to me. You can then also write a compilation which passes in the NoWarn for your diagnostic which should not show it any more, or alternatively suppress at each call site by using pragma.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

{
/// <summary>
/// Initializes a new instance of the <see cref="ExperimentalAttribute"/> class, specifying the ID that the compiler will use
/// when reporting a use of the API the attribute applies to.
/// </summary>
/// <param name="diagnosticId">The ID that the compiler will use when reporting a use of the API the attribute applies to.</param>
public ExperimentalAttribute(string diagnosticId)
{
DiagnosticId = diagnosticId;
}

/// <summary>
/// Gets or sets the ID that the compiler will use when reporting a use of the API the attribute applies to.
/// </summary>
/// <value>The unique diagnostic ID.</value>
/// <remarks>
/// The diagnostic ID is shown in build output for warnings and errors.
/// <para>This property represents the unique ID that can be used to suppress the warnings or errors, if needed.</para>
/// </remarks>
public string DiagnosticId { get; }

/// <summary>
/// Gets or sets the URL for corresponding documentation.
/// The API accepts a format string instead of an actual URL, creating a generic URL that includes the diagnostic ID.
/// </summary>
/// <value>The format string that represents a URL to corresponding documentation.</value>
/// <remarks>An example format string is <c>https://contoso.com/obsoletion-warnings/{0}</c>.</remarks>
public string? UrlFormat { get; set; }
}
}
7 changes: 7 additions & 0 deletions src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8281,6 +8281,13 @@ public sealed partial class ExcludeFromCodeCoverageAttribute : System.Attribute
public ExcludeFromCodeCoverageAttribute() { }
public string? Justification { get { throw null; } set { } }
}
[System.AttributeUsage(System.AttributeTargets.Assembly | System.AttributeTargets.Module | System.AttributeTargets.Class | System.AttributeTargets.Struct | System.AttributeTargets.Enum | System.AttributeTargets.Constructor | System.AttributeTargets.Method | System.AttributeTargets.Property | System.AttributeTargets.Field | System.AttributeTargets.Event | System.AttributeTargets.Interface | System.AttributeTargets.Delegate, Inherited = false)]
public sealed class ExperimentalAttribute : System.Attribute
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
{
public ExperimentalAttribute(string diagnosticId) { }
public string DiagnosticId { get { throw null; } }
public string? UrlFormat { get; set; }
}
[System.AttributeUsageAttribute(System.AttributeTargets.Field | System.AttributeTargets.Parameter | System.AttributeTargets.Property | System.AttributeTargets.ReturnValue, Inherited=false)]
public sealed partial class MaybeNullAttribute : System.Attribute
{
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
<Compile Include="System\Attributes.cs" />
<Compile Include="System\AttributeUsageAttributeTests.cs" />
<Compile Include="System\BadImageFormatExceptionTests.cs" />
<Compile Include="System\Diagnostics\CodeAnalysis\ExperimentalAttributeTests.cs" />
<Compile Include="System\NumberFormatTestHelper.cs" />
<Compile Include="System\StringTests.GenericMath.cs" />
<Compile Include="System\BooleanTests.cs" />
Expand Down
40 changes: 40 additions & 0 deletions ...raries/System.Runtime/tests/System/Diagnostics/CodeAnalysis/ExperimentalAttributeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Xunit;

namespace System.Diagnostics.CodeAnalysis.Tests
{
public class ExperimentalAttributeTests
{
[Theory]
// Note: Once the compiler support is implemented these should fail
[InlineData("")]
[InlineData(" ")]
[InlineData("\t")]
// Note end
[InlineData("diagnosticId")]
public void TestConstructor(string expected)
{
var attr = new ExperimentalAttribute(expected);

Assert.Equal(expected, attr.DiagnosticId);
Assert.Null(attr.UrlFormat);
}

[Theory]
[InlineData(null)]
[InlineData("")]
[InlineData("https://contoso.com/obsoletion-warnings/{0}")]
public void TestSetUrlFormat(string urlFormat)
{
var attr = new ExperimentalAttribute("diagnosticId")
{
UrlFormat = urlFormat
};

Assert.Equal("diagnosticId", attr.DiagnosticId);
Assert.Equal(urlFormat, attr.UrlFormat);
}
}
}