Report auto property by nojaf · Pull Request #15589 · dotnet/fsharp

nojaf · 2023-07-12T14:55:17Z

Related to #15586 and tries to address fsharp/fsharp-compiler-docs#79.

As mentioned in #15586, the SynMemberDefn.AutoProperty will be split into three parts when both get,set are present.

Consider the following example:

typeX()=member valY=1with get,set

This will generate two bindings: get_Y and set_Y.
Both will currently use the Y range for the Item in NameResolution.
When you later ask for the symbol use of Y, you now get the get_Y from checkResults.GetSymbolUseAtLocation.

There is no clever symbol detection going on here,

https://github.com/dotnet/fsharp/blob/3570d5db878dcdace5c984bbca810a2bfa4ca4b4/src/Compiler/Service/FSharpCheckerResults.fs#L2825-L2827

it just returns the first value. Where actually two symbols were found.

In this PR, when both get,set are present, the respective get_Y and set_Y members will now use the keyword (get or set) range for the NameResolution Item.

And a new property Item will be reported for the range of Y. That property will contain both the Getter and Setter:

letmfv= checkResults.GetSymbolUsesAtLocation(...,["Y"]).Symbol :?> FSharpMemberOrFunctionOrValue
Assert.True mfv.IsProperty
Assert.True mfv.HasGetterMethod
Assert.True mfv.HasSetterMethod
Assert.True (mfv.GetterMethod.CompiledName.StartsWith("get_"))
Assert.True (mfv.SetterMethod.CompiledName.StartsWith("set_"))

The same approach is possible for SynMemberDefn.GetSetMember where we can produce a Item.Property if both members are present.

The result of this change is that checkResults.GetSymbolUseAtLocation will report a single symbol for the property name and not just return the getter. And this solves the original problem with the tooltip. And it also has a positive effect on the new GetValSignatureText API.

auduchinok · 2023-07-13T14:59:38Z

@nojaf Is there a chance that this change could also help to improve things for non-auto properties too? 😇

typeT1()=memberx.P=0memberx.Pwith set (i:int)=()

typeT2()=memberthis.Pwith get _=1andset _ _ =()

auduchinok · 2023-07-13T15:01:33Z

There's also seems to be a crazy way to mix auto properties with normal ones:

typeT3()=member valP=1memberthis.Pwith set (i:int)=()

nojaf · 2023-07-13T15:14:58Z

Is there a chance that this change could also help to improve things for non-auto properties too?

Not with the current changes but I hope to use the same mechanism for SynMemberDefn.GetSetProperty.
I'll take a look at it, hopefully, the same trick could work as well there.

There's also seems to be a crazy way to mix auto properties with normal ones:

Because of course, why not 🥳🙈. I don't think my current change would impact that.
I do think it is correct to have two symbols in this case.

auduchinok · 2023-07-13T15:35:24Z

I do think it is correct to have two symbols in this case.

Apparently, it's considered to be a single property in the signature and compiled code. 😞

It looks like the setter from the second declaration is added to the auto property. Ideally, the same property should be reported at both P identifier ranges, and accessors should be reported if defined explicitly via get/set at their ranges.

nojaf · 2023-07-17T09:07:21Z

Ready for review.

Co-authored-by: dawe <dawedawe@posteo.de>

vzarytovskii · 2023-07-20T15:18:42Z

 FSharp.Compiler.Syntax.SynUnionCaseKind: Int32 get_Tag()
 FSharp.Compiler.Syntax.SynUnionCaseKind: System.String ToString()
