Add a SymbolWalker class

[JoshVarty]

I like the approach taken by the SyntaxWalker. It separates the traversal scaffolding code from my implementation. It would be nice to have a similar API available at the symbol level. Currently to traverse the symbols in a given compilation we must inherit from SymbolVisitor and write all the scaffolding ourselves.

If you think this is relatively simple (the SyntaxWalker seems pretty basic) you can mark is at "Up for grabs" and I'll take a stab at it.

[CyrusNajmabadi]

THe problem here is that Syntax is a tree, while Symbols are a graph. How does the SymbolWalker terminate?

[JoshVarty]

My thought was to visit them from the global namespace descending to child namespaces and types. Then from those types to methods and properties. Then from those methods/properties to their parameters, type arguments and locals. (Perhaps I'm missing something, but can we not think of this approach as traversing a tree?)

However, in looking over IMethodSymbol and IPropertySymbol I don't see any ways in which we can get to locals. I don't believe we should build this unless we could cover all the symbols in the compilation, so I'm going to close this for now.

[CyrusNajmabadi]

Ok. So now you get to an IParameterSymbol. Would you then 'walk' into it's type? If so, then you face recursion problems. If not, then what are you walking, just trees?

If that's the case, you could simply use a SyntaxWalker and call GetDeclaredSymbol on all the nodes it hits. You'll then have a SymbolWalker.

[JoshVarty]

No I wasn't thinking about walking the type of the IParameterSymbol, we'd stop when we reached it. The idea is that the underlying type there would have been visited elsewhere. (It's an INamedTypeSymbol so it would have been visited as the child of a namespace at some point).

If that's the case, you could simply use a SyntaxWalker and call GetDeclaredSymbol on all the nodes it hits. You'll then have a SymbolWalker.

I think you're thinking about visiting symbols on a document level. My thought process was to create something that would visit symbols on the compilation level.

I'm trying to (maybe wrongly?) think of symbols in a child-parent relationship. A namespace contains types. Types contain members. Members contain parameters, locals, type arguments etc. Unfortunately I think my approach breaks down once we reach the member level. There's no way I can see to easily get their "Child Symbols".

[CyrusNajmabadi]

I'm trying to (maybe wrongly?) think of symbols in a child-parent relationship.

I don't think it's wrong. I think it's just an interesting view over something intrinsically graph-like.

There's no way I can see to easily get their "Child Symbols".

Why not, once you hit a member, just go back to the syntax and go dive in and get the symbols from it and a semantic model?

[jnm2]

I'm trying to (maybe wrongly?) think of symbols in a child-parent relationship. A namespace contains types. Types contain members. Members contain parameters, locals, type arguments etc. Unfortunately I think my approach breaks down once we reach the member level. There's no way I can see to easily get their "Child Symbols".

So I'm interested in walking only declarations of things (types, members, parameters), not references (base class, parameter type, etc). I think this neatly solves the graph problem. SymbolDeclarationWalker?

[CyrusNajmabadi]

@jnm2

If that's the case, you could simply use a SyntaxWalker and call GetDeclaredSymbol on all the nodes it hits. You'll then have a SymbolWalker.

:)

[jnm2]

@CyrusNajmabadi But I don't want to walk syntax. I want a unified semantic view of things like partial classes. Also, walking the syntax and calling GetDeclaredSymbol on every node can't be as smart as looking through ISymbol members.

[jnm2]

Also, it's not inconceivable that there will be members in the symbolic model that are not present in the syntax. For example, With* methods if we get withers.

[CyrusNajmabadi]

Gotcha. FWIW though, what you're describing is just a small handlful of LOCs. I'm skeptical about the overal value here. It may be good for your very specific case. But it's unclear to me if it's going to be suitable elsewhere. I mean, some users will say "i don't want implicit members" some will say " i want to walk into some of these referenced types". some will say "i don't want members from other parts not in this file", etc. etc.

The walking code is so trivial that just writing it yourself seems super simple and easy to just do that way.

[jnm2]

Does this cover every ISymbol that represents an actual declaration in today's Roslyn?

public abstract class SymbolDeclarationWalker : SymbolVisitor
{
    public override void VisitAssembly(IAssemblySymbol symbol)
    {
        base.VisitAssembly(symbol);

        foreach (var module in symbol.Modules)
        {
            module.Accept(this);
        }
    }

    public override void VisitModule(IModuleSymbol symbol)
    {
        base.VisitModule(symbol);

        symbol.GlobalNamespace?.Accept(this);
    }

    public override void VisitNamespace(INamespaceSymbol symbol)
    {
        base.VisitNamespace(symbol);

        foreach (var member in symbol.GetMembers())
        {
            member.Accept(this);
        }
    }

    public override void VisitNamedType(INamedTypeSymbol symbol)
    {
        base.VisitNamedType(symbol);

        foreach (var typeParameter in symbol.TypeParameters)
        {
            VisitTypeParameter(typeParameter);
        }

        foreach (var member in symbol.GetMembers())
        {
            member.Accept(this);
        }
    }

    public override void VisitEvent(IEventSymbol symbol)
    {
        base.VisitEvent(symbol);

        if (symbol.AddMethod != null)
        {
            VisitMethod(symbol.AddMethod);
        }

        if (symbol.RemoveMethod != null)
        {
            VisitMethod(symbol.RemoveMethod);
        }

        if (symbol.RaiseMethod != null)
        {
            VisitMethod(symbol.RaiseMethod);
        }
    }

    public override void VisitProperty(IPropertySymbol symbol)
    {
        base.VisitProperty(symbol);

        foreach (var parameter in symbol.Parameters)
        {
            VisitParameter(parameter);
        }

        if (symbol.GetMethod != null)
        {
            VisitMethod(symbol.GetMethod);
        }

        if (symbol.SetMethod != null)
        {
            VisitMethod(symbol.SetMethod);
        }
    }

    public override void VisitMethod(IMethodSymbol symbol)
    {
        base.VisitMethod(symbol);

        foreach (var typeParameter in symbol.TypeParameters)
        {
            VisitTypeParameter(typeParameter);
        }

        foreach (var parameter in symbol.Parameters)
        {
            VisitParameter(parameter);
        }
    }
}

[CyrusNajmabadi]

i don't see it handling things like local declarations.

[jnm2]

I mean, some users will say "i don't want implicit members"

You often don't want most of what a walker walks through. The concept of filtering out what you don't care about is well understood.

some will say " i want to walk into some of these referenced types"

They can override and add a symbol.BaseType?.Accept(this); call then.

some will say "i don't want members from other parts not in this file"

Seems like they care enough about syntax in this scenario that they should start there.

[CyrusNajmabadi]

also fields.

[jnm2]

i don't see it handling things like local declarations.

👍 I was thinking about metadata, forgot about executable symbols.

[CyrusNajmabadi]

preprocessor symbols.

[jnm2]

also fields.

I don't think I missed that. Fields are walked via the VisitNamedType override.

[CyrusNajmabadi]

aliases.

[jnm2]

preprocessor symbols.

You cannot run into these unless you're analyzing syntax, can you? There's no way to relate them to members of a type, for instance.

[CyrusNajmabadi]

oh, i see. you intend someone to have overridden VisitField, and then have the overrides call base.Visit. I misunderstood. I thought someone would have their own visitor, and would compose it with this.

[jnm2]

aliases.

Also can't run into these unless you start from syntax.

[CyrusNajmabadi]

You cannot run into these unless you're analyzing syntax, can you? There's no way to relate them to members of a type, for instance.

I'm just pointing out symbols someone might want to walk :)

Also can't run into these unless you start from syntax.

Sure... but if someone thinks they're going to walk symbols... they might expect to be able to walk all the declared symbols :)

[jnm2]

I thought someone would have their own visitor, and would compose it with this.

They'd inherit SymbolDeclarationWalker and override either DefaultVisit, Visit, or VisitField in order to visit fields.

[CyrusNajmabadi]

Seems like they care enough about syntax in this scenario that they should start there.

I mean, i get what you're saying. but this all just seems like a way of saying: if you need this type, this type is here.

but my point is mainly, if you need this type, creating it yourself seems fine. and avoids any decision points in the future about if we feel: welp... guess we should/shoulnd't add locals...

[CyrusNajmabadi]

They'd inherit SymbolDeclarationWalker and override either DefaultVisit, Visit, or VisitField in order to visit fields.

Gotcha. In that case, i think Fields are ok.

[jnm2]

Sure... but if someone thinks they're going to walk symbols... they might expect to be able to walk all the declared symbols :)

If you tell a SymbolDeclarationWalker to visit a syntax tree, then sure. This expectation makes sense. If you tell it to visit an IAssemblySymbol, however, why would you expect to run into an IPreprocessorSymbol or an ILocalSymbol? There's no sense in which those things are part of an IAssemblySymbol's hierarchy unless you're willing to locate and parse source files and sometimes decompile methods.

[CyrusNajmabadi]

Basically, i follow the npm view of things. If you think that is valuable, publish it. if people like it, they use your package. you can then be opinionated there. Roslyn itself can provide the building blocks, without having to absorb all this stuff, and then be locked into it.

[CyrusNajmabadi]

If you tell a SymbolDeclarationWalker to visit a syntax tree, then sure. This expectation makes sense. If you tell it to visit an IAssemblySymbol, however, why would you expect to run into an IPreprocessorSymbol?

Because they're declared symbols in my compilation... :)

Why would i not think i would hit them? same with aliases. they're declared symbols.. it's Symbol-Declaration-Walker.

--

Now, from an impl perspective, it may be hard to provide. And that's a choice that can be made. But from my perspective just thinking about the API, it's what i would expect to get.

[CyrusNajmabadi]

A similar issue arose back when we did symbol-analyzers. We had RegisterSymbolAction(..., SymbolKinds), but then it didn't actually support all symbol kinds. Which was very surprising to people, and actually made several scenarios really hard to provide. For example, if you wanted to analyze parameters (for example, to check them for naming issues), you wouldn't hear about them.

[CyrusNajmabadi]

Note: this is why having people subclass would be confusing to me. I could override VisitAliasSymbol, but never hear about. A potnetialy cleaner approach would be for you to use a SymbolVisitor internally, but expose your own extension points making it clear exactly what one would hear about.

[jnm2]

The SyntaxWalker has a configurable depth kind:

roslyn/src/Compilers/Core/Portable/Syntax/SyntaxWalkerDepth.cs

Line 8 in 13033a4

public enum SyntaxWalkerDepth : int

If it was desired to enumerate all symbols from each declaring syntax of a method, something like this could be opt-in.

A potnetialy cleaner approach would be for you to use a SymbolVisitor internally, but expose your own extension points making it clear exactly what one would hear about.

Given SyntaxWalker's approach, it would be consistent to leave these in but give a SymbolWalkerDepth setting.

[CyrusNajmabadi]

Given SyntaxWalker's approach, it would be consistent to leave these in but give a SymbolWalkerDepth setting.

Would seem reasonable to me.

[jnm2]

If you tell it to visit an IAssemblySymbol, however, why would you expect to run into an IPreprocessorSymbol?

Because they're declared symbols in my compilation... :)

But you didn't pass a Compilation to the walker, you passed an IAssemblySymbol. It's not unreasonable to assume the walker doesn't know more than you statically tell it.

We could add a Visit(SyntaxNode) method at some point, and that way it would report IPreprocessorSymbols etc.

[CyrusNajmabadi]

But you didn't pass a Compilation to the walker, you passed an IAssemblySymbol.

to me, the thing you're passing it simply defines the 'highest level view' of what i'm walking. if i'm walking an assembly, i want all things that would be lower than that. the files are part of my assembly. so the declared symbols in that file are part of it :)

I get making the decision here, and i think it's not unreasonable. i'm just mentioning it's not intuitive to me. It's something that falls out of the particular implementation approach taken here.

It's not unreasonable to assume the walker doesn't know more than you statically tell it.

I don't know what this means. You can reach all this stuff from the IAssemblySymbol using the APIs available directly from it (for example, getting the Compilation and SemanticModels from ISourceAssemblySymbol). Your impl may choose not to examine that (and i wouldn't blame it). But it's definitely something can reach and consider. Whether or not it should is another matter. I could understand the desire from an impl perspective to avoid this. :)

