Skip to content

Conversation

@nojaf
Copy link
Contributor

@nojaf nojaf commented Jul 12, 2023

Fixes #15584.
Due to #15213, there are now two symbols for auto properties (which I still think is correct).
This leads to the double tooltip, as a first fix I'm only showing the tooltip for the getter.
Which was the original behaviour.
Ideally, this would show with get, set but that proves to be a bit more challenging.

@nojaf nojaf requested a review from a team as a code owner July 12, 2023 08:59
@auduchinok
Copy link
Member

auduchinok commented Jul 12, 2023

Due to #15213, there are now two symbols for auto properties (which I still think is correct).

I think it's not. There should be a single symbol for the property on the identifier range. The property symbol should then return the separate accessors symbols in GetterMethod and SetterMethod properties when applicable. It may be a bit tricky, since it may require some rewriting of the symbol logic for properties.

In addition to the property symbol, there should/may be accessor symbols reported at their declaration ranges.

We have another regression in tests showing exactly this case, where the additional setter symbol breaks things for us, and I was going to report it today as well. 🙂

@nojaf
Copy link
Contributor Author

nojaf commented Jul 12, 2023

Yeah, a single symbol that captures both get and set would indeed be the cleanest.
In my defence, my previous PR only highlighted an existing problem:

  • Two symbols were always created.
  • The setter was always ignored due to having the same name as the getter.

I did some investigation and it gets complicated after:

