Completion for attributes: searching for attributes recursively

[Neme12]

Customer scenario

A user tries to use code completion to add an attribute using a namespace-qualified name. The namespace doesn't appear in the completion list, and must be typed manually.

Bugs this fixes

Fixes #25589

Workarounds, if any

• Type the namespace name manually
• Add a using directive

Risk

Low. The pull request re-implements a change from an earlier release, preserving its performance-improving characteristics and restoring the original behavior.

Performance impact

We have no indication that this change will degrade performance in an observable manner. The change which led to this regression targeted a performance problem, but ended up making two changes: one of the changes was necessary to address the measured performance problem while the under was an unintended semantics change. This change restores the original semantics while preserving the performance improvement from the recent work.

Is this a regression from a previous update?

Yes. Introduced as part of #19863.

Root cause analysis

This situation only occurred when multiple levels of namespace nesting occurred. Tests have been added to prevent future regressions.

How was the bug found?

Customer reported.

Test documentation updated?

No.

[Neme12]

I added _cancellationToken

[Neme12]

The body of this method is exactly the same, I just moved it from INamespaceSymbolExtensions and changed the parameter to INamespaceOrTypeSymbol.

[Therzok]

Maybe this can be optimized a bit with a pre-filter, rather than post-filtering.

We could check IsAccessibleWithin here, rather than after, with the following reasoning:

class A
{
    class C
    {
        class MyAttribute : Attribute {}
    }
}

class B
{
    [$
    public void MyMethod() {}
}

Where $ is the caret where we trigger completion. The current method would unroll C and members of C and iterate them, which can cause additional stack resizes.

A minor change could be done as:
i.e.

public static IEnumerable<INamedTypeSymbol> GetAllTypes<T>(
    this INamespaceOrTypeSymbol namespaceOrTypeSymbol,
    CancellationToken cancellationToken,
    Func<INamedTypeSymbol, T, bool> filter = null,
    T state = default)
{
    var stack = new Stack<INamespaceOrTypeSymbol>();
    stack.Push(namespaceOrTypeSymbol);

    while (stack.Count > 0)
    {
        cancellationToken.ThrowIfCancellationRequested();
        var current = stack.Pop();
        if (current is INamespaceSymbol currentNs)
        {
            stack.Push(currentNs.GetMembers());
        }
        else
        {
            var namedType = (INamedTypeSymbol)current;
            if (filter == null || filter (namedType, state))
            {
                stack.Push(namedType.GetTypeMembers());
                yield return namedType;
            }
        }
    }
}

...
// Usage here. Note that we only do the `withinType ?? withinAssembly` check once now.
// Later edit: lambda has no captures, it is fine.
foreach (var type in namespaceOrType.GetAllTypes(cancellationToken, (t, within) => t.IsAccessibleWithin (within), withinType ?? withinAssembly)) {
    if (type.IsAttribute())
    {
        return true;
    }
}

[Therzok]

Or, instead of optional parameters, another overload could be added. It might be useful in other scenarios, not just this one.

[Neme12]

@Therzok Thanks, that look good like a possible way to do things but I wouldn't do this without measuring the difference first either. The concern I have here is that IsAccessibleWithin could potentially be a lot more expensive than IsAttribute, which would make this slower. When I dig a little into the implementation, it definitely looks more complex. IsAttribute, on the other hand is a simple base class check. Therefore I don't think we can make this change blindly just assuming it would be faster.

[Therzok]

I was going through the source code and noticed this, which is similar:
http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/FullyQualify/AbstractFullyQualifyCodeFixProvider.cs,298

There's also this, but it's a bit different, as it would need:
a) a filter which decides whether to yield return
b) a filter which decides whether to push its members

http://source.roslyn.io/#Microsoft.VisualStudio.LanguageServices/Implementation/Debugging/AbstractBreakpointResolver.cs,141
^ this one is pretty indirect, walk up the call tree from here:
http://source.roslyn.io/#Microsoft.VisualStudio.LanguageServices/Implementation/Debugging/AbstractBreakpointResolver.cs,214

