Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Function pointer calling convention IDE support #59062
Function pointer calling convention IDE support #59062
Changes from 6 commits
ab4d385
015ba4d
d059fdd
5d75a06
17729b6
401f294
aa40134
dc04f24
a4c777b
bbe7a76
c81035c
ac49884
c82742f
473e16a
313b53b
d8cce8c
191d071
a7ae4ee
fa85916
d0ded00
514665e
0814107
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
@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).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 am fine with this being classified as a class
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.
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.
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.
This is classification. There is no concept of 'correctness' :)
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.
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 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 🙂
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.
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).Absolutely sorry if it felt that way, I definitely didn't mean that.
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.
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 :)).
Yes, i agree with that. If the compiler goes that way, i'm ok keepign IDE in sync.
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'll leave you decide which approach is the best, just happy we could clarify the misunderstanding here ❤️
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.
The compiler will not give a type for a single word of
Stdcall
,Thiscall
,Cdecl
, orFastcall
. 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.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 a requirement?
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.
@sharwell At compiler-side, yes.
roslyn/src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerMethodSymbol.cs
Line 210 in eb95c2f
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 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?
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.
@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 problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a perf bug in Roslyn that made this call very slow a few months back. It's fixed now.