Report auto property redux

[nojaf]

Property symbol, key to data's gate,
In code, it holds a crucial state.
Accessors grant its value's grace,
In F# land, it finds its place.

After #15589, the symbols produced for auto-properties and get/set properties changed. An actual Item.Property is reported to the sink and contains both the get and set symbol. The ranges of the get and set were modified so that only one symbol is attached to the property's name range.

For example:

type X() =
    member val Y = 1 with get, set

Produced a Property symbol for Y with two nested symbols for get and set.
This in theory held but turned out to be problematic for #16056.

In the bigger picture, this area is quite complex. Properties, in any form, present a host of challenges. From syntax to symbols, numerous edge cases make it hard to consistently make the right choices.

Taking a step back, I originally intended to know if a symbol was indeed a property or not. In the case of Y, I would get either the get or the set member as they were not both stored. (See #15212 and #15213)

That helped, but now you could get multiple symbols for the same range.
#15285 helped to address that.
Again, not a perfect solution, but at the very least you now were able to access both symbols.

In #15589, I tried to unify both members into a property and have a single symbol. Alas, this is not ideal for all the workarounds editors have to do to deal with properties. So in this PR, I'm trying to report the property as an additional symbol and keep using the name range (Y) for the members.

GetSymbolUsesAtLocation would now return three symbols and that is the best trade-off scenario I can come up with right now.

In 43b6ac8, I'm reporting this late in the game during post-inference. I don't think this is quite ideal as I need to add multiple (questionable) dependencies to call the sink.

In 34fe1e4, I'm reporting the property after the desugaring of the members. Which is where it currently happens in the main branch.

You might be asking yourself, why do you really need the property symbol? The presence of the two members with the same range strongly hints at a property as well right? Well, the property symbol has an accurate implementation for the GetValSignatureText. Which is relying on information that is internal to FCS.

I have not updated all the tests yet, as I would like to discuss further what we should do with this.
Please chime in and let me know what would make the most sense to you.

[auduchinok]

@nojaf This is really cool! Could you please check that the properties are reported when there're critical errors? Some of the post-inference stages don't run in such cases.

let _ = UnresolvedName

type X() =
    member val Y = 1 with get, set

type X() =
    member val Y = UnresolvedName with get, set

Another interesting case to check it for interface implementations:

type I =
    abstract P: int with get, set

type T() =
    interface I with
        member val P = 1 with get, set

[vzarytovskii]

GetSymbolUsesAtLocation would now return three symbols and that is the best trade-off scenario I can come up with right now.

That might affect finding references (and maybe definition), renaming in vs. Could you please check if it's still working?

I.e. renaming will rename one symbol, definition and references returns expected number of results?

[nojaf]

I may not have asked this explicitly enough, but I would like to know what the optimal choice is between:

• 43b6ac8, I'm reporting this late in the game during post-inference. I don't think this is quite ideal as I need to add multiple (questionable) dependencies to call the sink.

• 34fe1e4, I'm reporting the property after the desugaring of the members. Which is where it currently happens in the main branch.

Before digging any deeper into this.

[psfinaki]

FYI we had a session on this today - decided to go with the current approach (adjusting CheckDeclarations). It's not the ideal way but it makes the situation better without compromising code design.

Once the CI is green, I can smoke test the IDE side of things.

[nojaf]

Hi @brianrourkeboll,

I broke one of your recent tests:

  Failed FSharp.Editor.Tests.CodeFixes.RemoveUnnecessaryParenthesesTests+Expressions.Basic expressions(expr: "\r\n            (match x with\r\n             | 1 "..., expected: "\r\n            match x with\r\n            | 1 ->"...) [9 ms]
  Error Message:
   FSharp.Editor.Tests.CodeFixes.CodeFixTestFramework+Xunit+MissingCodeFixException : MissingCodeFixException
  ("Expected a code fix but did not get one.",
   Xunit.Sdk.AllException: Assert.All() Failure: 3 out of 7 items in the collection did not pass.
[4]: Item: (            match x with,             (match x with)
     Xunit.Sdk.EqualException: Assert.Equal() Failure
                           ↓ (pos 12)
     Expected:             match x with
     Actual:               (match x with
                           ↑ (pos 12)
        at Xunit.Assert.Equal(String expected, String actual, Boolean ignoreCase, Boolean ignoreLineEndingDifferences, Boolean ignoreWhiteSpaceDifferences) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\StringAsserts.cs:line 244
        at FSharp.Editor.Tests.CodeFixes.CodeFixTestFramework.Xunit.shouldEqual@212.Invoke(Tuple`2 tupledArg) in D:\a\_work\1\s\vsintegration\tests\FSharp.Editor.Tests\CodeFixes\CodeFixTestFramework.fs:line 212
        at Xunit.Assert.All[T](IEnumerable`1 collection, Action`1 action) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\CollectionAsserts.cs:line 36
[3]: Item: (            | 1 -> (),              | 1 -> ())
     Xunit.Sdk.EqualException: Assert.Equal() Failure
                           ↓ (pos 12)
     Expected:             | 1 -> ()
     Actual:                | 1 -> ()
                           ↑ (pos 12)
        at Xunit.Assert.Equal(String expected, String actual, Boolean ignoreCase, Boolean ignoreLineEndingDifferences, Boolean ignoreWhiteSpaceDifferences) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\StringAsserts.cs:line 244
        at FSharp.Editor.Tests.CodeFixes.CodeFixTestFramework.Xunit.shouldEqual@212.Invoke(Tuple`2 tupledArg) in D:\a\_work\1\s\vsintegration\tests\FSharp.Editor.Tests\CodeFixes\CodeFixTestFramework.fs:line 212
        at Xunit.Assert.All[T](IEnumerable`1 collection, Action`1 action) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\CollectionAsserts.cs:line 36
[2]: Item: (            | _ -> (),              | _ -> ()))
     Xunit.Sdk.EqualException: Assert.Equal() Failure
                           ↓ (pos 12)
     Expected:             | _ -> ()
     Actual:                | _ -> ())
                           ↑ (pos 12)
        at Xunit.Assert.Equal(String expected, String actual, Boolean ignoreCase, Boolean ignoreLineEndingDifferences, Boolean ignoreWhiteSpaceDifferences) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\StringAsserts.cs:line 244
        at FSharp.Editor.Tests.CodeFixes.CodeFixTestFramework.Xunit.shouldEqual@212.Invoke(Tuple`2 tupledArg) in D:\a\_work\1\s\vsintegration\tests\FSharp.Editor.Tests\CodeFixes\CodeFixTestFramework.fs:line 212
        at Xunit.Assert.All[T](IEnumerable`1 collection, Action`1 action) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\CollectionAsserts.cs:line 36
   at Xunit.Assert.All[T](IEnumerable`1 collection, Action`1 action) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\CollectionAsserts.cs:line 45
   at FSharp.Editor.Tests.CodeFixes.CodeFixTestFramework.Xunit.shouldEqual(String expected, String actual) in D:\a\_work\1\s\vsintegration\tests\FSharp.Editor.Tests\CodeFixes\CodeFixTestFramework.fs:line 214
   at Xunit.Assert.RecordException(Action testCode) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\Record.cs:line 28)
  Stack Trace:
     at FSharp.Editor.Tests.CodeFixes.CodeFixTestFramework.Xunit.expectFix@254-4.Invoke(ResumableStateMachine`1& sm) in D:\a\_work\1\s\vsintegration\tests\FSharp.Editor.Tests\CodeFixes\CodeFixTestFramework.fs:line 257
   at FSharp.Editor.Tests.CodeFixes.CodeFixTestFramework.Xunit.expectFix@253-17.MoveNext(ResumableStateMachine`1& sm)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()

I'm not quite sure why this happens. Do you see any changes in the syntax tree that might explain this?

[brianrourkeboll]

@nojaf Nope, not you! Fixed in #16285. This is what happened: #16284 (comment)

[nojaf]

Pointed out the most relevant changes.

[edgarfgp]

This is fantastic work Florian. This uses an accurate range a could not get right for Indexed Properties #16023. Thanks

[psfinaki]

LGTM.
@dotnet/fsharp-team-msft please run your eyes on this.

[vzarytovskii]

GetSymbolUsesAtLocation would now return three symbols and that is the best trade-off scenario I can come up with right now.

That might affect finding references (and maybe definition), renaming in vs. Could you please check if it's still working?

I.e. renaming will rename one symbol, definition and references returns expected number of results?

This needs to be checked before merging. With explicit getters, setters, etc.

[nojaf]

This needs to be checked before merging. With explicit getters, setters, etc.

So, I checked the latest VS release:

and it isn't working for me even before the changes of this PR.

Then I tried using commit b645acea5 (which is before my original PR #15589), and it is not working there either.

As I'm not really knowledgeable in the VS tooling side of things and there is evidence this never worked properly, I don't think this should be a requirement to merge this PR.

I'd be happy to jump on a call with @psfinaki (once he is back) to follow up on this.

[vzarytovskii]

As I'm not really knowledgeable in the VS tooling side of things and there is evidence this never worked properly, I don't think this should be a requirement to merge this PR.

Hm, alright, it was broken earlier.

[vzarytovskii]

Should auto-merge as soon as all comments are resolved.

[nojaf]

Thanks Vlad!

[vzarytovskii]

Thanks Vlad!

Thank you! I would love to discuss graph TC enablement and an opt-out for it via fsproj sometime.