[Therzok]

So, a second catch-all proposal would be:

public static IEnumerable<INamedTypeSymbol> GetAllTypes<T>(
    this INamespaceOrTypeSymbol namespaceOrTypeSymbol,
    CancellationToken cancellationToken,
    Func<INamedTypeSymbol, T, bool> resultFilter = null,
    T resultState = default,
    Func<INamedTypeSymbol, U, bool> childrenFilter = null,
    U childrenState = default)
{
    var stack = new Stack<INamespaceOrTypeSymbol>();
    stack.Push(namespaceOrTypeSymbol);

    while (stack.Count > 0)
    {
        cancellationToken.ThrowIfCancellationRequested();
        var current = stack.Pop();
        if (current is INamespaceSymbol currentNs)
        {
            stack.Push(currentNs.GetMembers());
        }
        else
        {
            var namedType = (INamedTypeSymbol)current;
            if (childrenFilter == null || childrenFilter (namedType, childrenState))
            {
                stack.Push(namedType.GetTypeMembers());
            }
            if (resultFilter == null || resultFilter (namedType, resultState))
            {
                yield return namedType;
            }
        }
    }
}

[Neme12]

I think we'd prefer the solution with another overload since the same functionality is already used in another place. But as I said, we don't know if this would actually be better given how complex IsAccessibleWithin is compared to IsAttribute.

[Therzok]

I think this can be modeled in a way that we can:
a) both gain performance of not pushing in types which are not accessible
b) use the faster IsAttribute() check for result filtering.

[Neme12]

@Therzok But childrenFilter (IsAccessibleWithin) is still called before IsAttribute, which might be a problem, depending on how efficient it is.

[Therzok]

Okay, I misunderstood what you meant by running IsAccessibleWithin on every node. Yes, doing post-filtering is better in this case.

[Therzok]

Any idea how to get nuget packages for local roslyn builds? I want to profile the scenario we had and see how timings compare.

Although, they will not provide identical results, since we replaced roslyn layer, but I expect that the common cut off point AbstractRecommendationService.ShouldIncludeSymbol should yield the same results.

[Neme12]

rebased to 15.7 by @sharwell's request

[CyrusNajmabadi]

Note: as per the issue thread:

Unfortunately, it's hard to know for certain (without a perf investigation) if adding the walk back in would be a problem or not. Potentially walking (and realizing) all types in a compilation is something that could be quite expensive. Worse, it might be fine in some scenarios, but might be terrible for someone with, say, some interestingly generated assembly.

Given enough time, the best thing here would probably be to do a deep perf investigation of how thigns might be impacted if we restored the full walk, but avoided lambda allocations. However, if there isn't the time for that, it would likely be acceptable to just default to adding all namespaces. This would be extremely safe, with practically no risk of perf problems, while only degrading the experience somewhat by including namespaces in the list that weren't necessary.

The former is costly but more precise. The latter is much cheaper, but less precise.

I'm not a fan at all of any sort of unbounded walk during intellisense. Intellisense follows an Concurrent-but-blocking model. Meaning, if the user types <letter><tab> then we'll kick off the work in the BG to compute the results, but then we'll block on them on <Tab> so we have accurate results with which to complete with. This approach is necessary because one of our principles around completion is that it should behave consistently, and should not give you different results based on timing (which would lead to throwing off muscle memory).

As such, we try to make sure that our completion algorithms are fixed in some dimensions. i.e. "number of symbols directly in a namespace".

An algorithm that can potentially hit every type in a dll (and force hte compiler to then bind each of those types to figure out if its an attribute) is pretty concerning. All it takes is one 'interesting' dll, and people may find that completion can hang on htem for an unbounded amount of time.

If at all possible, i would prefer we not have any sort of algorithm here that walks all types.

[sharwell]