[jnm2]

the files are part of my assembly.

I don't think so. Files are part of a project. Assemblies are compilation outputs.
If this forces you to think in terminology that's closer to the practical reality, great!

[CyrusNajmabadi]

I don't think so. Files are part of a project. Assemblies are compilation outputs.

Sorry. 'assembly' in the IAssemblySymbol sense. From the language/compiler's perspective the assembly includes your source symbols/trees/etc.. i agree that's confusing given how .net (and C#) often refer to 'assembly'. Here, i just mean: How the Roslyn API sees things.

[jnm2]

From the language/compiler's perspective the assembly includes your symbol.

Using the ISymbol API and given an IPreprocessorSymbol, can I find the IAssemblySymbol it belongs to?

[CyrusNajmabadi]

It's been too long since i used PPSymbol. I know you can get the IAssemblySymbol from an IAliasSymbol though. Ideally PPSymbol works the same way. But, without trying it out, i'm not certain. :)

[jnm2]

Basically, i follow the npm view of things. If you think that is valuable, publish it. if people like it, they use your package. you can then be opinionated there. Roslyn itself can provide the building blocks, without having to absorb all this stuff, and then be locked into it.

After this discussion I think there's a clear pattern to follow for consistency with SyntaxWalker, but I don't particularly feel that it should be in Roslyn unless demand comes back for it.