Inline rename: symbol duplicated when it's a generic type parameter

[cartermp]

type Record = { IntVal: int; StringVal: string }

[<AbstractClass>]
type AbstractFoo<'T>() =
    abstract member Foo: 'T -> 'T

type C() =
    inherit AbstractFoo<Record>()
    override __.Foo(r) = { r with IntVal = 12 }

Rename Record at either the definition or use site:

Info:

• .NET Framework 4.6.1 project
• VS 15.6 Update 2
• VF# tools 15.4.1.17112103

[cartermp]

Reduced repo:

[<AbstractClass>]
type AbstractFoo<'T>() =
    abstract member Foo: 'T -> 'T

type C() =
    inherit AbstractFoo<int>()
    override __.Foo(i) = i

0. Ensure you're debugging and set a breakpoint here: https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/NameResolution.fs#L1465
1. Click int in the above code.

Expected:

Single symbol use for int, which should be a UseInType Occurage.

Actual:

Three symbol uses for int:

1. UseInType:
   

2. UseInType:
   

3. Use:
   

This surfaces in inline rename in the original issue text because the code there just finds the spans for each symbol use's location and replaces the text. Since the inline rename service finds multiple symbols for the same thing, it has multiple spans to apply the change to, hence the bug.

@dsyme The capturedNameResolutions that are used to find symbols are filled when the document is typechecked - as far as I can tell, code to find the symbols has no bug in it. Is it correct behavior to find what amounts to duplicate symbols for the same thing? If not, then this might actually be a bug in the typechecker.

[dsyme]

@dsyme The capturedNameResolutions that are used to find symbols are filled when the document is typechecked - as far as I can tell, code to find the symbols has no bug in it. Is it correct behavior to find what amounts to duplicate symbols for the same thing? If not, then this might actually be a bug in the typechecker.

There can be duplicates in this list - there are some phases of checking we run multiple types, especially related to type parameter declarations, and it's hard to prevent duplicates creeping in. IIRC there is code later to remove duplicates - it's likely that this isn't dealing with type parameters

[cartermp]

Makes sense. Yes, currently there doesn't seem to be anything in FCS which will filter out "duplicate" symbols (either that or I haven't looked hard enough). There is code in Inline Rename which will distinct on symbols, but because in this case they are distinct symbols, they're not filtered out.

What I think we're after here is:

• Get all symbols based on the predicate
• Distinct down
• "Distinct" down again, but for symbols which are distinct but still represent the same line of code (e.g., a symbol Record with UseInType and a symbol for Record with Use, when both are in the same span, indicates a duplicate)

What I'm still a bit confused by is why we'd report multiple symbols for the same span.

[dsyme]

I think I was thinking of RemoveDuplicateItems in service.fs

[dsyme]

I think I was thinking of RemoveDuplicateItems in service.fs. But that's for autocomplete, not symbol usage.

There's ItemsAreEffectivelyEqual in NameResolution.fs, which is made public via IsEffectivelySameAs. But we don't make GetAllUsesOfAllSymbols distinct by this. I'm a bit unsure of the perf characteristics of ItemsAreEffectivelyEqual and GetAllUsesOfAllSymbols is a bit of a hot path.

Just adding code to selectively de-duplicate type parameters would probably work

[cartermp]

This is fixed in #4655