Report all-empty top level statements. #54385
Conversation
| if (!topLevelStatement.IsKind(SyntaxKind.EmptyStatement)) | ||
| { | ||
| hasNonEmptyGlobalSatement = true; | ||
| } |
There was a problem hiding this comment.
Should the error be reported if the top-level program contains only local functions?
i.e,
using System;;
//void M() { } // Currently, uncommenting this line will make the error goes away if I interpret the implementation correctly.
class Program {}
``` #ResolvedThere was a problem hiding this comment.
Should the error be reported if the top-level program contains only local functions?
#53472 (comment) - "LDM decided to make it an error if all top-level statements are empty." I do not see LDM deciding anyjthing about local function statements.
There was a problem hiding this comment.
@AlekseyTs Yeah I see the PR matches LDM decision, but I mean, was that considered?
The goal of the error is to tell the user "your program will execute nothing and just return 0 immediately", which is the case when the top-level statements are local functions.
There was a problem hiding this comment.
The goal of the error is to tell the user "your program will execute nothing and just return 0 immediately",
I do not think that is the goal, an entry point function is always executed. There is plenty of ways to write code that does nothing even when it has non-empty statements. We were addressing specific customers scenario and I don't think there was a goal to generalize the issue. If you feel that the issue should be generailized, open an issue and provide your reasons. I beleive a local function that is never called produces a warning, in my opinion that is good enough to give a hint that the code probably doesn't do what was intended.
|
@jcouv, @dotnet/roslyn-compiler For the second review. |
| diagnostics = bag.ToReadOnlyAndFree(); | ||
| } | ||
|
|
||
| childrenBuilder.Add(CreateSimpleProgram(firstGlobalStatement, hasAwaitExpressions, isIterator, hasReturnWithExpression, diagnostics)); |
There was a problem hiding this comment.
nit: extra empty line below #Resolved
|
|
||
| var comp = CreateCompilation(text, options: TestOptions.DebugExe, parseOptions: DefaultParseOptions); | ||
| CompileAndVerify(comp, expectedOutput: "Hi!"); | ||
| } |
There was a problem hiding this comment.
nit: Consider adding a test from the original issue using System; ; or something like it #Resolved
ghost
added this to the 
…ures/caller-argument-expression * upstream/main: (492 commits) Add nullable ref annotations to SyntaxFactory (#54199) Add 'replace' APIs and hook them up to the IDE (#54270) Allow implicit implementation of non-public interface members (#54182) Make insertion a stage of the official build (#54376) Cleanup Remove unused property Simplify glyph computation Report all-empty top level statements. (#54385) Calculate TypeParameterKind based on the container type (#54200) vert not roaming Split tests Multple matches Report as we get results Fixup tests Update tests Avoid thread dependency in VirtualMemoryNotificationListener constructor Fast progression search. Add LanguageVersion 10 (#54359) Sure, why not ...
Fixes #53472.