Function pointer calling convention IDE support #59062

Youssef1313 · 2022-01-25T19:48:07Z

...ces/CSharp/Portable/Classification/SyntaxClassification/CSharpSyntaxClassificationService.cs

...p/Portable/Classification/SyntaxClassification/FunctionPointerCallingConventionClassifier.cs

Sergio0694 · 2022-01-25T23:48:05Z

...p/Portable/Classification/SyntaxClassification/FunctionPointerCallingConventionClassifier.cs

+            var callingConventionSyntax = (FunctionPointerUnmanagedCallingConventionSyntax)syntax;
+            if (CSharpSyntaxFacts.Instance.IsSpecialUnmanagedCallingConvention(callingConventionSyntax))
+            {
+                result.Add(new ClassifiedSpan(callingConventionSyntax.Span, ClassificationTypeNames.ClassName));


As mentioned on Discord, this might actually be more appropriate to be highlighted as a keyword rather than a class, given the compiler is not looking up any type, and there's no associated type symbol here. As discussed with Fred, these identifiers here are effectively "a sort of contextual keyword". Related, we might want to update the coloring for this case in the tooltip as well.

@CyrusNajmabadi What do you think about classifying as a keyword?

Also I've been looking about the logic that handles function pointers in quick info but can't find where it's. Can you point me out? (specifically, going to definition from quick info already works if only if the compiler is using the type from corlib - where is this logic?) (I think I figured out this. The question is whether it should be classified as a keyword).

i am fine with this being classified as a class

Isn't that incorrect here? As per our conversation with Fred yesterday (who said he'd be fine with these being classified as a keyword), those are more akin to keywords than classes, given the compiler does not look the types at all. If we classified those as classes, we'd both (1) have an inconsistency in that no symbol info would be available (eg. in quick info), and (2) not actually mirror what the compiler is doing at all, as it's not actually using or looking up those types at all. According to the ECMA spec those calling conventions here are just special symbols, not classes.

Isn't that incorrect here?

This is classification. There is no concept of 'correctness' :)

According to the ECMA spec those calling conventions here are just special symbols, not classes.

That's fine. I'm still ok with it being classified in this fashion. Classification is not hte compiler. It's whatever we want it to be.

"You keep attacking me as if you think I'm saying let's do a worse solution"

It was absolutely not my intention to attack you (I mean why would I ever want to do that Cyrus?), so I'm really sorry if my messages came across that way. I was just genuinely confused about why the proposed changes were being an issue for you, is all. I do recognize that you just had a different view on the topic, and I was actively trying to understand that better. Nothing of what I said was ever meant to be an attack towards you at all, same as with all other times we happened to disagree on things before as well, of course. I do understand your point better now that you've listed those specific aspects related to complexity and maintainability 🙂

It keeps compiler and ide classification in sync.

If this point is essential, then I think the key point to get to an agreement is to first define what's the expected output of the compiler API (especially that it has effect on the public ToDisplayParts method).

You keep attacking me as of you think I'm saying let's do a worse solution

Absolutely sorry if it felt that way, I definitely didn't mean that.

Sorry, 'attack' was too strong a word. I likely wasn't making my thoughts very clear (i tend to be juggling like 15 things at at a time :)).

If this point is essential, then I think the key point to get to an agreement is to first define what's the expected output of the compiler API (especially that it has effect on the public ToDisplayParts method).

Yes, i agree with that. If the compiler goes that way, i'm ok keepign IDE in sync.

I'll leave you decide which approach is the best, just happy we could clarify the misunderstanding here ❤️

The compiler will not give a type for a single word of Stdcall, Thiscall, Cdecl, or Fastcall. Regardless of whether we decide to return symbols for other combinations of those those words or other calling conventions, we specifically do not look at symbols for those cases. They do not correspond to types and are not emitted as modreqs on the function pointer.

Youssef1313 · 2022-01-26T10:46:56Z

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs

+        public static bool IsSpecialUnmanagedCallingConvention(FunctionPointerUnmanagedCallingConventionSyntax syntax)
+        {
+            if (syntax.Parent is not FunctionPointerUnmanagedCallingConventionListSyntax list)
+            {
+                return false;
+            }
+
+            return list.CallingConventions.Count == 1 &&
+                syntax.Name.ValueText is "Cdecl" or "Stdcall" or "Thiscall" or "Fastcall";
+        }


Worth adding a comment to spec and/or relevant part of compiler implementation?

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ICompilationExtensions.cs

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs

Co-authored-by: CyrusNajmabadi <[email protected]>

…3/Roslyn into func-pointer-conv-ide

Youssef1313 · 2022-01-26T15:27:38Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

                            case SignatureCallingConvention.CDecl:
