Skip to content

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

Merged
psfinaki merged 7 commits into
dotnet:mainfrom
brianrourkeboll:nameof-classification
Feb 12, 2025
Merged

Classify nameof<'T> & match … with nameof ident -> … correctly#18300
psfinaki merged 7 commits into
dotnet:mainfrom
brianrourkeboll:nameof-classification

Conversation

@brianrourkeboll

@brianrourkebollbrianrourkeboll commented Feb 8, 2025

Copy link
Copy Markdown
Contributor

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.

@github-actions

github-actionsBot commented Feb 8, 2025

Copy link
Copy Markdown
Contributor

❗ Release notes required


✅ Found changes and release notes in following paths:

Change pathRelease notes pathDescription
src/Compilerdocs/release-notes/.FSharp.Compiler.Service/9.0.300.md

@brianrourkebollbrianrourkeboll changed the title Classify nameof<'T>, match … with nameof ident -> …, correctlyClassify nameof<'T> & match … with nameof ident -> … correctlyFeb 8, 2025
Comment threadsrc/Compiler/Checking/Expressions/CheckExpressions.fs Outdated
@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 ownerFebruary 8, 2025 20:13
@edgarfgp

Copy link
Copy Markdown
Contributor

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

@brianrourkeboll

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

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

brianrourkeboll commented Feb 9, 2025

Copy link
Copy Markdown
ContributorAuthor

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

letf 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.)

@auduchinokauduchinok left a comment

Copy link
Copy Markdown
Member

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? 🙂

Comment threadsrc/Compiler/Checking/CheckPatterns.fs

@psfinakipsfinaki left a comment

Copy link
Copy Markdown
Contributor

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:mainFeb 12, 2025
@brianrourkeboll
brianrourkeboll deleted the nameof-classification branch February 12, 2025 14:14
Sign up for freeto 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

@brianrourkeboll@edgarfgp@auduchinok@psfinaki@T-Gro