New Function Pointer Syntax #45684

333fred · 2020-07-06T17:00:05Z

This updates function pointers to use the new syntax decided on in LDM. This is syntax-level changes only: the updated calling convention binding will come in the next PR.

Fixes #44312.

… generators on Linux doesn't produce newline differences.

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4

333fred · 2020-07-07T17:02:28Z

@dotnet/roslyn-compiler @AlekseyTs, this is now ready for review.

@dotnet/roslyn-ide, this includes syntax changes to function pointer types, and formatter/intellisense context fixes to ensure the existing tests still pass. We'll still need to make intellisense changes for the calling convention location at a later point.

333fred · 2020-07-07T17:04:52Z

src/Workspaces/CSharpTest/Formatting/FormattingTests_FunctionPointers.cs

+            await AssertFormatAsync(expected, content);
+        }
+
+        [Fact(Skip = "PROTOTYPE(func-ptr): Even though the rule is returning put a space, it's getting ignored, resulting in delegate*invalid <")]


I'm assuming that this is because invalid is parsed as trivia on a missing managed or unmanaged token? Those two are the only valid token types for the spot.

The "invalid" token isn't being parsed into the calling convention position? It looks based on the formatting change that's what you intended, right?

It is parsed into the calling convention position, it's parsed as trivia on a missing managed token.

In reply to: 451061448 [](ancestors = 451061448)

So I guess if the formatter's rule is we don't touch things around missing stuff, then maybe the behavior you're seeing is correct. Once the user has typed correct code, it'll format correctly; if the user put their code into a broken state while they're doing other editing, not touching is fine for that small window.

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs

jasonmalinowski · 2020-07-07T18:21:45Z

...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs

@@ -961,15 +961,27 @@ public static bool IsDefaultExpressionContext(this SyntaxTree syntaxTree, int po
            {
                case SyntaxKind.LessThanToken:
                case SyntaxKind.CommaToken:
-                    return token.Parent is FunctionPointerTypeSyntax;
+                    return token.Parent is FunctionPointerParameterList;


Looks like you can delete the reference to #39865 now on line 959. #Resolved

jasonmalinowski · 2020-07-07T18:27:33Z

src/Workspaces/CSharpTest/Formatting/FormattingTests_FunctionPointers.cs

+            await AssertFormatAsync(expected, content);
+        }
+
+        [Fact(Skip = "PROTOTYPE(func-ptr): Even though the rule is returning put a space, it's getting ignored, resulting in delegate*invalid <")]


The "invalid" token isn't being parsed into the calling convention position? It looks based on the formatting change that's what you intended, right?

jasonmalinowski · 2020-07-07T18:37:43Z

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/SpacingFormattingRule.cs

+                    switch (currentKind)
+                    {
+                        case SyntaxKind.IdentifierToken:
+                        case SyntaxKindEx.ManagedKeyword:
+                        case SyntaxKindEx.UnmanagedKeyword:
+                            return CreateAdjustSpacesOperation(1, AdjustSpacesOption.ForceSpacesIfOnSingleLine);
+                    }
+                }


If we just checked that the currentToken is the FunctionPointerCallingConvention's ManagedSpecifier then we can remove this block, right? And we're having to do this because of the funkiness around not being able to reference new syntax node types in our code?

Yeah, we could accomplish it that way. And yes, it would rely on new syntax node types.

In reply to: 451066787 [](ancestors = 451066787)

@333fred If you could either do that work or just file a bug to do it, I think I'd feel better since this then is a bit more flexible around precisely what we might get out of the parser.

Filed #45897.

...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs

src/Features/CSharp/Portable/Completion/KeywordRecommenders/ReadOnlyKeywordRecommender.cs

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

* Revert unneeded publicapi file changes. * Simplify handling in some IDE code.

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs

src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs

src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerMethodSymbol.cs

src/Compilers/CSharp/Portable/GlobalSuppressions.cs

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

...ource/CompilerGeneratorTools/Source/VisualBasicSyntaxGenerator/GreenNodes/GreenNodeWriter.vb

AlekseyTs · 2020-07-08T01:48:22Z

Done with review pass (iteration 8) #Closed

333fred · 2020-07-08T20:57:55Z

Because the API no longer exists.

In reply to: 655220662 [](ancestors = 655220662)

Refers to: src/Compilers/CSharp/Portable/GlobalSuppressions.cs:41 in c4be137. [](commit_id = c4be137, deletion_comment = True)

* Rename ManagedSpecifier for clarity. * Handle more cases in symbol binding and increase code clarity. * Deduplicate type checks when binding parameters.

333fred · 2020-07-08T23:29:45Z

@AlekseyTs addressed feedback.

In reply to: 655231532 [](ancestors = 655231532)