-                                builder.Add(CreatePart(SymbolDisplayPartKind.ClassName, symbol, "Cdecl"));
+                                builder.Add(CreatePart(SymbolDisplayPartKind.Keyword, symbol, "Cdecl"));
                                break;
                            case SignatureCallingConvention.StdCall:
-                                builder.Add(CreatePart(SymbolDisplayPartKind.ClassName, symbol, "Stdcall"));
+                                builder.Add(CreatePart(SymbolDisplayPartKind.Keyword, symbol, "Stdcall"));
                                break;
                            case SignatureCallingConvention.ThisCall:
-                                builder.Add(CreatePart(SymbolDisplayPartKind.ClassName, symbol, "Thiscall"));
+                                builder.Add(CreatePart(SymbolDisplayPartKind.Keyword, symbol, "Thiscall"));
                                break;
                            case SignatureCallingConvention.FastCall:
-                                builder.Add(CreatePart(SymbolDisplayPartKind.ClassName, symbol, "Fastcall"));
+                                builder.Add(CreatePart(SymbolDisplayPartKind.Keyword, symbol, "Fastcall"));


Those changes are needed for the Quick Info in IDE to show them as keywords instead of classes.

do we need to change this? why not just make the IDE side match the logic here and classify these as classnames? i don't get why we feel so strongly these need to be keywords.

@CyrusNajmabadi It was discussed on Discord and @333fred was concerned about classifying as class names when the compiler isn't actually using the class.

I feel like ClassName would be a strange choice. These are contextual keywords, right?

@JoeRobich They're not contextual keywords per se. They're "special identifiers" that I can't think of anything similar to them in the language. There is no SyntaxKind.CdeclKeyword for example.

They're "hard-coded" in the compiler:

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

Lines 147 to 154 in 7ba92d6

case { CallingConventions: { Count: 1 } specifiers }:

return specifiers[0].Name switch

{

// Special identifiers cases

{ ValueText: "Cdecl" } => CallingConvention.CDecl,

{ ValueText: "Stdcall" } => CallingConvention.Standard,

{ ValueText: "Thiscall" } => CallingConvention.ThisCall,

{ ValueText: "Fastcall" } => CallingConvention.FastCall,

To an end-user, they make look like contextual keywords since it's very similar. In fact, I think they could have been implemented as contextual keywords (but it's going to be a bit more complex implementation-wise for nothing).

CyrusNajmabadi · 2022-01-26T20:46:36Z

...p/Portable/Classification/SyntaxClassification/FunctionPointerCallingConventionClassifier.cs

+            var callingConventionSyntax = (FunctionPointerUnmanagedCallingConventionSyntax)syntax;
+            if (callingConventionSyntax.IsSpecialUnmanagedCallingConvention())
+            {
+                result.Add(new ClassifiedSpan(callingConventionSyntax.Span, ClassificationTypeNames.Keyword));
+                return;
+            }
+
+            var type = semanticModel.Compilation.UnmanagedCallingConventionType(callingConventionSyntax.Name.ValueText);
+            if (type is not null)
+            {
+                result.Add(new ClassifiedSpan(callingConventionSyntax.Name.Span, ClassificationTypeNames.ClassName));
+            }


Suggested change

var callingConventionSyntax = (FunctionPointerUnmanagedCallingConventionSyntax)syntax;

if (callingConventionSyntax.IsSpecialUnmanagedCallingConvention())

{

result.Add(new ClassifiedSpan(callingConventionSyntax.Span, ClassificationTypeNames.Keyword));

return;

}

var type = semanticModel.Compilation.UnmanagedCallingConventionType(callingConventionSyntax.Name.ValueText);

if (type is not null)

{

result.Add(new ClassifiedSpan(callingConventionSyntax.Name.Span, ClassificationTypeNames.ClassName));

}

var callingConventionSyntax = (FunctionPointerUnmanagedCallingConventionSyntax)syntax;

if (callingConventionSyntax.IsSpecialUnmanagedCallingConvention() ||

semanticModel.Compilation.UnmanagedCallingConventionType(callingConventionSyntax.Name.ValueText) != null);

{

result.Add(new ClassifiedSpan(callingConventionSyntax.Name.Span, ClassificationTypeNames.ClassName));

}

@CyrusNajmabadi This suggestion doesn't match the original intent.

I think we first need to settle on what behavior the IDE should have.

Youssef1313 · 2022-01-28T10:09:17Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

