-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Initial support for "Simple Programs" feature. #41706
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.
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
Initial support for "Simple Programs" feature. #41706
Conversation
This is a punch through to support simple single file scenarios. Related to dotnet#41704.
|
Would you like feedback on the draft? Or would you prefer that wait until a later point? #Closed |
We use Draft PRs for personal review. #Closed |
Ok sounds good. The reason i brought it up was because there are other PRs going through that affect statement parsing (both in Master and in features/local-function-attributes), so i wanted to try to help out to make that as smooth as possible. Will provide more info once you're ready for that. Thanks! #Closed |
|
@dotnet/roslyn-compiler Please review. |
| if (!IsScript) | ||
| { | ||
| IsInAsync = true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this logic be commented? it's not clear ot me why a top level using forces an async parsing context. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not clear ot me why a top level using forces an async parsing context.
Simple Program is implicitly async. I guess the speclet doesn't say that explicitly, but it can be inferred from the first code snippet in the https://github.com/dotnet/csharplang/blob/master/proposals/Simple-programs.md#semantics section. I'll clarify that.
In reply to: 379666212 [](ancestors = 379666212)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple Program is implicitly async.
Ah, good to know.
I'll clarify that.
👍 #Closed
| if (this.IsIncrementalAndFactoryContextMatches) | ||
| { | ||
| if (CanReuseMemberDeclaration(CurrentNodeKind)) | ||
| if (CanReuseMemberDeclaration(CurrentNodeKind, isGlobalNonScript: isGlobal && !IsScript)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels a little weird to need to pass a global-parsing-state-flag into the nested helper. i.e. it feels cleaner to me that CanReuseMemberDeclaration just reference IsScript within it. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it feels cleaner to me that CanReuseMemberDeclaration just reference IsScript within it.
Could you please elaborate? IsScript simply indicates what kind parse options we are using, it says nothing about the context.
In reply to: 379668721 [](ancestors = 379668721)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it feels like you could just call CanReuseMemberDeclaration(CurrentNodeKind, isGlobal). Then, inside CanReuseMemberDeclaration if you care if it is global and non-script you can just have a local in there like var isGlobalNonScript = isGlobal && !IsScript;.
In other words, It doesn't feel like teh caller has to provide this information. But maybe i'm missing something here. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way of putting it: contextual information should pass downward. Data that is the same for the entire doc (like IsScript) doesn't need to pass down, it can just be retrieved when needed. #Resolved
| if (IsFieldDeclaration(isEvent: false)) | ||
| { | ||
| if (acceptStatement) | ||
| if (isGlobal && !haveAttributes && !(haveModifiers && IsScript)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, these conditions are pretty hard to both grok and to determine correctness around. i don't really get a sense for why this is the appropriate rules for this sort of situation. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, these conditions are pretty hard to both grok and to determine correctness around. i don't really get a sense for why this is the appropriate rules for this sort of situation.
The conditions are mostly preserving original logic. acceptStatement was false if there were attributes or modifiers. We allow modifiers for non-script case though
In reply to: 379669424 [](ancestors = 379669424)
| // inGlobalNonScript context, but that would require extending NodeFlags enum, which | ||
| // already uses all bits available in a byte. | ||
| // At the same time that would allow us to reuse top-level statements (GlobalStatementSyntax) as well. | ||
| return !isGlobalNonScript; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. the .IsScript check can move here. #Resolved
|
I am holding off on reviewing this pending LDM review of the draft specification. I expect this prototype may be helpful in gathering some experience, but I believe that the draft specification is unlikely to be approved by the LDM in its current form, and changes are likely to affect implementation decisions in the prototype. #Resolved |
|
We're not going to block compiler development on the LDM approval process. The spec here is a work in progress that @MadsTorgersen is in sync with. As the LDM process evolves so will the implementation and the draft of the proposal. |
|
@jaredpar I did not intend to block anything. I expect this prototype may be helpful in gathering some experience with the proposed feature leading up to an LDM review of the proposed specification. I am not confident that a code review would be a good use of my time as I believe the specification is likely to change significantly or be withdrawn. I will be happy to explain why if/when we review the proposal in the LDM. There are other features that have been reviewed and are unimplemented for C# 9.0. But by all means, development can continue on this prototype if you believe that is the best way to move forward on LDM-approved features, and I hope it leads to useful insight for LDM review. #Resolved |
|
@dotnet/roslyn-compiler Please review. |
1 similar comment
|
@dotnet/roslyn-compiler Please review. |
| { | ||
| Binder outer = VisitCore(parent.Parent); // a binder for the body of the type enclosing this type | ||
| var container = ((NamespaceOrTypeSymbol)outer.ContainingMemberOrLambda).GetSourceTypeMember(parent.Identifier.ValueText, 0, SyntaxKind.EnumDeclaration, parent); | ||
| var container = ((NamespaceOrTypeSymbol)ContainingMemberOrLambdaSkippingSimpleProgramEntryPoint(outer)).GetSourceTypeMember(parent.Identifier.ValueText, 0, SyntaxKind.EnumDeclaration, parent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider splitting this statement and the similar change above into multiple lines #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider splitting this statement and the similar change above into multiple lines
In the next change (will go in as a separate PR) I am adjusting what binder we create in VisitCompilationUnit method, making the use of ContainingMemberOrLambdaSkippingSimpleProgramEntryPoint unnecessary. I am removing that helper and restoring call-sites to the state prior to this change.
In reply to: 381495525 [](ancestors = 381495525)
|
(self-requesting so the PR will show up in GitHub Pull Requests in VS Code) #Resolved |
RikkiGibson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some comments but I'm still working on reviewing this PR. I haven't fully reviewed LanguageParser.cs or the tests yet.
| if ((options & LookupOptions.LabelsOnly) != 0 && scope.IsLastBinderWithinMember()) | ||
| { | ||
| // Labels declared outside of a member are not visible inside. | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we only hit this line when a member references a label declared at the top level? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we only hit this line when a member references a label declared at the top level?
We hit this line for every label reference that cannot be resolved within a member body.
In reply to: 381535605 [](ancestors = 381535605)
| } | ||
|
|
||
| RoslynDebug.Assert(simpleProgramEntryPointSymbol is object); | ||
| entryPointCandidates.Clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we plan on using the entry point candidates we found at this point to give diagnostics in a future PR? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we plan on using the entry point candidates we found at this point to give diagnostics in a future PR?
What diagnostics do you have in mind? We already reported ErrorCode.WRN_MainIgnored above. Is this not sufficient?
In reply to: 381552010 [](ancestors = 381552010)
| // Do not instrument implicitly-declared methods, except for constructors. | ||
| // Instrument implicit constructors in order to instrument member initializers. | ||
| if (method.IsImplicitlyDeclared && !method.IsImplicitConstructor) | ||
| if (method.IsImplicitlyDeclared && !method.IsImplicitConstructor && !(method is SynthesizedSimpleProgramEntryPointSymbol)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect to be able to instrument simple programs? How is that done? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect to be able to instrument simple programs?
Why wouldn't we?
How is that done?
As any other method body, I guess.
In reply to: 381637134 [](ancestors = 381637134)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misread what the check was actually doing here. Chatted offline and the change makes sense to me now.
In reply to: 382160492 [](ancestors = 382160492,381637134)
| case SyntaxKind.UnsafeKeyword: | ||
| if (this.PeekToken(1).Kind == SyntaxKind.OpenBraceToken) | ||
| { | ||
| return CheckFeatureAvailability(_syntaxFactory.GlobalStatement(ParseUnsafeStatement())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider naming this method something like "CheckSimpleProgramsFeatureAvailability" #Pending
| /// It should be used as a guardrail, not as a crutch, so it asserts if no progress was made. | ||
| /// </summary> | ||
| protected bool IsMakingProgress(ref int lastTokenPosition) | ||
| protected bool IsMakingProgress(ref int lastTokenPosition, bool assertIfFalse = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this change be reverted before merging to master? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this change be reverted before merging to master?
I do not have plans to do that at the moment. This doesn't change behavior for any existing code.
In reply to: 381652952 [](ancestors = 381652952)
| /// <summary> | ||
| /// Represents implicitly declared type for a Simple Program feature. | ||
| /// </summary> | ||
| internal sealed class SimpleProgramNamedTypeSymbol : SourceMemberContainerTypeSymbol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed there is a Synthesized folder here where the SynthesizedSimpleProgramEntryPointSymbol has been placed, should this also go in there? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed there is a Synthesized folder here where the SynthesizedSimpleProgramEntryPointSymbol has been placed, should this also go in there?
This is where ImplicitNamedTypeSymbol lives, which is another flavor of the same. I decided to keep them close.
In reply to: 381655876 [](ancestors = 381655876)
| @@ -0,0 +1,22 @@ | |||
| Simple Programs | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this "top-level statements"? "Simple programs" is more the goal of the feature, rather than the feature itself. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| internal static CSharpSyntaxNode ExtractReturnTypeSyntax(this MethodSymbol method) | ||
| { | ||
| if (method is SynthesizedSimpleProgramEntryPointSymbol synthesized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't expect this to work at all. This method is asking for the syntax of the stated return type, but we don't have a stated return type. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't expect this to work at all. This method is asking for the syntax of the stated return type, but we don't have a stated return type.
We need this method to return reasonable non-null node. To the user it can be surfaced only as an error location. The current implementation is reasonable enough, in my opinion. There is a PROTOTYPE comment to give it another thought later.
In reply to: 382794614 [](ancestors = 382794614)
| private int _recursionDepth; | ||
| private TerminatorState _termState; // Resettable | ||
| private bool _isInTry; // Resettable | ||
| private bool _checkedSimpleProgramsFeatureAvailability; // Resettable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just to prevent reporting an error for each statement? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just to prevent reporting an error for each statement?
Yes.
In reply to: 382803207 [](ancestors = 382803207)
| return locals?.ToImmutableAndFree() ?? ImmutableArray<LocalFunctionSymbol>.Empty; | ||
| } | ||
|
|
||
| internal override bool IsLocalFunctionsScopeBinder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> true? It keeps things closer together, which is easier for me to read. #WontFix
| /// reduce the stack usage during recursive parsing. | ||
| /// </summary> | ||
| /// <returns>Returns null if we can't parse anything (even partially).</returns> | ||
| private MemberDeclarationSyntax ParseMemberDeclarationOrStatementCore(SyntaxKind parentKind) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should probably mention it's only meant to be used at the top level #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should probably mention it's only meant to be used at the top level
The name is clear enough, I believe, it parses either a member or a statement. That is only valid to do at the top level.
In reply to: 382846924 [](ancestors = 382846924)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were also parsing in class declarations for error recovery. Is that wrong? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were also parsing in class declarations for error recovery. Is that wrong?
We don't parse statements within a class or a namespace declaration.
In reply to: 382871052 [](ancestors = 382871052)
|
|
||
| private static Symbol ContainingMemberOrLambdaSkippingSimpleProgramEntryPoint(Binder binder) | ||
| { | ||
| var containingMemberOrLambda = binder.ContainingMemberOrLambda; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the above, I guess this sometimes (always?) returns types or namespaces. Why is it called ContainingMemberOrLambda? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the above, I guess this sometimes (always?) returns types or namespaces. Why is it called ContainingMemberOrLambda?
Because some binders return members or lambdas or even local functions. This PR doesn't change this behavior of the API.
In reply to: 382851209 [](ancestors = 382851209)
|
|
||
| RoslynDebug.Assert(simpleProgramEntryPointSymbol is object); | ||
| entryPointCandidates.Clear(); | ||
| entryPointCandidates.Add(simpleProgramEntryPointSymbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the remaining pieces of this method are validating that the entry point is acceptable, but since we're synthesizing it I presume we already know that. Why not just return the symbol immediately? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the remaining pieces of this method are validating that the entry point is acceptable, but since we're synthesizing it I presume we already know that. Why not just return the symbol immediately?
This is the candidate and I do not mind doing for it what we would do for any other candidate. I believe this is more robust approach than the suggested one because we don't need to worry if any changes below made in the future invalidate the assumption that it is safe to return.
In reply to: 382854192 [](ancestors = 382854192)
| { | ||
| var saveTerm = _termState; | ||
| _termState |= TerminatorState.IsPossibleStatementStartOrStop; // partial statements can abort if a new statement starts | ||
| bool wasInAsync = IsInAsync; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method only supposed to be called to parse things contained directly in the compilation unit? I'm wondering if what scenario do we get here and IsInAsync was already 'true'. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method only supposed to be called to parse things contained directly in the compilation unit? I'm wondering if what scenario do we get here and IsInAsync was already 'true'.
I prefer to not make any assumptions, this is more robust in the long term.
In reply to: 382864944 [](ancestors = 382864944)
| var topLevelStatement = ParseLocalDeclarationStatement(); | ||
| IsInAsync = wasInAsync; | ||
|
|
||
| if (topLevelStatement is DeclarationSyntax declaration && IsMakingProgress(ref lastTokenPosition, assertIfFalse: false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a strange way to detect whether the statement that was parsed was meaningful. Could we maybe extract out a helper for ParseLocalDeclarationStatement that returns null when it has nothing to consume from the source text, and the ParseLocalDeclarationStatement method can call it and replace with an "empty" LocalDeclarationStatement to preserve behavior for existing callers? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a strange way to detect whether the statement that was parsed was meaningful. Could we maybe extract out a helper for ParseLocalDeclarationStatement that returns null when it has nothing to consume from the source text, and the ParseLocalDeclarationStatement method can call it and replace with an "empty" LocalDeclarationStatement to preserve behavior for existing callers?
I think that would still require checking if we are making any progress, but in a different place. I'd rather keep this logic here, to avoid people calling the wrong helper unintentionally.
In reply to: 382867655 [](ancestors = 382867655)
|
@agocke I think I responded to all your comments. #Closed |
| Debug.Assert(identifierOrThisOpt != null); | ||
|
|
||
| // check availability of readonly members feature for indexers, properties and methods | ||
| CheckForVersionSpecificModifiers(modifiers, SyntaxKind.ReadOnlyKeyword, MessageID.IDS_FeatureReadOnlyMembers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels odd to potentially issue LangVersion diagnostics for readonly members here since it doesn't seem like such members are ever allowed at the top level. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels odd to potentially issue LangVersion diagnostics for readonly members here since it doesn't seem like such members are ever allowed at the top level.
The more errors we can report for misplaced declaration, the better, IMHO.
In reply to: 382868202 [](ancestors = 382868202)
|
One question #41706 (comment). LGTM otherwise. #Resolved |
| comp = CreateCompilation(text1 + text2, options: TestOptions.DebugExe, parseOptions: DefaultParseOptions); | ||
| comp.VerifyDiagnostics(); | ||
|
|
||
| Assert.Throws<System.ArgumentException>(() => CreateCompilation(new[] { Parse(text1, filename: "text1", DefaultParseOptions), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this throw? Because of inconsistent parse options between syntax trees? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this throw? Because of inconsistent parse options between syntax trees?
Yes. This PR doesn't change this behavior.
In reply to: 382937604 [](ancestors = 382937604)
| Test().EndsWith(null); // 3 | ||
| System.Func<string> d = Test; // 4 | ||
| d(); | ||
| _ = nameof(Test); // 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be a // 5 comment here if there is no diagnostic? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be a // 5 comment here if there is no diagnostic?
Comments are there to easily distinguish lines from each other in the base-line they are not indicating anything else. There is no requirement to have any matching diagnostics. Nevertheless, once the PROTOTYPE comment below is addressed, there will be a diagnostics for this line.
In reply to: 383037398 [](ancestors = 383037398)
| Test().EndsWith(null); // 8 | ||
| var d = new System.Func<string>(Test); // 9 | ||
| d(); | ||
| _ = nameof(Test); // 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be a // 10 comment here if there is no diagnostic? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be a // 10 comment here if there is no diagnostic?
Same response. Baselines set expectations, not comments in the test snippets.
In reply to: 383037440 [](ancestors = 383037440)
This is a punch through to support simple single file scenarios.
Related to #41704.