// Convert auto properties to let bindings in the pre-list
let rec preAutoProps memb =
match memb with
| SynMemberDefn.AutoProperty(ident = id) when id.idText = "" -> []
| SynMemberDefn.AutoProperty(attributes=Attributes attribs; isStatic=isStatic; ident=id; typeOpt=tyOpt; propKind=propKind; xmlDoc=xmlDoc; synExpr=synExpr; range=mWholeAutoProp) ->
// Only the keep the field-targeted attributes
let attribs = attribs |> List.filter (fun a -> match a.Target with Some t when t.idText = "field" -> true | _ -> false)
let mLetPortion = synExpr.Range
let fldId = ident (CompilerGeneratedName id.idText, mLetPortion)
let headPat = SynPat.LongIdent (SynLongIdent([fldId], [], [None]), None, Some noInferredTypars, SynArgPats.Pats [], None, mLetPortion)
let retInfo = match tyOpt with None -> None | Some ty -> Some (None, SynReturnInfo((ty, SynInfo.unnamedRetVal), ty.Range))
let isMutable =
match propKind with
| SynMemberKind.PropertySet
| SynMemberKind.PropertyGetSet -> true
| _ -> false
let attribs = mkAttributeList attribs mWholeAutoProp
let binding = mkSynBinding (xmlDoc, headPat) (None, false, isMutable, mLetPortion, DebugPointAtBinding.NoneAtInvisible, retInfo, synExpr, synExpr.Range, [], attribs, None, SynBindingTrivia.Zero)
[(SynMemberDefn.LetBindings ([binding], isStatic, false, mWholeAutoProp))]
| SynMemberDefn.Interface (members=Some membs) -> membs |> List.collect preAutoProps
| SynMemberDefn.LetBindings _
| SynMemberDefn.ImplicitCtor _
| SynMemberDefn.Open _
| SynMemberDefn.ImplicitInherit _ -> [memb]
| _ -> []
// Convert auto properties to member bindings in the post-list
let rec postAutoProps memb =
match memb with
| SynMemberDefn.AutoProperty(ident = id) when id.idText = "" -> []
| SynMemberDefn.AutoProperty(attributes=Attributes attribs; isStatic=isStatic; ident=id; typeOpt=tyOpt; propKind=propKind; memberFlags=memberFlags; memberFlagsForSet=memberFlagsForSet; xmlDoc=xmlDoc; accessibility=access; trivia = { GetSetKeywords = mGetSetOpt }) ->
let mMemberPortion = id.idRange
// Only the keep the non-field-targeted attributes
let attribs = attribs |> List.filter (fun a -> match a.Target with Some t when t.idText = "field" -> false | _ -> true)
let fldId = ident (CompilerGeneratedName id.idText, mMemberPortion)
let headPatIds = if isStatic then [id] else [ident ("__", mMemberPortion);id]
let headPat = SynPat.LongIdent (SynLongIdent(headPatIds, [], List.replicate headPatIds.Length None), None, Some noInferredTypars, SynArgPats.Pats [], None, mMemberPortion)
let memberFlags = { memberFlags with GetterOrSetterIsCompilerGenerated = true }
let memberFlagsForSet = { memberFlagsForSet with GetterOrSetterIsCompilerGenerated = true }
match propKind, mGetSetOpt with
| SynMemberKind.PropertySet, Some gs -> errorR(Error(FSComp.SR.parsMutableOnAutoPropertyShouldBeGetSetNotJustSet(), gs.Range))
| _ -> ()
[
match propKind with
| SynMemberKind.Member
| SynMemberKind.PropertyGet
| SynMemberKind.PropertyGetSet ->
let getter =
let rhsExpr = SynExpr.Ident fldId
let retInfo = match tyOpt with None -> None | Some ty -> Some (None, SynReturnInfo((ty, SynInfo.unnamedRetVal), ty.Range))
let attribs = mkAttributeList attribs mMemberPortion
let binding = mkSynBinding (xmlDoc, headPat) (access, false, false, mMemberPortion, DebugPointAtBinding.NoneAtInvisible, retInfo, rhsExpr, rhsExpr.Range, [], attribs, Some memberFlags, SynBindingTrivia.Zero)
SynMemberDefn.Member (binding, mMemberPortion)
yield getter
| _ -> ()
match propKind with
| SynMemberKind.PropertySet
| SynMemberKind.PropertyGetSet ->
let setter =
let vId = ident("v", mMemberPortion)
let headPat = SynPat.LongIdent (SynLongIdent(headPatIds, [], List.replicate headPatIds.Length None), None, Some noInferredTypars, SynArgPats.Pats [mkSynPatVar None vId], None, mMemberPortion)
let rhsExpr = mkSynAssign (SynExpr.Ident fldId) (SynExpr.Ident vId)
let binding = mkSynBinding (xmlDoc, headPat) (access, false, false, mMemberPortion, DebugPointAtBinding.NoneAtInvisible, None, rhsExpr, rhsExpr.Range, [], [], Some memberFlagsForSet, SynBindingTrivia.Zero)
SynMemberDefn.Member (binding, mMemberPortion)
yield setter
| _ -> ()]
| SynMemberDefn.Interface (ty, mWith, Some membs, m) ->
let membs' = membs |> List.collect postAutoProps
[SynMemberDefn.Interface (ty, mWith, Some membs', m)]
| SynMemberDefn.LetBindings _
| SynMemberDefn.ImplicitCtor _
| SynMemberDefn.Open _
| SynMemberDefn.ImplicitInherit _ -> []
| _ -> [memb]
let preMembers = membersIncludingAutoProps |> List.collect preAutoProps
let postMembers = membersIncludingAutoProps |> List.collect postAutoProps
preMembers @ postMembers

SynMemberDefn.AutoProperty gets transformed into three members:

  • SynMemberDefn.LetBindings to store the field
  • SynMemberDefn.Member for the getter
  • SynMemberDefn.Member for the setter

The members eventually end up in:

| SynMemberDefn.Member (bind, m), _ ->
// Phase2A: member binding - create prelim valspec (for recursive reference) and RecursiveBindingInfo
let NormalizedBinding(_, _, _, _, _, _, _, valSynData, _, _, _, _) as bind = BindingNormalization.NormalizeBinding ValOrMemberBinding cenv envForTycon bind
let (SynValData(memberFlagsOpt, _, _)) = valSynData
match tcref.TypeOrMeasureKind with
| TyparKind.Type -> ()
| TyparKind.Measure ->
match memberFlagsOpt with
| None -> ()
| Some memberFlags ->
if memberFlags.IsInstance then error(Error(FSComp.SR.tcMeasureDeclarationsRequireStaticMembers(), m))
match memberFlags.MemberKind with
| SynMemberKind.Constructor -> error(Error(FSComp.SR.tcMeasureDeclarationsRequireStaticMembersNotConstructors(), m))
| _ -> ()
let envForMember =
match incrClassCtorLhsOpt with
| None -> AddDeclaredTypars CheckForDuplicateTypars copyOfTyconTypars envForTycon
| Some _ -> envForTycon
let rbind = NormalizedRecBindingDefn(containerInfo, newslotsOK, declKind, bind)
let overridesOK = declKind.CanOverrideOrImplement
let (binds, _values), (tpenv, recBindIdx) = AnalyzeAndMakeAndPublishRecursiveValue overridesOK false cenv envForMember (tpenv, recBindIdx) rbind
let cbinds = [ for rbind in binds -> Phase2AMember rbind ]
let innerState = (incrClassCtorLhsOpt, envForTycon, tpenv, recBindIdx, List.rev binds @ uncheckedBindsRev)
cbinds, innerState

And once AnalyzeAndMakeAndPublishRecursiveValue is called, they will be added to the Sinks in

match cenv.tcSink.CurrentSink with
| Some _ when not vspec.IsCompilerGenerated && shouldNotifySink vspec ->
let nenv = AddFakeNamedValRefToNameEnv vspec.DisplayName env.NameEnv (mkLocalValRef vspec)
CallEnvSink cenv.tcSink (vspec.Range, nenv, env.eAccessRights)
let item = Item.Value(mkLocalValRef vspec)
CallNameResolutionSink cenv.tcSink (vspec.Range, nenv, item, emptyTyparInst, ItemOccurence.Binding, env.eAccessRights)
| _ -> ()

Maybe

let shouldNotifySink (vspec: Val) =
match vspec.MemberInfo with
// `this` reference named `__`. It's either:
// * generated by compiler for auto properties or
// * provided by source code (i.e. `member _.Method = ...`)
// We don't notify sink about it to prevent generating `FSharpSymbol` for it and appearing in completion list.
| None when
vspec.IsBaseVal ||
vspec.IsMemberThisVal && vspec.LogicalName = "__" -> false
| _ -> true

can be extended, not to report the two members.

And afterwards, report the AutoProperty somewhere around

Copy link
Member

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

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

OK as a hotfix for the current tooltip behavior.

@auduchinok
Copy link
Member

@nojaf Thanks for such a good analysis!

Maybe ... can be extended, not to report the two members.

Or the getter and setter could be reported at the get and set ranges instead.

And afterwards, report the AutoProperty somewhere around

That sounds great, but I'm not sure yet how easy that would be. 🙂

@nojaf nojaf mentioned this pull request Jul 12, 2023
@nojaf
Copy link
Contributor Author

nojaf commented Jul 12, 2023

Or the getter and setter could be reported at the get and set ranges instead.

Yeah, but when you have member val Foo = "bla" with get, set, if get and set correspond to the members, you do want to have some symbol for Foo right?

That sounds great, but I'm not sure yet how easy that would be. 🙂

Yeah, got somewhere in #15589 but it is a can of worms for sure.

@auduchinok
Copy link
Member

Yeah, but when you have member val Foo = "bla" with get, set, if get and set correspond to the members, you do want to have some symbol for Foo right?

Yes, something like FSharpMemberOrFunctionOrValue(cenv, P pinfo, Item.Property (pinfo.PropertyName, [pinfo])).

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.

Tooltips: two tooltips are produced for auto property with accessors clause

4 participants