@@ -559,16 +559,16 @@ void visitFunctionPointerSignature(IMethodSymbol symbol)
                        switch (symbol.CallingConvention)
                        {
                            case SignatureCallingConvention.CDecl:
-                                builder.Add(CreatePart(SymbolDisplayPartKind.ClassName, symbol, "Cdecl"));
+                                builder.Add(CreatePart(SymbolDisplayPartKind.Keyword, symbol, "Cdecl"));


I think I should pass a null symbol for these if the team agreed they should be keywords.

@333fred @CyrusNajmabadi Please let me know how I should proceed.

i would honestly just revert the compielr change, and do everything on the IDE side. i feel like we've spent an inordinate amount of time on something absolutely trivial and this is going in circles. let's make this simple and easy:

no compiler changes. that definitely simplifies things.

make the IDE operate in line with teh compiler today. also very simple on the IDE side.

bing bang boom.

@CyrusNajmabadi Alright. Will do that. I may take few days since my next exam is in few days.

@CyrusNajmabadi I'm now always classifying as class. Can you review please? Thanks!

...p/Portable/Classification/SyntaxClassification/FunctionPointerCallingConventionClassifier.cs

CyrusNajmabadi · 2022-02-04T18:00:00Z

src/Workspaces/CSharp/Portable/Classification/ClassificationHelpers.cs

@@ -289,6 +289,10 @@ private static bool IsVerbatimStringToken(SyntaxToken token)
            {
                return ClassificationTypeNames.LabelName;
            }
+            else if (token.Parent.IsKind(SyntaxKind.FunctionPointerUnmanagedCallingConvention))
+            {
+                return ClassificationTypeNames.ClassName;


don't we want to do this if GetCallingConventionSymbol returns non-null?

e.g. if you write delegate[Gibberish] we wouldn't classify that guy :)

@CyrusNajmabadi This matches the compiler classification :)

roslyn/src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

Line 588 in eb95c2f

builder.Add(CreatePart(SymbolDisplayPartKind.ClassName, conventionType, conventionType.Name[CallConvLength..]));

Quick info relies on compiler classification too:

sharwell · 2022-02-04T23:03:58Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ICompilationExtensions.cs

+
+        public static INamedTypeSymbol? UnmanagedCallingConventionType(this Compilation compilation, string callConv)
+        {
+            var corLibrary = compilation.GetSpecialType(SpecialType.System_Object).ContainingAssembly;


❔ Is this a requirement?

@sharwell At compiler-side, yes.

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

Line 210 in eb95c2f

specifierType = compilation.Assembly.CorLibrary.LookupTopLevelMetadataType(ref metadataName, digThroughForwardedTypes: false);

Sergio0694 · 2022-11-15T10:53:25Z

Hey @CyrusNajmabadi, could you take another look at this and check out the replies to your review notes when you have some time? It would be awesome to finally resolve this and get it merged for the next release. Thank you! 😄

Sergio0694 · 2023-04-12T09:25:48Z

@CyrusNajmabadi small ping, could we get another review on this one to confirm whether all concerns were addressed?
@Youssef1313 could you update the PR branch to get it ready to go assuming all CI runs pass? Thank you! 😄

Sergio0694 · 2023-04-12T09:29:03Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ICompilationExtensions.cs

+
+        public static INamedTypeSymbol? UnmanagedCallingConventionType(this Compilation compilation, string callConv)
+        {
+            var corLibrary = compilation.GetSpecialType(SpecialType.System_Object).ContainingAssembly;


I remember @jkoritzinsky mentioning how this API could be particularly slow, so much so they tried to avoid using it entirely in the COM generators. Is that still a concern (and if so, should we consider using an alternative here), or has that been fixed now?

@Sergio0694 To my knowledge GetSpecialType is fast, and internally it should be calculated once, and then the lookup is a simple fast array index (not even a Dictionary<TKey, TValue>).

Not sure how it caused issues in generators. I'd love to know more about that.

There was a perf bug in Roslyn that made this call very slow a few months back. It's fixed now.

Youssef1313 · 2023-04-12T10:12:17Z

@CyrusNajmabadi small ping, could we get another review on this one to confirm whether all concerns were addressed? @Youssef1313 could you update the PR branch to get it ready to go assuming all CI runs pass? Thank you! 😄

I'm not even sure what is the "exact" behavior we would like. I'd let @CyrusNajmabadi drive this to completion whenever he gets time.

Sergio0694 · 2023-10-26T18:41:05Z

@CyrusNajmabadi friendly ping about this as you asked 😄

CyrusNajmabadi · 2023-10-26T19:05:58Z

pr would need to be brought up to date. moving to draft in the meantime.

Sergio0694 · 2023-10-26T19:58:25Z

@Youssef1313 would you have time to rebase/update this whenever you get a chance? 🙂

Youssef1313 · 2023-11-11T18:32:45Z

This was already fixed. I opened #70773 for fixing go to def

Classify function pointer calling convention

ab4d385

Youssef1313 requested a review from a team as a code owner January 25, 2022 19:48

dotnet-issue-labeler bot added the Area-IDE label Jan 25, 2022

ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 25, 2022

Youssef1313 marked this pull request as draft January 25, 2022 19:48

Youssef1313 added 2 commits January 25, 2022 22:26

Fix GoToDefinition for function pointer calling conventions

015ba4d

Update CSharpGoToDefinitionTests.vb

d059fdd

Youssef1313 marked this pull request as ready for review January 25, 2022 20:26

Youssef1313 changed the title ~~Classify function pointer calling convention~~ Function pointer calling convention IDE support Jan 25, 2022

CyrusNajmabadi reviewed Jan 25, 2022

View reviewed changes

...ces/CSharp/Portable/Classification/SyntaxClassification/CSharpSyntaxClassificationService.cs Outdated Show resolved Hide resolved

333fred reviewed Jan 25, 2022

View reviewed changes

...p/Portable/Classification/SyntaxClassification/FunctionPointerCallingConventionClassifier.cs Outdated Show resolved Hide resolved

Youssef1313 added 3 commits January 26, 2022 01:03

Adjust logic

5d75a06

Adjust classifier logic

17729b6

Refactor

401f294

jinujoseph assigned CyrusNajmabadi Jan 25, 2022

Sergio0694 reviewed Jan 25, 2022

View reviewed changes

Youssef1313 added 3 commits January 26, 2022 01:54

Update CSharpSyntaxFacts.cs

aa40134

Update CSharpGoToDefinitionTests.vb

dc04f24

Make static

a4c777b

Youssef1313 requested a review from CyrusNajmabadi January 26, 2022 09:54

Youssef1313 commented Jan 26, 2022

View reviewed changes

Youssef1313 added 3 commits January 26, 2022 15:08

Use Net6 in tests

bbe7a76

Fix error

c81035c

Add non-corlib test

ac49884

CyrusNajmabadi reviewed Jan 26, 2022

View reviewed changes

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ICompilationExtensions.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jan 26, 2022

View reviewed changes

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jan 26, 2022

View reviewed changes

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

c82742f

Co-authored-by: CyrusNajmabadi <[email protected]>

Adjust symbol display

d8cce8c

Youssef1313 requested review from 333fred, CyrusNajmabadi and Sergio0694 January 26, 2022 15:24

Merge branch 'func-pointer-conv-ide' of https://github.com/Youssef131…

191d071

…3/Roslyn into func-pointer-conv-ide

Youssef1313 requested a review from a team as a code owner January 26, 2022 15:26

Youssef1313 commented Jan 26, 2022

View reviewed changes

Update TotalClassifierTests.cs

a7ae4ee

CyrusNajmabadi reviewed Jan 26, 2022

View reviewed changes

Youssef1313 commented Jan 28, 2022

View reviewed changes

Address feedback

fa85916

Youssef1313 commented Feb 4, 2022

View reviewed changes

...p/Portable/Classification/SyntaxClassification/FunctionPointerCallingConventionClassifier.cs Outdated Show resolved Hide resolved

Youssef1313 added 2 commits February 4, 2022 15:29

Simplify

d0ded00

Update CSharpGoToDefinitionTests.vb

514665e

CyrusNajmabadi reviewed Feb 4, 2022

View reviewed changes

runfoapp bot mentioned this pull request Feb 4, 2022

[Flaky Test] VisualBasic.BasicAddMissingReference.InvokeSomeFixesInVisualBasicThenVerifyReferences #57980

Closed

sharwell reviewed Feb 4, 2022

View reviewed changes

Update SyntacticClassifierTests.cs

0814107

Sergio0694 reviewed Apr 12, 2023

View reviewed changes

CyrusNajmabadi marked this pull request as draft October 26, 2023 19:06

Youssef1313 closed this Nov 11, 2023

Youssef1313 deleted the func-pointer-conv-ide branch November 11, 2023 18:32

	case { CallingConventions: { Count: 1 } specifiers }:
	return specifiers[0].Name switch
	{
	// Special identifiers cases
	{ ValueText: "Cdecl" } => CallingConvention.CDecl,
	{ ValueText: "Stdcall" } => CallingConvention.Standard,
	{ ValueText: "Thiscall" } => CallingConvention.ThisCall,
	{ ValueText: "Fastcall" } => CallingConvention.FastCall,

Function pointer calling convention IDE support #59062

Function pointer calling convention IDE support #59062

Conversation

Youssef1313 commented Jan 25, 2022 • edited Loading

Sergio0694 Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Youssef1313 Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 commented Nov 15, 2022

Sergio0694 commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 commented Apr 12, 2023

Sergio0694 commented Oct 26, 2023

CyrusNajmabadi commented Oct 26, 2023

Sergio0694 commented Oct 26, 2023

Youssef1313 commented Nov 11, 2023

Youssef1313 commented Jan 25, 2022 •

edited

Loading

Sergio0694 Jan 25, 2022 •

edited

Loading

Youssef1313 Jan 25, 2022 •

edited

Loading

333fred Jan 28, 2022 •

edited

Loading