Allow users to define EmbeddedAttribute #76523

333fred · 2024-12-19T20:13:58Z

Alternative approach to #71546. Today, our enforcement of whether to allow users to define Microsoft.CodeAnalysis.EmbeddedAttribute is inconsistent; if we need to generate it, we error if the user defines one. But if the we don't need to generate it (as we increasingly do not since .NET 6), we allow the user to define their own version. This PR standardizes our enforcement, and enables the compiler to use the user-declared attribute instead generating one. We also document the breaking change from the standardized enforcement.

…ser-defined-embedded * origin/source-gen-warning: Additional testing Add tests

333fred · 2024-12-19T20:19:57Z

@dotnet/roslyn-compiler for review. I'll propose a source-generation API to allow authors to create this API easily, but this change can go in separately. For VB, there are existing tests verifying that it respects the EmbeddedAttribute definition, and allows defining in the same assembly. I opted not to enforce any shape on the VB as it doesn't do any form of validation in any scenario for the attribute definition today. We now validate the VB side, because we will synthesize an EmbeddedAttribute application for the type. This has also been documented as a breaking change.

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs

src/Compilers/CSharp/Test/Emit3/Attributes/AttributeTests_Embedded.cs

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs

AlekseyTs · 2024-12-20T05:37:19Z

Done with review pass (commit 8)

…tself didn't have one applied to it.

333fred · 2024-12-21T01:28:53Z

@AlekseyTs I believe I've address your feedback, thanks!

jjonescz · 2024-12-23T08:35:00Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md

+
+***Introduced in Visual Studio 2022 version 17.13***
+
+The compiler now validates the shape of `Microsoft.CodeAnalysis.EmbeddedAttribute` when declared in source. Previously, the compiler


Unrelated to the PR: it's not clear to me what the Microsoft.CodeAnalysis.EmbeddedAttribute does. Is that documented somewhere?

AlekseyTs · 2024-12-23T16:46:07Z

docs/compilers/Visual Basic/Compiler Breaking Changes - DotNet 10.md

+***Introduced in Visual Studio 2022 version 17.13***
+
+The compiler now validates the shape of `Microsoft.CodeAnalysis.EmbeddedAttribute` when declared in source. Previously, the compiler
+would allow user-defined declarations of this attribute, but only when it didn't need to generate one itself. We now validate that:


, but only when it didn't need to generate one itself

", but only when it didn't need to generate one itself" Is this part accurate for VB?

AlekseyTs · 2024-12-23T16:47:05Z

docs/compilers/Visual Basic/Compiler Breaking Changes - DotNet 10.md

