-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Produce errors for 'partial file record' #62686
Conversation
src/Compilers/CSharp/Test/Syntax/Parsing/FileModifierParsingTests.cs
Outdated
Show resolved
Hide resolved
The change here is revealing another bug has been for a long, and I don't think a breaking change here will be desirable. roslyn/src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs Lines 2330 to 2354 in 7693a6f
Here, |
|
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 changes here properly fix #22439?
[Fact] | ||
public void StaticPartialLambda() |
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.
If in future the parser allowed partial
, we should update this code path:
ModifierUtils.ToDeclarationModifiers(syntax.Modifiers, diagnostics.DiagnosticBag ?? new DiagnosticBag()); |
This test makes sure this will not be forgotten.
CreateCompilation("partial public enum E { }").VerifyDiagnostics( | ||
// (1,1): error CS0267: The 'partial' modifier can only appear immediately before 'class', 'record', 'struct', 'interface', or a method return type. | ||
// partial public enum E { } | ||
Diagnostic(ErrorCode.ERR_PartialMisplaced, "partial").WithLocation(1, 1), | ||
// (1,21): error CS0267: The 'partial' modifier can only appear immediately before 'class', 'record', 'struct', 'interface', or a method return type. | ||
// partial public enum E { } | ||
Diagnostic(ErrorCode.ERR_PartialMisplaced, "E").WithLocation(1, 21)); |
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.
The duplicate diagnostic is an existing behavior. But whether you want me to fix that in this PR or in a follow-up, I'm okay with that.
[Fact] | ||
public void StaticPartialLocalFunction() |
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 test serves the same purpose as the one I commented on for lambdas, but for this code path:
roslyn/src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs
Lines 47 to 49 in 7693a6f
_declarationModifiers = | |
DeclarationModifiers.Private | | |
syntax.Modifiers.ToDeclarationModifiers(diagnostics: _declarationDiagnostics.DiagnosticBag); |
@jcouv for second review. Thanks! |
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.
LGTM Thanks (iteration 4)
@jcouv @RikkiGibson Is this ready to merge? |
Indeed. Thanks @Youssef1313! |
* upstream/main: (53 commits) Remove 'mangleName' parameter in PENamedTypeSymbolNonGeneric (dotnet#62813) Produce errors for 'partial file record' (dotnet#62686) [EnC] Allow renaming methods, properties, events etc. (dotnet#62364) Update AbstractFileHeaderDiagnosticAnalyzer.cs Rename file Make static Move tests Refactor Support ref field assignment in object initializers (dotnet#62584) Fix cref tags on generated VB's SyntaxFactory (dotnet#62578) Simplify Simplify Simplify Simplify Simplify Simplify Simplify Simplify Move helper methods Remove serialization ...
Fixes #62679
Fixes #22439