Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Basic handling of function pointer types in the IDE #44564

Merged
merged 17 commits into from
May 29, 2020
Merged

Basic handling of function pointer types in the IDE #44564

merged 17 commits into from
May 29, 2020

Conversation

333fred
Copy link
Member

@333fred 333fred commented May 26, 2020

  • 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.

@333fred
Basic handling of function pointer types in the IDE:
d1971b8 
* 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.
@333fred 333fred added Area-IDE Feature - Function Pointers Adding Function Pointers labels May 26, 2020
@333fred
Copy link
Member Author

333fred commented May 26, 2020

@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.

@333fred
PR Feedback and Tests
c703569 
* Added tests for all the new code paths.
* Fixed up the formatter.
* Fixed up extract method's understanding of rvalues for function pointers.
@333fred
Merge remote-tracking branch 'dotnet/features/function-pointers' into…
3d643fa 
… ide

* dotnet/features/function-pointers:
  Revert implicit change.
  Rename test
  Other typo.
  Minor PR feedback.
  Add additional conversion verification.
  PR Feedback:
@333fred 333fred marked this pull request as ready for review May 28, 2020 19:33
@333fred 333fred requested review from a team as code owners May 28, 2020 19:33
@333fred
Copy link
Member Author

333fred commented May 29, 2020

@dotnet/roslyn-compiler @AlekseyTs for the small compiler api change change in this PR.

cston
cston approved these changes May 29, 2020
View reviewed changes
Copy link
Member

@cston cston left a comment

Choose a reason for hiding this comment

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

Compiler changes LGTM.

@CyrusNajmabadi
Copy link
Member

fwiw, i also had my brain break on that localization message. what about taking the address of a method instead of using &?

@333fred
Copy link
Member Author

333fred commented May 29, 2020 •
edited
Loading

fwiw, i also had my brain break on that localization message. what about taking the address of a method instead of using &?

I updated it to &method group. I'm not going to change it further at this point. If we get feedback after release we can change it then.

AlekseyTs
AlekseyTs reviewed May 29, 2020
View reviewed changes
src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt
@@ -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
Copy link
Contributor

@AlekseyTs AlekseyTs May 29, 2020 •
edited
Loading

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

Copy link
Contributor

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)

AlekseyTs
AlekseyTs reviewed May 29, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Syntax/SyntaxFactory.cs Outdated
/// <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)
Copy link
Contributor

@AlekseyTs AlekseyTs May 29, 2020 •
edited
Loading

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

AlekseyTs
AlekseyTs reviewed May 29, 2020
View reviewed changes
src/Compilers/VisualBasic/Portable/Syntax/SyntaxNodeFactories.vb
@@ -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
Copy link
Contributor

@AlekseyTs AlekseyTs May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is this needed? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's for the EditorBrowsableAttribute.

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

AlekseyTs
AlekseyTs reviewed May 29, 2020
View reviewed changes
src/Compilers/VisualBasic/Portable/Syntax/SyntaxNodeFactories.vb Outdated
@@ -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
Copy link
Contributor

@AlekseyTs AlekseyTs May 29, 2020 •
edited
Loading

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

AlekseyTs
AlekseyTs reviewed May 29, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Syntax/SyntaxFactory.cs Outdated
/// <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)
Copy link
Contributor

@AlekseyTs AlekseyTs May 29, 2020 •
edited
Loading

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

Copy link
Member Author

@333fred 333fred May 29, 2020 •
edited
Loading

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)

AlekseyTs
AlekseyTs reviewed May 29, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Syntax/SyntaxFactory.cs Outdated
/// <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)
Copy link
Contributor

@AlekseyTs AlekseyTs May 29, 2020 •
edited
Loading

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

AlekseyTs
AlekseyTs reviewed May 29, 2020
View reviewed changes
src/Compilers/VisualBasic/Portable/Syntax/SyntaxNodeFactories.vb Outdated
@@ -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
Copy link
Contributor

@AlekseyTs AlekseyTs May 29, 2020 •
edited
Loading

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

@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 29, 2020 •
edited
Loading

Done with review pass for changes under Compilers (iteration 13) #Closed

@333fred
Copy link
Member Author

333fred commented May 29, 2020

@AlekseyTs addressed feedback

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

@333fred 333fred requested a review from a team as a code owner May 29, 2020 19:54
AlekseyTs
AlekseyTs reviewed May 29, 2020
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/FunctionPointerTests.cs Outdated
public void UsingTest()
{
CreateCompilationWithFunctionPointers(@"
using s = delegate*<void>;").VerifyDiagnostics();
Copy link
Contributor

@AlekseyTs AlekseyTs May 29, 2020 •
edited
Loading

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

Copy link
Contributor

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)

Copy link
Contributor

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)

AlekseyTs
AlekseyTs approved these changes May 29, 2020
View reviewed changes
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 (iteration 17), assuming CI is passing.

ghost
ghost approved these changes May 29, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@333fred 333fred merged commit 91e77e7 into dotnet:features/function-pointers May 29, 2020
@333fred 333fred deleted the ide branch May 29, 2020 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@cston cston cston approved these changes

Assignees
No one assigned
Labels
Area-IDE Feature - Function Pointers Adding Function Pointers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@333fred @CyrusNajmabadi @AlekseyTs @jasonmalinowski @cston