@CyrusNajmabadi I imagine there is room for further improvement here. However, This was a regression introduced in the 15.6 release due to an unintentional elimination of a recursive walk. The performance problem was correction by an unrelated change. This pull request was validated this morning against the original use cases leading to the performance issue and did not cause a regression, so I'm reviewing it on the basis of a regression bug fix and not the ideal final state.

[CyrusNajmabadi]

This pull request was validated this morning against the original use cases leading to the performance issue and did not cause a regression,

I'm very concerned that it's only being examined in that context. To give some background, we had previously done full walks in completion to determine symbol applicability, and we also had to roll that back because of the cost for some customers where that walk turned out to be too expensive.

So adding this back in seems highly risky, for marginal gain. A much simpler, and low risk solution, would be to just show the nested namespaces for a namespace, regardless of whether or not those nested namespaces transitively contained an attribute in them.

This would mean no risk of perf degradation for any customers, while also only having the drawback of a few more namespaces being shown.

Given that hte UI today already shows namespaces interleaved with types, it doesn't seem like a bad situation to have the UI just show a few more namespaces. Contrast the known badness there with the unknown badness of hitting a bad perf case in a perf critical scenario.

--

Note: i would normally not be too concerned. However, given that it's now twice htat a full walk has led to problems, i would be very wary of adding a full walk back in unless the benefits of such an approach were really significant. In this case, that doesn't at all seem like the case. The full walk only marginally improves the UI.

[sharwell]

@CyrusNajmabadi performance metrics from the 15.5 release do not indicate that the scenario you are concerned about is causing problems. In the absence of direct evidence of a problem, I would prefer a regression bug fix focus on precisely restoring the original behavior and we can leave the remaining performance concerns for a later work item.

[CyrusNajmabadi]

@CyrusNajmabadi performance metrics from the 15.5 release do not indicate that the scenario you are concerned about is causing problems.

Performance metrics also did not indicate the memory issue that @Therzok originally found and fixed. So it's unclear to me if the perf metrics are up to the task of detecting and reporting back these sorts of issues.

The current approach seems risky, for little benefit. Is that risk offset by something substantively beneficial?

[Neme12]

Just to show some examples here (System and Microsoft.CodeAnalysis):
filtered namespaces on the left (this PR and before @Therzok 's one), all namespaces on the right

Originally my idea was to show all namespaces and I saw that as a pretty satisfactory behavior, but after knowing that this indeed wasn't an intentional change and the filtering itself wasn't actually known to be a performance problem (even though @CyrusNajmabadi has understandable concerns that it might be for some customers and mentioned that things similar to this have been a problem), I'm definitely leaning on keeping the filtering as is. (But you may notice I might be slightly biased here because I wrote this PR 😄 )

It's not substantially beneficial, but it is nice to have, and definitely noticable. I see two advantages of the filtering

1. it helps you if you want to find all possible attributes that exist (I'm not sure how many people do this, but I could imagine myself trying to find an attribute having forgotten its name)
2. it would definitely be a suboptimal experience for completion to lead you into a namespace and then show nothing inside it, which would happen a lot if we don't filter. It may even seem to users like completion is broken at that point.

Considering this (especially the second point), I would prefer not to prematurely regress behavior and remove features without having some evidence/reports that this is actually a problem in this particular case. (And if it is, I wouldn't want to give up so quickly on finding a more performant solution to get the same behavior)

[sharwell]

@jinujoseph Requesting approval to merge this regression bug fix

[jinujoseph]

test windows_debug_unit64_prtest

[Neme12]

Note: I'm not taking any stance on this. @sharwell If you think this is a good idea then that's fine but I hope you don't rely on any of my attempts to do measurements in your assessment (which I don't remember exactly but I think did show some slowdown even though it was still a lot better than the original state of things before @Therzok 's PR). I don't want to take any responsibility here.

[Neme12]

I merged master into this to hopefully get the CI tests up to date and working.