Main attributes for simple programs #59471
Conversation
chsienki
force-pushed
the
main_attributes
branch
from
0902ba2 to
5bf19bb
Compare
chsienki
force-pushed
the
main_attributes
branch
from
5bf19bb to
1c26df5
Compare
|
@chsienki Please add a link to the .md file with detailed feature specification. |
| @@ -22,7 +22,7 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols | |||
| /// </summary> | |||
| internal abstract class SourceMethodSymbolWithAttributes : SourceMethodSymbol, IAttributeTargetSymbol | |||
| { | |||
| private CustomAttributesBag<CSharpAttributeData> _lazyCustomAttributesBag; | |||
| internal CustomAttributesBag<CSharpAttributeData> _lazyCustomAttributesBag; | |||
There was a problem hiding this comment.
Oops, this was supposed to be protected not internal.
Was just re-using the attribute bag for the derived symbol, but I can make a copy on the derived symbol instead.
There was a problem hiding this comment.
but I can make a copy on the derived symbol instead.
I don't think that would be the right thing to do either. See my other comments
| @@ -233,7 +233,7 @@ internal ReturnTypeWellKnownAttributeData GetDecodedReturnTypeWellKnownAttribute | |||
| /// <remarks> | |||
| /// Forces binding and decoding of attributes. | |||
| /// </remarks> | |||
| private CustomAttributesBag<CSharpAttributeData> GetAttributesBag() | |||
| internal virtual CustomAttributesBag<CSharpAttributeData> GetAttributesBag() | |||
There was a problem hiding this comment.
This change looks suspicious. It feels that we simply should override
/// <summary>
/// Gets the syntax list of custom attributes that declares attributes for this method symbol.
/// </summary>
internal virtual OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclarations()
for SynthesizedSimpleProgramEntryPointSymbol #Closed
| /// <remarks> | ||
| /// Forces binding and decoding of attributes. | ||
| /// </remarks> | ||
| internal override CustomAttributesBag<CSharpAttributeData> GetAttributesBag() |
| return _lazyCustomAttributesBag!; | ||
| } | ||
|
|
||
| AttributeLocation IAttributeTargetSymbol.DefaultAttributeLocation => AttributeLocation.Main; |
| return _lazyCustomAttributesBag!; | ||
| } | ||
|
|
||
| AttributeLocation IAttributeTargetSymbol.DefaultAttributeLocation => AttributeLocation.Main; |
There was a problem hiding this comment.
This implementation feels suspicious. We are supposed to return "Location of an attribute if an explicit location is not specified via attribute target specification syntax." However, attributes without explicit "main" target shouldn't be applicable to this symbol. Perhaps the default location should be AttributeLocation.Assembly. That is what we use for attributes on assembly?
| { | ||
| if (_lazyCustomAttributesBag == null || !_lazyCustomAttributesBag.IsSealed) | ||
| { | ||
| var mergedAttributes = ((SourceAssemblySymbol)this.ContainingAssembly).GetAttributeDeclarations(); |
| { | ||
| if (_lazyCustomAttributesBag == null || !_lazyCustomAttributesBag.IsSealed) | ||
| { | ||
| var mergedAttributes = ((SourceAssemblySymbol)this.ContainingAssembly).GetAttributeDeclarations(); |
There was a problem hiding this comment.
Note that we are creating a SynthesizedSimpleProgramEntryPointSymbol per each compilation unit that has top-level statements. It doesn't feel right to bind all "main" attributes for every symbol like that. It looks like dotnet/csharplang#5045 suggests to allow "main" targeted attributes in any file, then perhaps we should bind attributes only for symbol that is returned by
internal static SynthesizedSimpleProgramEntryPointSymbol? GetSimpleProgramEntryPoint(CSharpCompilation compilation)
``` #Closed
| @@ -244,6 +244,7 @@ internal enum MessageID | |||
|
|
|||
| IDS_FeatureCacheStaticMethodGroupConversion = MessageBase + 12816, | |||
| IDS_FeatureRawStringLiterals = MessageBase + 12817, | |||
| IDS_FeatureMainAttrLoc = MessageBase + 12818, | |||
| @@ -3,6 +3,7 @@ Microsoft.CodeAnalysis.CSharp.SyntaxKind.InterpolatedRawStringEndToken = 9082 -> | |||
| Microsoft.CodeAnalysis.CSharp.SyntaxKind.InterpolatedSingleLineRawStringStartToken = 9080 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind | |||
| Microsoft.CodeAnalysis.CSharp.SyntaxKind.MultiLineRawStringLiteralToken = 9073 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind | |||
| Microsoft.CodeAnalysis.CSharp.SyntaxKind.SingleLineRawStringLiteralToken = 9072 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind | |||
| Microsoft.CodeAnalysis.CSharp.SyntaxKind.MainKeyword = 8447 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind | |||
There was a problem hiding this comment.
Since we are working in a feature branch, consider placing additions at the end. That is going to simplify merges. #Closed
It looks like implementation of this property should be adjusted to take "main" attributes into account Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs:384 in a07f5e0. [](commit_id = a07f5e0, deletion_comment = False) |
| @@ -556,6 +556,78 @@ public void TestMultipleGlobalAttributeDeclarations() | |||
| Assert.NotEqual(default, ad.CloseBracketToken); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void TestGlobalMainAttribute() | |||
| @@ -556,6 +556,78 @@ public void TestMultipleGlobalAttributeDeclarations() | |||
| Assert.NotEqual(default, ad.CloseBracketToken); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void TestGlobalMainAttribute() | |||
| public void TestGlobalMainAttribute_LangVersion() | ||
| { | ||
| var text = "[main:a]"; | ||
| var file = this.ParseFile(text, CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.CSharp10)); |
| @@ -5043,6 +5043,228 @@ class C<T> | |||
| }); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void TestMainAttributes() | |||
There was a problem hiding this comment.
Alternatively we could have a dedicated file for all "main" related tests.
| attribute.VerifyValue(0, TypedConstantKind.Primitive, "one"); | ||
| Assert.Equal(@"MyAttribute(""one"")", attribute.ToString()); | ||
|
|
||
| CompileAndVerify(source, expectedOutput: "Hello World"); |
| "); | ||
|
|
||
| CompileAndVerify(source, expectedOutput: "Hello World") | ||
| .VerifyTypeIL("Program", @" |
There was a problem hiding this comment.
| } | ||
|
|
||
| [ConditionalFact(typeof(CoreClrOnly))] | ||
| public void TestMainAttributes_IL() |
| verifyAttribute("two"), | ||
| verifyAttribute("three")); | ||
|
|
||
| CompileAndVerify(source, expectedOutput: "Hello World"); |
| [main: My(""one"")] | ||
| public class MyAttribute : Attribute { public MyAttribute(string name) {} } | ||
| "); | ||
| source.VerifyDiagnostics( |
| public class MyAttribute : Attribute {{ public MyAttribute(string name) {{}} }} | ||
| "); | ||
|
|
||
| source.VerifyDiagnostics(valid ? Array.Empty<DiagnosticDescription>() : |
| using System; | ||
|
|
||
| [main: My(""one"")] | ||
| [assembly: My(""two"")] |
There was a problem hiding this comment.
I think it will be good to test where an attribute without an explicit target is going to end up.
|
It looks like there are legitimate test failures. |
| CompileAndVerify(source, expectedOutput: "Hello World"); | ||
| } | ||
|
|
||
| [ConditionalFact(typeof(CoreClrOnly))] |
|
Done with review pass (commit 7) |
|
Sorry, closed by mistake. Reopened. |
| @@ -6986,4 +6986,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ | |||
| <data name="ERR_LineContainsDifferentWhitespace" xml:space="preserve"> | |||
| <value>Line contains different whitespace than the closing line of the raw string literal: '{0}' versus '{1}'</value> | |||
| </data> | |||
| <data name="IDS_FeatureMainAttrLoc" xml:space="preserve"> | |||
| <value>main as an attribute target specifier</value> | |||
There was a problem hiding this comment.
| <value>main as an attribute target specifier</value> | |
| <value>'main' as an attribute target specifier</value> |
|
@dotnet/roslyn-compiler PR is updated and ready for re-review. Accompanying speclet is here: dotnet/csharplang#5817 |
| @@ -302,7 +302,7 @@ private CustomAttributesBag<CSharpAttributeData> GetAttributesBag(ref CustomAttr | |||
| /// <summary> | |||
| /// Gets the attributes applied on this symbol. | |||
| /// Returns an empty array if there are no attributes. | |||
| /// </summary> | |||
| /// </summary> | |||
| attrLocation = CheckFeatureAvailability(attrLocation, MessageID.IDS_FeatureModuleAttrLoc); | ||
| } | ||
| AttributeLocation.Module => CheckFeatureAvailability(attrLocation, MessageID.IDS_FeatureModuleAttrLoc), | ||
| AttributeLocation.Main => CheckFeatureAvailability(attrLocation, MessageID.IDS_FeatureMainAttributeLocation), |
There was a problem hiding this comment.
We generally want to prefer doing semantic checks for these types of things if reasonable to do.
| // if we're the entry point that will ultimately be selected. | ||
| if (this == GetSimpleProgramEntryPoint(DeclaringCompilation)) | ||
| { | ||
| return OneOrMany.Create(((SourceAssemblySymbol)ContainingAssembly).GetAttributeDeclarations()); |
There was a problem hiding this comment.
Where is the later filtering on these for only main declarations?
| }; | ||
| return methods.Cast<MethodSymbol>().ToImmutableArray(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add some test cases showing local function interactions, something like this scenario:
[main: My]
void M() {}
|
Moved to draft since LDM moved the feature to backlog (notes). |
Implements
[main: ...]attributes for top level / simple programs.Handles parsing, attribute lookup for
SynthesizedSimpleProgramEntryPointSymbol, as well as some basic diagnostics like langver and disallowing in scripting.Doesn't yet work for regular program entry points or issue diagnostics if you use
[main:in a non executable program.CSharpLang tracking issue: dotnet/csharplang#5045
Relates to test plan #57047