Roslyn: Intellisense doens't work in Attributes

[vsfeedback]

When typing out an Attribute, Intellisense doesn't include any available Namespaces other than those included in the System namespace.

This makes traversing Namespaces very difficult when specifying an attribute on a member.

To replicate:
Start typing out an Attribute Usage on a Class, Method, Field, Property, etc.
Look through the Intellisense list for a Namespace that you're looking for (other than System). It doesn't appear.

_This issue has been moved from https://developercommunity.visualstudio.com/content/problem/216577/intellisense-doenst-work-in-attributes.html
VSTS ticketId: 584324_
_These are the original issue comments:_
(no comments)
_These are the original issue solutions:_
(no solutions)

[Neme12]

Could you please provide a repro with some sample code and exact steps? Thanks!

[Neme12]

Do you have have a using for the relevant namespace?

[elvishfiend]

@Neme12 I've created a "repro" here: https://github.com/elvishfiend/AttributeBugRepro/tree/master.
I can probably make a video walking through the issue if necessary.

I've added a using for the root namespace, and for the containing namespace, but the problem persists, and neither should be required for navigating the entire namespace with Intellisense.

[elvishfiend]

Here's a sequence of images to hopefully provide better context about what I'm attempting to do.

[Neme12]

Thanks, I see what's going on. It's only looking one level deep to see if there's any attribute class in that namespace, not into nested namespaces.

Is is possible this by design for performance reasons? Could somebody weigh in on this? @CyrusNajmabadi @sharwell Thanks.

If this indeed can't be done in a performance sensitive way, my suggestion would be that we offer all namespaces regardless of whether they contain any attributes and then only filter attribute classes inside each namespace. The current experience seems bad as there's no way to get completion to help you navigate. IMO it's better to offer more than the user may need than not offer something they're looking for.

[Neme12]

This is the method in question;
https://github.com/dotnet/roslyn/blob/8917d3b6f55f1beb2548f06baa88f79139f4351a/src/Workspaces/Core/Portable/Shared/Extensions/ISymbolExtensions.cs#L560-L588

[Neme12]

I submitted a PR: #25622, we'll see what others think about this.
Thanks for reporting the issue!

[Neme12]

BTW here's a small repro not requiring any packages:
```c#
namespace Namespace1.Namespace2
{
class CustomAttribute : System.Attribute { }
}

[$$] // 'Namespace1' isn't offered
class C
{
}
```

If I type Namespace1.$$, Namespace2 is offered. Similarly, if the attribute is only contained within 1 namespace, the namespace is offered.

[elvishfiend]

I was working to towards this conclusion as I was creating the repro, but wasn't quite sure how to explain it.
Hopefully the fix gets approved.
Thanks!

[CyrusNajmabadi]

Note: this could effectively walk the entire symbol namespace. That's definitely concerning. @sharwell, how good are our perf tests around typing and completion such that we'd catch any issues if this regresses things here?

[CyrusNajmabadi]

Back in ye olde annals of history:

We used to actually do the walk of all namespaces, and it was far too expensive to be acceptable. So we turned it off. During roslyn, i think we looked into this, and decided it was ok to just do a single layer of filtering. With the idea being that the vast majority of cases would be handled either by:

1. the user already has the right using in scope, and thus can see his attribute in the list.
2. the user doesn't have that using. but they then type the attribute name, and we offer to add-using.

I think we felt it was acceptable to just support those as the primary mechanism for this context, and we knowingly allowed this other scenario (wanting to walk through namespaces, even those without attributes) to suffer.

--

However, given the state we're in today, i think being so restrictive isn't really that helpful. I mean, we already do show namespaces in the list. So i think we could just move to a model where we show the attributes in the namespace, and we show all child namespaces. Worse case, a few more namespaces are shown, even if they don't contain attributes. But is that really that important? Now that we have completion filtering, users can just hit alt-C to filter down just to classes.

So, i would just remove the 'smarts' from teh feature entirely. just include namespaces, attribute types, and named types that contain nested-attributes-types in them.

This should address this bug, not have perf concerns, and should not majorly degrade the experience.

[elvishfiend]

Using @Neme12 's repro, it's working as expected/desired in VS2015 and VS2013, so this seems like a feature regression.

If it causes performance issues, I'd be keen on either:

• Offer every namespace, and filter attributes (already suggested by @Neme12 )
• Offer every namespace if it contains a namespace or an attribute, and filter attributes

[CyrusNajmabadi]

I would be fine with: "Offer every namespace, and filter attributes (already suggested by @Neme12 )"

[CyrusNajmabadi]

Note: this is a regression. @Neme12 found the PR that caused it: https://github.com/dotnet/roslyn/pull/19863

That PR was specifically introduced to address an allocation perf problem. However, it ultimately addressed the allocation problem in a slightly unintentional way. It both removed some allocations from the core codepath and it removed a potential full walk of the namespace and type hierarchy that used to exist.

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.