src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerMethodSymbol.cs

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenFunctionPointersTests.cs

AlekseyTs · 2020-07-09T17:12:35Z

Done with review pass (iteration 10) #Closed

333fred · 2020-07-09T19:04:29Z

@AlekseyTs addressed feedback.

In reply to: 656247690 [](ancestors = 656247690)

src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerMethodSymbol.cs

AlekseyTs

LGTM (iteration 11), modulo the remaining use of Text property #45684 (review)

333fred · 2020-07-09T22:17:47Z

@dotnet/roslyn-compiler for a second review.

333fred · 2020-07-09T22:57:08Z

@jasonmalinowski did you have any more comments?

jasonmalinowski · 2020-07-09T22:58:34Z

...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs

            }

-            return token.Parent is ParameterSyntax { Parent: FunctionPointerTypeSyntax _ };
+            return token switch


You might be able to merge this and the previous switch into a single pattern patch...

I didn't want to duplicate the parent is check. When we can use C# 9 or patterns this can be cleaned up :)

jasonmalinowski · 2020-07-09T22:59:35Z

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/SpacingFormattingRule.cs

+                    switch (currentKind)
+                    {
+                        case SyntaxKind.IdentifierToken:
+                        case SyntaxKindEx.ManagedKeyword:
+                        case SyntaxKindEx.UnmanagedKeyword:
+                            return CreateAdjustSpacesOperation(1, AdjustSpacesOption.ForceSpacesIfOnSingleLine);
+                    }
+                }


@333fred If you could either do that work or just file a bug to do it, I think I'd feel better since this then is a bit more flexible around precisely what we might get out of the parser.

jasonmalinowski

Looks good, regarding the two formatting tests I think you should update the baseline to lock the current behavior; if the rule is we don't touch spacing of tokens when something is missing, then the behavior is actually expected. It's just as likely somebody got themselves into that broken state and doesn't want to be fighting with the formatter while stuff is clearly broken.

Other request is to either move some code to use the types to simplify the formatter checks, or file a bug on the IDE for later cleanup. Your call.

jasonmalinowski · 2020-07-09T23:03:47Z

src/Workspaces/CSharpTest/Formatting/FormattingTests_FunctionPointers.cs

+            await AssertFormatAsync(expected, content);
+        }
+
+        [Fact(Skip = "PROTOTYPE(func-ptr): Even though the rule is returning put a space, it's getting ignored, resulting in delegate*invalid <")]


So I guess if the formatter's rule is we don't touch things around missing stuff, then maybe the behavior you're seeing is correct. Once the user has typed correct code, it'll format correctly; if the user put their code into a broken state while they're doing other editing, not touching is fine for that small window.

jasonmalinowski · 2020-07-09T23:04:10Z

src/Workspaces/CSharpTest/Formatting/FormattingTests_FunctionPointers.cs

+            await AssertFormatAsync(expected, content);
+        }
+
+        [Fact(Skip = "PROTOTYPE(func-ptr): Even though the rule is returning put a space, it's getting ignored, resulting in delegate*invalid [")]


Ditto: as long s it's formatting the stuff that it does understand, I guess that's good.

333fred · 2020-07-10T17:35:19Z

@dotnet/roslyn-compiler for a second review please

RikkiGibson · 2020-07-10T18:01:11Z

Taking a look

src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

* Enable tests as appropriate * Fix doc comments * Remove unnecessary generic parameters.

ghost

Auto-approval

333fred added 3 commits July 6, 2020 09:57

Generate VB syntax in a platform-agnostic manner, so that running the…

dcfe894

… generators on Linux doesn't produce newline differences.

Update generated syntax to the new form.

2477df9

Update parsing and binding implementation for the new syntax.

739e2e8

333fred added Area-Compilers Feature - Function Pointers Adding Function Pointers labels Jul 6, 2020

alrz reviewed Jul 6, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/Syntax.xml Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 6, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4 Show resolved Hide resolved

333fred added 3 commits July 6, 2020 17:14

Update IDE code.

4060db8

Fix FunctionPointerUnmanagedCallingConventionListSyntax naming.

5a8c762

Fixup public api, formatting, and suppressions.

c39f164

333fred requested review from AlekseyTs and a team July 7, 2020 17:00

333fred added the Area-IDE label Jul 7, 2020

333fred marked this pull request as ready for review July 7, 2020 17:00

333fred commented Jul 7, 2020

View reviewed changes

jasonmalinowski reviewed Jul 7, 2020

View reviewed changes

Fix FunctionPointerParameterList naming.

4f740fa

333fred commented Jul 7, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/Syntax.xml Outdated Show resolved Hide resolved

PR Feedback:

c4be137

* Revert unneeded publicapi file changes. * Simplify handling in some IDE code.