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.

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

Classify nameof<'T> & match … with nameof ident -> … correctly #18300

Merged
merged 7 commits into from
Feb 12, 2025

Conversation

brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Feb 8, 2025

Description

Fixes #10026.

  • Record the name resolution of the nameof identifier in type application expressions and patterns to enable correct colorization:
    • Resolve nameof in nameof<'T>.
    • Resolve nameof in match … with nameof ident -> ….

Before

image

After

image

Checklist

  • Test cases added.
  • Release notes entry updated.

Copy link
Contributor

github-actions bot commented Feb 8, 2025

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.300.md

@brianrourkeboll brianrourkeboll changed the title Classify nameof<'T>, match … with nameof ident -> …, correctly Classify nameof<'T> & match … with nameof ident -> … correctly Feb 8, 2025
@brianrourkeboll brianrourkeboll marked this pull request as ready for review February 8, 2025 20:13
@brianrourkeboll brianrourkeboll requested a review from a team as a code owner February 8, 2025 20:13
@edgarfgp
Copy link
Contributor

edgarfgp commented Feb 9, 2025

Thanks for this Brian. Will this also cover the cases match ... with | ... -> nameof(..) function | ... -> nameof(...) ?

@brianrourkeboll
Copy link
Contributor Author

Thanks for this Brian. Will this also cover the cases match ... with | ... -> nameof(..) function | ... -> nameof(...) ?

In the result expression of match clauses? It's already colorized correctly there, no?

This PR covers (1) generic nameof<'T> in expressions and (2) nameof ident in patterns (whether in a match clause or anywhere else).

@edgarfgp
Copy link
Contributor

edgarfgp commented Feb 9, 2025

Thanks for this Brian. Will this also cover the cases match ... with | ... -> nameof(..) function | ... -> nameof(...) ?

In the result expression of match clauses? It's already colorized correctly there, no?

This PR covers (1) generic nameof<'T> in expressions and (2) nameof ident in patterns (whether in a match clause or anywhere else).

In Rider currently looks like this

Screenshot 2025-02-09 at 18 59 49

@brianrourkeboll
Copy link
Contributor Author

brianrourkeboll commented Feb 9, 2025

In Rider currently looks like this

Screenshot 2025-02-09 at 18 59 49

I don't have Rider installed on this machine, but what do other "intrinsic functions" (raise, reraise, typeof, typedefof, sizeof) look like in that position?

What does nameof look like in

let f x = nameof x

?

FCS classifies nameof as SemanticClassificationType.IntrinsicFunction (assuming the name resolution has been recorded properly):

let (|KeywordIntrinsicValue|_|) (vref: ValRef) =
if
valRefEq g g.raise_vref vref
|| valRefEq g g.reraise_vref vref
|| valRefEq g g.typeof_vref vref
|| valRefEq g g.typedefof_vref vref
|| valRefEq g g.sizeof_vref vref
|| valRefEq g g.nameof_vref vref
then
Some()
else
None

| Item.Value KeywordIntrinsicValue, ItemOccurrence.Use, m -> add m SemanticClassificationType.IntrinsicFunction

The VS integration maps that to its own, separate classification, in the same category as keywords:

| SemanticClassificationType.IntrinsicFunction -> ClassificationTypeNames.Keyword

It looks like Rider doesn't use the FCS API here; it must be doing its own classification: https://github.com/search?q=repo%3AJetBrains%2Fresharper-fsharp+SemanticClassificationType&type=code

Screenshot 2025-02-09 at 18 59 49

My guess is that this PR will at least make the nameofs on the left-hand side of the -> have the same color as the nameofs on the right-hand side in Rider, though. (And in VS Code, since Ionide also does not color intrinsic functions as keywords.)

Copy link
Member

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

@brianrourkeboll Cool, thanks! Could you look at these two things, please? 🙂

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Nice. Thank you @brianrourkeboll :)

@psfinaki psfinaki merged commit 79e8531 into dotnet:main Feb 12, 2025
33 checks passed
@brianrourkeboll brianrourkeboll deleted the nameof-classification branch February 12, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Nameof is not classified in generic form or pattern form
5 participants