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

Jump to bottom

Report all-empty top level statements. #54385

Merged
merged 3 commits into from
Jun 25, 2021
Merged

Report all-empty top level statements. #54385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1838375
Report all-empty top level statements.
AlekseyTs Jun 25, 2021
6edd1cc
Fix a test
AlekseyTs Jun 25, 2021
a716471
PR feedback
AlekseyTs Jun 25, 2021
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/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6722,4 +6722,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_BuilderAttributeDisallowed" xml:space="preserve">
<value>The AsyncMethodBuilder attribute is disallowed on anonymous methods without an explicit return type.</value>
</data>
<data name="ERR_SimpleProgramIsEmpty" xml:space="preserve">
<value>At least one top-level statement must be non-empty.</value>
</data>
</root>
21 changes: 18 additions & 3 deletions src/Compilers/CSharp/Portable/Declarations/DeclarationTreeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ private ImmutableArray<SingleNamespaceOrTypeDeclaration> VisitNamespaceChildren(
bool isIterator = false;
bool hasReturnWithExpression = false;
GlobalStatementSyntax firstGlobalStatement = null;
bool hasNonEmptyGlobalSatement = false;

var childrenBuilder = ArrayBuilder<SingleNamespaceOrTypeDeclaration>.GetInstance();
foreach (var member in members)
Expand All @@ -74,6 +75,11 @@ private ImmutableArray<SingleNamespaceOrTypeDeclaration> VisitNamespaceChildren(
firstGlobalStatement ??= global;
var topLevelStatement = global.Statement;

if (!topLevelStatement.IsKind(SyntaxKind.EmptyStatement))
{
hasNonEmptyGlobalSatement = true;
}
Comment on lines +78 to +81
Copy link
Member

@Youssef1313 Youssef1313 Jun 25, 2021 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

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 {}
``` #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.


if (!hasAwaitExpressions)
{
hasAwaitExpressions = SyntaxFacts.HasAwaitOperations(topLevelStatement);
Expand All @@ -98,7 +104,16 @@ private ImmutableArray<SingleNamespaceOrTypeDeclaration> VisitNamespaceChildren(
// wrap all global statements in a compilation unit into a simple program type:
if (firstGlobalStatement is object)
{
childrenBuilder.Add(CreateSimpleProgram(firstGlobalStatement, hasAwaitExpressions, isIterator, hasReturnWithExpression));
var diagnostics = ImmutableArray<Diagnostic>.Empty;

if (!hasNonEmptyGlobalSatement)
{
var bag = DiagnosticBag.GetInstance();
bag.Add(ErrorCode.ERR_SimpleProgramIsEmpty, ((EmptyStatementSyntax)firstGlobalStatement.Statement).SemicolonToken.GetLocation());
diagnostics = bag.ToReadOnlyAndFree();
}

childrenBuilder.Add(CreateSimpleProgram(firstGlobalStatement, hasAwaitExpressions, isIterator, hasReturnWithExpression, diagnostics));
Copy link
Member

@jcouv jcouv Jun 25, 2021 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

nit: extra empty line below #Resolved

}

// wrap all members that are defined in a namespace or compilation unit into an implicit type:
Expand Down Expand Up @@ -130,7 +145,7 @@ private static SingleNamespaceOrTypeDeclaration CreateImplicitClass(ImmutableSeg
diagnostics: ImmutableArray<Diagnostic>.Empty);
}

private static SingleNamespaceOrTypeDeclaration CreateSimpleProgram(GlobalStatementSyntax firstGlobalStatement, bool hasAwaitExpressions, bool isIterator, bool hasReturnWithExpression)
private static SingleNamespaceOrTypeDeclaration CreateSimpleProgram(GlobalStatementSyntax firstGlobalStatement, bool hasAwaitExpressions, bool isIterator, bool hasReturnWithExpression, ImmutableArray<Diagnostic> diagnostics)
{
return new SingleTypeDeclaration(
kind: DeclarationKind.SimpleProgram,
Expand All @@ -144,7 +159,7 @@ private static SingleNamespaceOrTypeDeclaration CreateSimpleProgram(GlobalStatem
nameLocation: new SourceLocation(firstGlobalStatement.GetFirstToken()),
memberNames: ImmutableSegmentedDictionary<string, VoidResult>.Empty,
children: ImmutableArray<SingleTypeDeclaration>.Empty,
diagnostics: ImmutableArray<Diagnostic>.Empty);
diagnostics: diagnostics);
}

/// <summary>
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1959,6 +1959,7 @@ internal enum ErrorCode
ERR_CantConvAnonMethReturnType = 8934,
ERR_BuilderAttributeDisallowed = 8935,
ERR_FeatureNotAvailableInVersion10 = 8936,
ERR_SimpleProgramIsEmpty = 8937,

#endregion

Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hans.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hant.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Compilers/CSharp/Test/Semantic/Semantics/FunctionPointerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,12 @@ public void UsingAliasTest()
Diagnostic(ErrorCode.ERR_IdentifierExpectedKW, "delegate").WithArguments("", "delegate").WithLocation(2, 11),
// (2,25): error CS0116: A namespace cannot directly contain members such as fields or methods
// using s = delegate*<void>;
Diagnostic(ErrorCode.ERR_NamespaceUnexpected, ">").WithLocation(2, 25)
Diagnostic(ErrorCode.ERR_NamespaceUnexpected, ">").WithLocation(2, 25),

// See same-named test in TopLevelStatementsParsingTests, there is a single top-level statement in the tree and it is an empty statement.
// (2,26): error CS8937: At least one top-level statement must be non-empty.
// using s = delegate*<void>;
Diagnostic(ErrorCode.ERR_SimpleProgramIsEmpty, ";").WithLocation(2, 26)
);
}

Expand Down
93 changes: 93 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/TopLevelStatementsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8819,5 +8819,98 @@ void M()
Statements (0)
");
}

[Fact]
public void EmptyStatements_01()
{
var text = @";";

var comp = CreateCompilation(text, options: TestOptions.DebugExe, parseOptions: DefaultParseOptions);
comp.VerifyDiagnostics(
// (1,1): error CS8937: At least one top-level statement must be non-empty.
// ;
Diagnostic(ErrorCode.ERR_SimpleProgramIsEmpty, ";").WithLocation(1, 1)
);
}

[Fact]
public void EmptyStatements_02()
{
var text = @";;


;;
;";

var comp = CreateCompilation(text, options: TestOptions.DebugExe, parseOptions: DefaultParseOptions);
comp.VerifyDiagnostics(
// (1,1): error CS8937: At least one top-level statement must be non-empty.
// ;;
Diagnostic(ErrorCode.ERR_SimpleProgramIsEmpty, ";").WithLocation(1, 1)
);
}

[Fact]
public void EmptyStatements_03()
{
var text = @"
System.Console.WriteLine(""Hi!"");
;;
;
";

var comp = CreateCompilation(text, options: TestOptions.DebugExe, parseOptions: DefaultParseOptions);
CompileAndVerify(comp, expectedOutput: "Hi!");
}

[Fact]
public void EmptyStatements_04()
{
var text = @"
;;
;
System.Console.WriteLine(""Hi!"");";

var comp = CreateCompilation(text, options: TestOptions.DebugExe, parseOptions: DefaultParseOptions);
CompileAndVerify(comp, expectedOutput: "Hi!");
}

[Fact]
public void EmptyStatements_05()
{
var text = @"
;
System.Console.WriteLine(""Hi!"");
;
";

var comp = CreateCompilation(text, options: TestOptions.DebugExe, parseOptions: DefaultParseOptions);
CompileAndVerify(comp, expectedOutput: "Hi!");
}
Copy link
Member

@jcouv jcouv Jun 25, 2021 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

nit: Consider adding a test from the original issue using System; ; or something like it #Resolved


[Fact]
public void EmptyStatements_06()
{
var text =
@"
using System;
;

class Program
{
static void Main(String[] args) {}
}
";

var comp = CreateCompilation(text, options: TestOptions.DebugExe, parseOptions: DefaultParseOptions);
comp.VerifyDiagnostics(
// (3,1): error CS8937: At least one top-level statement must be non-empty.
// ;
Diagnostic(ErrorCode.ERR_SimpleProgramIsEmpty, ";").WithLocation(3, 1),
// (7,17): warning CS7022: The entry point of the program is global code; ignoring 'Program.Main(string[])' entry point.
// static void Main(String[] args) {}
Diagnostic(ErrorCode.WRN_MainIgnored, "Main").WithArguments("Program.Main(string[])").WithLocation(7, 17)
);
}
}
}
Loading