-FSharp.Compiler.Syntax.SynValData: FSharp.Compiler.Syntax.SynValData NewSynValData(Microsoft.FSharp.Core.FSharpOption`1[FSharp.Compiler.Syntax.SynMemberFlags], FSharp.Compiler.Syntax.SynValInfo, Microsoft.FSharp.Core.FSharpOption`1[FSharp.Compiler.Syntax.Ident])
+FSharp.Compiler.Syntax.SynValData: FSharp.Compiler.Syntax.SynValData NewSynValData(Microsoft.FSharp.Core.FSharpOption`1[FSharp.Compiler.Syntax.SynMemberFlags], FSharp.Compiler.Syntax.SynValInfo, Microsoft.FSharp.Core.FSharpOption`1[FSharp.Compiler.Syntax.Ident], Microsoft.FSharp.Core.FSharpOption`1[FSharp.Compiler.Syntax.Ident])


This is a breaking change, right?

Strictly speaking yes, but every FCS release has these.
You could say the same thing about the recent changes in https://github.com/dotnet/fsharp/commits/main/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.release.bsl

vzarytovskii

This one in particular is a bit more scary to me.

@0101 @auduchinok @T-Gro please take a look

vzarytovskii

This one in particular is a bit more scary to me.

@0101 @auduchinok @T-Gro please take a look

psfinaki

Left a few remarks, mostly minor things and some learning opportunities for me.

Thanks a lot!

psfinaki · 2023-07-20T15:33:49Z

+ match mGetSetOpt with
+ | Some (GetSetKeywords.GetSet(set = mSet)) -> Ident(id.idText, mSet)
+ | _ -> id
+ if isStatic then [id] else [ident ("__", mMemberPortion);id]


On a high level, how do things bubble up if it's static actually?

psfinaki · 2023-07-20T15:35:54Z

@@ -11418,8 +11419,7 @@ and AnalyzeRecursiveInstanceMemberDecl
 // the definition of these symbols.
 //
 // See https://github.com/fsharp/FSharp.Compiler.Service/issues/79.


I guess the comments above can go now?

psfinaki · 2023-07-20T15:51:02Z

+ | DifferentGetterAndSetter(getValRef, setValRef) ->
+ let g = NicePrint.stringValOrMember displayEnv cenv.infoReader getValRef
+ let s = NicePrint.stringValOrMember displayEnv cenv.infoReader setValRef
+ $"{g}\n{s}"


Don't know if we have any consistent approach on that but maybe consider using Environment.NewLine

psfinaki · 2023-07-20T15:52:34Z

+ if p.HasGetter && p.HasSetter then "with get, set"
+ elif p.HasGetter then "with get"
+ elif p.HasSetter then "with set"
+ else ""


I guess this shouldn't happen?

No, that is unreachable code indeed.

psfinaki · 2023-07-20T15:55:51Z


 [<Test>]
- let ``AutoProperty with get,set has two symbols`` () =
+ let ``AutoProperty with get,set has a single symbol!`` () =


Like the exclamation mark - feels like the emotional core of the PR :)

nojaf mentioned this pull request Jul 12, 2023
Detect get/set combination for GetStructuredToolTipText. #15586
Merged

nojaf force-pushed the report-auto-property branch from 8b11472 to 1c40060Compare July 13, 2023 14:52

nojaf marked this pull request as ready for review July 17, 2023 09:07

nojaf requested a review from a team as a code owner July 17, 2023 09:07

nojaf added 16 commits July 17, 2023 14:20

WIP: report auto property
4ade24f

Update unit test to reflect the new world order.
d06d45b

Revert short term tooltip solution.
1fbadf2

Revert initial approach.
4f0c2b5

Keep track of transformed auto properties in SynValData.
6783496

Report single property item for regular property.
91771b9

Fix existing tests
fa650c5

Only add meta data when auto property has both get and set.
fc2f065

Update SynValInfo in syntax tree tests.
3dd2b64

Use correct range of property if available.
80f3b65

Update surface area.
f67b707

Update existing tests.
89a1a7a

Correct GetValSignatureText for Properties
321a4e9

Update old tests
c734136

Remove test.fsi
a37065d

Update VS test
81b0fc6

nojaf force-pushed the report-auto-property branch from 5fec4c0 to 81b0fc6Compare July 17, 2023 12:20

dawedaweand others added 2 commits July 19, 2023 10:24

Merge branch 'main' into report-auto-property
50cd92d

Update syntax tree tests
216766e

dawedawe reviewed Jul 19, 2023
View reviewed changes

Comment threadsrc/Compiler/SyntaxTree/SyntaxTree.fsi Outdated

dawedawe approved these changes Jul 19, 2023
View reviewed changes

nojafand others added 3 commits July 19, 2023 13:07

Update src/Compiler/SyntaxTree/SyntaxTree.fsi
662c45c
Co-authored-by: dawe <dawedawe@posteo.de>

Merge branch 'main' into report-auto-property
00827bd

Remove reverted tests
3f7d14e

vzarytovskii reviewed Jul 20, 2023
View reviewed changes

vzarytovskii approved these changes Jul 20, 2023
View reviewed changes

Merge branch 'main' into report-auto-property
75adadd

psfinaki approved these changes Jul 20, 2023
View reviewed changes

vzarytovskii merged commit 1e78d39 into dotnet:mainJul 20, 2023

allisonchou mentioned this pull request Jul 21, 2023
[Automated] PRs inserted in VS build main-33921.120 #15661
Closed

auduchinok mentioned this pull request Oct 9, 2023
FSharpMemberOrFunctionOrValue.DeclarationLocation returns wrong location for auto property #16056
Closed

nojaf mentioned this pull request Oct 12, 2023
Report auto property redux #16116
Merged

github-actionsBot mentioned this pull request Mar 31, 2026
Add regression test: #16056, auto property DeclarationLocation returns correct range #19533
Merged

Uh oh!

Conversation

nojaf commented Jul 12, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auduchinok commented Jul 13, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auduchinok commented Jul 13, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojaf commented Jul 13, 2023

Uh oh!

auduchinok commented Jul 13, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojaf commented Jul 17, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vzarytovskii left a comment

Choose a reason for hiding this comment

Uh oh!

vzarytovskii left a comment

Choose a reason for hiding this comment

Uh oh!

psfinaki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nojaf commented Jul 12, 2023•
edited
Loading

auduchinok commented Jul 13, 2023•
edited
Loading

auduchinok commented Jul 13, 2023•
edited
Loading

auduchinok commented Jul 13, 2023•
edited
Loading