Don't add parenthesis when committing type with accessible nested type using dot

[genlu]

fix #78515, #51629

[genlu]

Maybe we could make it even less aggressive (and simpler) in auto-completing () by only checking for types declared public, thoughts?

[CyrusNajmabadi]

We have an IsAccessibleTo helper. Can you use that?

[genlu]

@CyrusNajmabadi Done. Could you pls take another look?

[jasonmalinowski]

Is this comment stale now? I can't quite tell. But it does allude to what I'm a bit confused about -- should we be explicitly adding the parenthesis only when there isn't a commit character, or the user typed a paren? I guess this original behavior is making me scratch my head in the first place...

[genlu]

I think this comment still applies, especially when combined with the new comment a couple of lines below. I don't remember the original design, but this behavior doesn't feel counter intuitive to me. Since this is after new, the type name can only be used as a constructor (except the nested type scenario, which is what this PR's about), if you commit with . or ;, you'd need to move cursor back and type the ( otherwise, and when you commit TAB, you can keep on typing ( without moving cursor

[CyrusNajmabadi]

note: this is not really accurate. for example, you can have protected/private nested types that are then referencable from within the inner type. i think you want if (typeMember.IsAccessibleWithin( and then pass the .ContainingType for where the operation was invokved from. Which i think is on hte syntax context. So basically, get rid of the first two 'if' checks, and update the bottom one.

these are good to test as well.

[genlu]

yea, this is intended to be a heuristic to cover common cases. Because we might be handed a speculative member model here (see this), I think we can't reliably do something like below, otherwise it might throw

var containingType = syntaxContext.SemanticModel.GetDeclaredSymbol(syntaxContext.ContainingTypeDeclaration!, cancellationToken);
if (typeMember.IsAccessibleWithin(containingType))
...

you can have protected/private nested types that are then referencable from within the inner type

I think this is fine. The change is only relevant when you are typing the outer type name, and I think people typically don't access the nested ones via the outer one when they are within the inner or derived type. For example

class Outer
{
    class Inner1
    {
        void M()
        {
            // you could use `Inner` instead of `Outer.Inner` here
        }
    }
    
}

[CyrusNajmabadi]

I think this is fine. The change is only relevant when you are typing the outer type name, and I think people typically don't access the nested ones via the outer one when they are within the inner or derived type. For example

That's a reasonable point. Though i think you'll still just be in an easier scenario simplifying, and using hte above approach. it's a case where, IMO, it's simpler and more correct.

From offline convo, you should be fine to call GetDeclaredSymbol even for speculative scenarios.

[genlu]

Talked offline, the comment about speculative member model above stands so merging as is.
added more comments in the code to explain this.

[CyrusNajmabadi]

Preemptively signing off if you make the suggested change. if you can't, please let me know why. thanks.

[CyrusNajmabadi]

Specifically, the suggested change about checking accessibility properly. not the minor suggestion to use an expression-bodied member.