+7. It must be allowed on any type declaration (class, struct, interface, enum, or delegate)
+
+```cs
+namespace Microsoft.CodeAnalysis;


Are we supposed to use VB in this document?

AlekseyTs · 2024-12-23T16:50:19Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs

+
+            if (this.IsMicrosoftCodeAnalysisEmbeddedAttribute() && GetEarlyDecodedWellKnownAttributeData() is null or { HasCodeAnalysisEmbeddedAttribute: false })
+            {
+                // This is Microsoft.CodeAnalysis.EmbeddedAttribute, and the user didn't manually apply this attribute to itself. Grab the parameterless constructor


// This is Microsoft.CodeAnalysis.EmbeddedAttribute

Consider adding an assert to verify the fact that "This is Microsoft.CodeAnalysis.EmbeddedAttribute"

AlekseyTs · 2024-12-23T16:53:04Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs

+                // 7. It must be allowed on any type declaration (class, struct, interface, enum, or delegate)
+                // 8. It must be non-generic (checked as part of IsMicrosoftCodeAnalysisEmbeddedAttribute, we don't error on this because both types can exist)
+
+                const AttributeTargets ExpectedTargets = AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface | AttributeTargets.Enum | AttributeTargets.Delegate;


ExpectedTargets

It looks like the code is not following naming conventions for locals. Despite the const modifier, this is a local.

AlekseyTs · 2024-12-23T17:01:14Z

src/Compilers/CSharp/Test/Emit3/Attributes/AttributeTests_Embedded.cs

+                {
+                    public void M(in int p)
+                    {
+                        // This should trigger generating another EmbeddedAttribute


// This should trigger generating another EmbeddedAttribute

Is this accurate?

AlekseyTs · 2024-12-23T17:07:20Z

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceMemberContainerTypeSymbol.vb

@@ -60,6 +60,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols
            ReportedVarianceDiagnostics = &H2    ' Set if variance diagnostics have been reported.
            ReportedBaseClassConstraintsDiagnostics = &H4    ' Set if base class constraints diagnostics have been reported.
            ReportedInterfacesConstraintsDiagnostics = &H8    ' Set if constraints diagnostics for base/implemented interfaces have been reported.
+            ReportedCodeAnalysisEmbeddedAttributeDiagnostics = &H10 ' Set if 


' Set if

It looks like the comment is not finished

AlekseyTs · 2024-12-23T17:15:28Z

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceNamedTypeSymbol.vb

+                ' 7. It must be allowed on any type declaration (class, struct, interface, enum, Or delegate)
+                ' 8. It must be non-generic
+
+                Dim expectedTargets = AttributeTargets.Class Or AttributeTargets.Struct Or AttributeTargets.Interface Or AttributeTargets.Enum Or AttributeTargets.Delegate


Dim

Const?

AlekseyTs · 2024-12-23T17:26:41Z

src/Compilers/CSharp/Test/Emit3/Attributes/AttributeTests_Embedded.cs

@@ -20,7 +22,8 @@ public void ReferencingEmbeddedAttributesFromTheSameAssemblySucceeds()
            var code = @"
 namespace Microsoft.CodeAnalysis
 {
-    internal class EmbeddedAttribute : System.Attribute { }
+    [Embedded]


[Embedded]

It looks like this change (the attribute application) is no longer necessary here and in other similar places.

AlekseyTs · 2024-12-23T17:28:45Z

src/Compilers/CSharp/Test/Emit3/Attributes/AttributeTests_Embedded.cs

+                {
+                    public void M(in int p)
+                    {
+                        // This should trigger generating another EmbeddedAttribute


// This should trigger generating another EmbeddedAttribute

Is this accurate?

AlekseyTs · 2024-12-23T17:30:07Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs

@@ -1748,6 +1752,21 @@ internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, r
                        ImmutableArray.Create(new TypedConstant(compilation.GetWellKnownType(WellKnownType.System_Type), TypedConstantKind.Type, originalType)),
                        isOptionalUse: true));
            }
+
+            if (this.IsMicrosoftCodeAnalysisEmbeddedAttribute() && GetEarlyDecodedWellKnownAttributeData() is null or { HasCodeAnalysisEmbeddedAttribute: false })


&& GetEarlyDecodedWellKnownAttributeData() is null or { HasCodeAnalysisEmbeddedAttribute: false }

Are we testing positive and negative effect of this condition?

AlekseyTs · 2024-12-23T17:30:32Z

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceNamedTypeSymbol.vb

+            If Me.IsMicrosoftCodeAnalysisEmbeddedAttribute() Then
+                Dim earlyAttributeData = GetEarlyDecodedWellKnownAttributeData()
+
+                If earlyAttributeData Is Nothing OrElse Not earlyAttributeData.HasCodeAnalysisEmbeddedAttribute Then


earlyAttributeData Is Nothing OrElse Not earlyAttributeData.HasCodeAnalysisEmbeddedAttribute

Are we testing positive and negative effect of this condition?

AlekseyTs · 2024-12-23T17:31:18Z

Done with review pass (commit 10)

333fred added 6 commits December 17, 2024 15:39

Work on embedded attribute recognition in source

ea2ae9d

Add tests

92fc3ee

Additional testing

6e7e00f

Refactoring for simplicity

2c20205

Merge remote-tracking branch 'origin/source-gen-warning' into allow-u…

5b57054

…ser-defined-embedded * origin/source-gen-warning: Additional testing Add tests

Always validate in source, and document breaking change

bfec56d

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 19, 2024

Typo

ea31a28

333fred marked this pull request as ready for review December 19, 2024 20:18

333fred requested a review from a team as a code owner December 19, 2024 20:18

Update test

ffe043b

333fred mentioned this pull request Dec 19, 2024

Do not report conflicting types errors for source-generated APIs #71546

Closed

AlekseyTs reviewed Dec 20, 2024

View reviewed changes

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md Show resolved Hide resolved

AlekseyTs reviewed Dec 20, 2024

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Dec 20, 2024

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Attributes/AttributeTests_Embedded.cs Show resolved Hide resolved

AlekseyTs reviewed Dec 20, 2024

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Attributes/AttributeTests_Embedded.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Dec 20, 2024

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs Outdated Show resolved Hide resolved

333fred added 2 commits December 20, 2024 14:53

Synthesize an application of EmbeddedAttribute if EmbeddedAttribute i…

28d5fd6

…tself didn't have one applied to it.

Add VB validation implementation.

fc211ce

jjonescz reviewed Dec 23, 2024

View reviewed changes

AlekseyTs reviewed Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to define EmbeddedAttribute #76523

Allow users to define EmbeddedAttribute #76523

333fred commented Dec 19, 2024

333fred commented Dec 19, 2024 •

edited

Loading

AlekseyTs commented Dec 20, 2024

333fred commented Dec 21, 2024

jjonescz Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs Dec 23, 2024

AlekseyTs commented Dec 23, 2024


		*Introduced in Visual Studio 2022 version 17.13*

		The compiler now validates the shape of `Microsoft.CodeAnalysis.EmbeddedAttribute` when declared in source. Previously, the compiler

Allow users to define EmbeddedAttribute #76523

Are you sure you want to change the base?

Allow users to define EmbeddedAttribute #76523

Conversation

333fred commented Dec 19, 2024

333fred commented Dec 19, 2024 • edited Loading

AlekseyTs commented Dec 20, 2024

333fred commented Dec 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 23, 2024

333fred commented Dec 19, 2024 •

edited

Loading