Skip to content

Produce errors for 'partial file record'#62686

Merged
jcouv merged 5 commits intodotnet:mainfrom
Youssef1313:partial-must-be-last-modifier
Jul 21, 2022
Merged

Produce errors for 'partial file record'#62686
jcouv merged 5 commits intodotnet:mainfrom
Youssef1313:partial-must-be-last-modifier

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jul 15, 2022 •
edited
Loading

Fixes #62679
Fixes #22439

@Youssef1313 Youssef1313 requested a review from a team as a code owner July 15, 2022 20:46
@ghost ghost added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jul 15, 2022
@jcouv jcouv assigned jcouv and RikkiGibson and unassigned jcouv Jul 15, 2022
@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 16, 2022

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

[Fact]
public void EntryPoint_02()
{
const string text1 = @"
using System.Threading.Tasks;
using System;
public partial class C
{
public static partial Task Main();
public static partial async Task Main()
{
Console.Write(1);
}
}";
var verifier = CompileAndVerify(
text1,
parseOptions: TestOptions.RegularWithExtendedPartialMethods,
options: TestOptions.DebugExe,
expectedOutput: "1");
verifier.VerifyDiagnostics(
// (8,38): warning CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
// public static partial async Task Main()
Diagnostic(ErrorCode.WRN_AsyncLacksAwaits, "Main").WithLocation(8, 38));
}

Here, partial is allowed before async 😕

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 16, 2022 •
edited
Loading

Draft for now. I'm working on a different fix. It's ready now :)

@Youssef1313 Youssef1313 marked this pull request as draft July 16, 2022 06:46
Youssef1313
Youssef1313 commented Jul 16, 2022
View reviewed changes
Copy link
Member Author

@Youssef1313 Youssef1313 left a 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?

Youssef1313
Youssef1313 commented Jul 16, 2022
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/LambdaTests.cs
Comment on lines +6786 to +6787
[Fact]
public void StaticPartialLambda()
Copy link
Member Author

@Youssef1313 Youssef1313 Jul 16, 2022

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:

roslyn/src/Compilers/CSharp/Portable/Binder/Binder_Lambda.cs

Line 307 in 7693a6f

ModifierUtils.ToDeclarationModifiers(syntax.Modifiers, diagnostics.DiagnosticBag ?? new DiagnosticBag());
to report ERR_PartialMisplaced.

This test makes sure this will not be forgotten.

Youssef1313
Youssef1313 commented Jul 16, 2022
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/EnumTests.cs
Comment on lines +406 to +412
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));
Copy link
Member Author

@Youssef1313 Youssef1313 Jul 16, 2022

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.

Youssef1313
Youssef1313 commented Jul 16, 2022
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/LocalFunctionTests.cs
Comment on lines +129 to +130
[Fact]
public void StaticPartialLocalFunction()
Copy link
Member Author

@Youssef1313 Youssef1313 Jul 16, 2022

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);

@Youssef1313 Youssef1313 marked this pull request as ready for review July 16, 2022 08:40
@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 20, 2022

@jcouv for second review. Thanks!

jcouv
jcouv approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@jcouv jcouv left a 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)

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 21, 2022

@jcouv @RikkiGibson Is this ready to merge?

@jcouv jcouv merged commit d614564 into dotnet:main Jul 21, 2022
@ghost ghost added this to the Next milestone Jul 21, 2022
@jcouv
Copy link
Member

jcouv commented Jul 21, 2022

Indeed. Thanks @Youssef1313!

@Youssef1313 Youssef1313 deleted the partial-must-be-last-modifier branch July 21, 2022 16:51
333fred added a commit to 333fred/roslyn that referenced this pull request Jul 21, 2022
@333fred
Merge remote-tracking branch 'upstream/main' into forbid-inumber
230897a 
* 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
  ...
This was referenced Jul 26, 2022
Closed
Closed
@allisonchou allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022
@ryzngard ryzngard mentioned this pull request Jul 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@RikkiGibson RikkiGibson

@jcouv jcouv

Labels

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

Projects

None yet

Milestone

17.4 P1

Development

Successfully merging this pull request may close these issues.

Bad parser behavior when combining file, partial, and record partial check is in the parser because of initial problems moving it to binding

4 participants

@Youssef1313 @jcouv @RikkiGibson @allisonchou