Skip to content

Comments

Add separate error for disallowed members in global namespace#81221

Merged
AlekseyTs merged 20 commits intodotnet:mainfrom
stbau04:feature/81141-enhance-properties-on-top-level
Dec 18, 2025
Merged

Add separate error for disallowed members in global namespace#81221
AlekseyTs merged 20 commits intodotnet:mainfrom
stbau04:feature/81141-enhance-properties-on-top-level

Conversation

@stbau04
Copy link
Contributor

@stbau04 stbau04 commented Nov 13, 2025

Fixes #81141
Adds a dedicated error, for when members are defined next to statements in the global namespace. The current error error CS0116: A namespace cannot directly contain members such as fields, methods or statements can be confusing, as statements (including local functions) can be defined as top level statements, but not any other members. I could not figure any suitable single message for both global statements and a namespace, therefore i split the message into two separate ones.

@dotnet-policy-service dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Nov 13, 2025
@CyrusNajmabadi
Copy link
Contributor

I know this is in draft, but i took a quick look. It's unclear why all teh changes are needed, versus just chnaging the resource string.

@stbau04
Copy link
Contributor Author

stbau04 commented Nov 18, 2025

@CyrusNajmabadi I could not think of a error message that would fit both situations properly. The best thing i could come with would be something similar to The scope cannot directly contain members such as fields, properties or event, but i feel like this is no enhancement at all as it is unclear what scope is meant

(that's why i went with two seperate errors in the first way)

@stbau04
Copy link
Contributor Author

stbau04 commented Nov 19, 2025

Those new labels are probably wrong, i just totally screwed up merging

@stbau04 stbau04 marked this pull request as ready for review November 20, 2025 19:36
@stbau04 stbau04 requested a review from a team as a code owner November 20, 2025 19:36
@stbau04
Copy link
Contributor Author

stbau04 commented Nov 27, 2025

@CyrusNajmabadi Would you maybe have a moment for a quick review? Or should i resolve the conflict first?

{
var parentSyntax = member.Parent;
var errorCode = parentSyntax.IsKind(SyntaxKind.CompilationUnit)
&& parentSyntax.ChildNodes().Any(node => node.IsKind(SyntaxKind.GlobalStatement))
Copy link
Contributor

Choose a reason for hiding this comment

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

this will do a linear walk on the compilation unit each time. instead of this, i would pass in the parent as the preceding argument (before 'members') in AddNonTypeMembers. Then compute and store if the comp-unit has global statements. (can just do parent is CompilationUnit compilationUnit && compilationUnit.Members.Any(m => m is GlobalStatementSyntax)).

int[] exclude = new int[] { (int)ErrorCode.ERR_NamespaceUnexpected };

compilation.GetDiagnostics().Where(d => !exclude.Contains(d.Code)).Verify(
// (3,21): error CS9344: The global namespace cannot directly contain members such as fields or properties
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of this, add ERR_GlobalNamespaceUnexpected to exclude as well.

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/81141")]
public void TestFieldInTopLevelStatement()
{
var source = """
Copy link
Contributor

Choose a reason for hiding this comment

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

for all new tessts, use raw-strings.

@CyrusNajmabadi
Copy link
Contributor

Would you maybe have a moment for a quick review?

Looking good. Just a few things to adjust.

@@ -31248,12 +31248,30 @@ public static System.Action TakeOutParam<T>(T y, out T x)
int[] exclude = new int[] { (int)ErrorCode.ERR_NamespaceUnexpected };
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 12, 2025

Choose a reason for hiding this comment

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

ERR_NamespaceUnexpected

I think we should replace this with ERR_CompilationUnitUnexpected and revert baseline changes below #Closed

@@ -31421,12 +31439,30 @@ public static System.Action TakeOutParam<T>(T y, out T x)
int[] exclude = new int[] { (int)ErrorCode.ERR_NamespaceUnexpected };
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 12, 2025

Choose a reason for hiding this comment

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

ERR_NamespaceUnexpected

Same here #Closed

// (3,12): error CS0214: Pointers and fixed size buffers may only be used in an unsafe context
// fixed bool a[2], b[H.TakeOutParam(1, out var x1)];
Diagnostic(ErrorCode.ERR_UnsafeNeeded, "a[2]").WithLocation(3, 12),
// (3,20): error CS0133: The expression being assigned to '<invalid-global-code>.b' must be constant
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 12, 2025

Choose a reason for hiding this comment

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

// (3,20): error CS0133: The expression being assigned to '.b' must be constant

Consider putting this error into its original position to avoid unnecessary diff. #Closed

}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/81141")]
public void TestFieldInNamespace()
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 12, 2025

Choose a reason for hiding this comment

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

TestFieldInNamespace

It doesn't look like there is a namespace declaration in the code. Could you please elaborate? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was a test that reported ERR_NamespaceUnexpected before the latest adaptions. Fixed it

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 12, 2025

@stbau04 It looks like some tests are failing #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 12, 2025

Done with review pass (commit 15) #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 17)

@AlekseyTs AlekseyTs requested a review from a team December 16, 2025 19:27
@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-compiler For a second review on a community PR

ERR_InequalityOperatorInPatternNotSupported = 9345,
ERR_EncUpdateRequiresEmittingExplicitInterfaceImplementationNotSupportedByTheRuntime = 9346,
ERR_ExtensionParameterInStaticContext = 9347,
ERR_CompilationUnitUnexpected = 9348,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to adjust the number in base-lines as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were talking about those, right? 11195ac

@AlekseyTs AlekseyTs merged commit 43bebc0 into dotnet:main Dec 18, 2025
25 checks passed
@AlekseyTs
Copy link
Contributor

@stbau04 Thank you for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. Needs UX Triage VSCode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enhance properties with top-level statements

4 participants