-
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
Basic handling of function pointer types in the IDE #44564
Conversation
* Offer `delegate` keyword when function pointer types are permissible. * Completion inside function pointer type argument lists (including valid ref modifiers) * SymbolKey support. * Basic formatting support. * Support in add/remove parameter.
@CyrusNajmabadi @jasonmalinowski I'm not publishing this yet because the previous PR isn't merged, but the relevant changes are in the last commit and are basically ready for review (sans tests). I'm going to get started on tests locally, but I'd appreciate an initial pass of the implementation. |
src/VisualStudio/CSharp/Impl/ChangeSignature/CSharpChangeSignatureViewModelFactoryService.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/ChangeSignature/AddParameterDialogViewModel.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/SymbolKey/SymbolKey.FunctionPointerTypeSymbolKey.cs
Outdated
Show resolved
Hide resolved
...ces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions_Accessibility.cs
Outdated
Show resolved
Hide resolved
...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxNodeExtensions.cs
Outdated
Show resolved
Hide resolved
...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs
Show resolved
Hide resolved
...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/SyntaxTreeExtensions.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/SyntaxTreeExtensions.cs
Outdated
Show resolved
Hide resolved
* Added tests for all the new code paths. * Fixed up the formatter. * Fixed up extract method's understanding of rvalues for function pointers.
… ide * dotnet/features/function-pointers: Revert implicit change. Rename test Other typo. Minor PR feedback. Add additional conversion verification. PR Feedback:
src/EditorFeatures/CSharp/AutomaticCompletion/Sessions/LessAndGreaterThanCompletionSession.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest2/Recommendations/DecimalKeywordRecommenderTests.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest2/Recommendations/DelegateKeywordRecommenderTests.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest2/Recommendations/DelegateKeywordRecommenderTests.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest2/Recommendations/DelegateKeywordRecommenderTests.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest2/Recommendations/DelegateKeywordRecommenderTests.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest2/Recommendations/DynamicKeywordRecommenderTests.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest2/Recommendations/ReadOnlyKeywordRecommenderTests.cs
Show resolved
Hide resolved
@dotnet/roslyn-compiler @AlekseyTs for the small compiler api change change in this PR. |
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.
Compiler changes LGTM.
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenFunctionPointersTests.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest2/Recommendations/DelegateKeywordRecommenderTests.cs
Outdated
Show resolved
Hide resolved
...Studio/VisualBasic/Impl/ChangeSignature/VisualBasicChangeSignatureViewModelFactoryService.vb
Show resolved
Hide resolved
...ndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs
Show resolved
Hide resolved
fwiw, i also had my brain break on that localization message. what about |
I updated it to |
@@ -122,6 +124,7 @@ static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.UnaryPattern(Microsoft.CodeAn | |||
*REMOVED*Microsoft.CodeAnalysis.CSharp.Syntax.ObjectCreationExpressionSyntax.ArgumentList.get -> Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentListSyntax | |||
*REMOVED*Microsoft.CodeAnalysis.CSharp.Syntax.ObjectCreationExpressionSyntax.Initializer.get -> Microsoft.CodeAnalysis.CSharp.Syntax.InitializerExpressionSyntax | |||
*REMOVED*Microsoft.CodeAnalysis.CSharp.Syntax.ObjectCreationExpressionSyntax.NewKeyword.get -> Microsoft.CodeAnalysis.SyntaxToken | |||
*REMOVED*static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.ParseTypeName(string text, int offset = 0, bool consumeFullText = true) -> Microsoft.CodeAnalysis.CSharp.Syntax.TypeSyntax |
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.
Shouldn't we keep this API? #Closed
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.
Or is the only change that we removed default values for parameters?
In reply to: 432672190 [](ancestors = 432672190)
/// <summary> | ||
/// Parse a TypeNameSyntax node using the grammar rule for type names. | ||
/// </summary> | ||
public static TypeSyntax ParseTypeName(string text, int offset = 0, ParseOptions? options = null, bool consumeFullText = true) |
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.
options [](start = 90, length = 7)
Why is this needed? #Closed
@@ -15,6 +15,7 @@ Imports Microsoft.CodeAnalysis.VisualBasic.SyntaxFacts | |||
Imports InternalSyntax = Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax | |||
Imports Microsoft.CodeAnalysis.Syntax | |||
Imports System.Collections.Immutable | |||
Imports System.ComponentModel |
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.
Is this needed? #Closed
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.
@@ -191,14 +192,25 @@ Namespace Microsoft.CodeAnalysis.VisualBasic | |||
''' </summary> | |||
''' <param name="text">The input string</param> | |||
''' <param name="offset">The starting offset in the string</param> | |||
Public Shared Function ParseTypeName(text As String, Optional offset As Integer = 0, Optional consumeFullText As Boolean = True) As TypeSyntax | |||
Using p = New InternalSyntax.Parser(MakeSourceText(text, offset), VisualBasicParseOptions.Default) | |||
Public Shared Function ParseTypeName(text As String, Optional offset As Integer = 0, Optional options As ParseOptions = Nothing, Optional consumeFullText As Boolean = True) As TypeSyntax |
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.
ParseOptions [](start = 113, length = 12)
Should this be VisualBasicParseOptions
as in ParseCompilationUnit
and other similar APIs? #Closed
/// <summary> | ||
/// Parse a TypeNameSyntax node using the grammar rule for type names. | ||
/// </summary> | ||
public static TypeSyntax ParseTypeName(string text, int offset = 0, ParseOptions? options = null, bool consumeFullText = true) |
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.
ParseOptions [](start = 76, length = 12)
Should this be CSharpParseOptions
as in ParseCompilationUnit
? #Closed
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.
We seem to be inconsistent as to whether we take a ParseOptions or a CSharpParseOptions, but given that this (and the rest where we take a ParseOptions
) just casts immediately I'll make it CSharpParseOptions
.
In reply to: 432679991 [](ancestors = 432679991)
/// <summary> | ||
/// Parse a TypeNameSyntax node using the grammar rule for type names. | ||
/// </summary> | ||
public static TypeSyntax ParseTypeName(string text, int offset = 0, ParseOptions? options = null, bool consumeFullText = true) |
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.
ParseTypeName [](start = 33, length = 13)
Please add targeted compiler test for this API #Closed
@@ -191,14 +192,25 @@ Namespace Microsoft.CodeAnalysis.VisualBasic | |||
''' </summary> | |||
''' <param name="text">The input string</param> | |||
''' <param name="offset">The starting offset in the string</param> | |||
Public Shared Function ParseTypeName(text As String, Optional offset As Integer = 0, Optional consumeFullText As Boolean = True) As TypeSyntax | |||
Using p = New InternalSyntax.Parser(MakeSourceText(text, offset), VisualBasicParseOptions.Default) | |||
Public Shared Function ParseTypeName(text As String, Optional offset As Integer = 0, Optional options As ParseOptions = Nothing, Optional consumeFullText As Boolean = True) As TypeSyntax |
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.
ParseTypeName [](start = 31, length = 13)
Please add targeted compiler test for this API #Closed
Done with review pass for changes under Compilers (iteration 13) #Closed |
@AlekseyTs addressed feedback In reply to: 636138129 [](ancestors = 636138129) |
public void UsingTest() | ||
{ | ||
CreateCompilationWithFunctionPointers(@" | ||
using s = delegate*<void>;").VerifyDiagnostics(); |
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.
I would expect us to try using alias in a meaningful way, compile and run the code. Could be followed up on in a separate PR. #Closed
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.
It looks like this test is failing in CI, a change is needed anyway.
In reply to: 432721087 [](ancestors = 432721087)
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.
I would recommend adding a parse tree test for this scenario
In reply to: 432723697 [](ancestors = 432723697,432721087)
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 (iteration 17), assuming CI is passing.
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.
Auto-approval
delegate
keyword when function pointer types are permissible.