HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

IdentifierReferenceResolver: resolving at project level

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
levelidentifierreferenceresolverprojectresolving

Problem

One of the most important parts at the core of rubberduck is the IdentifierReferenceResolver class, which is responsible for resolving each declaration as the parse tree is being walked and function, member and variable calls are being encountered.

Proper scoping is the single most important concept here: when we meet an assignment to foo, we possibly have 20 foo identifier declarations, and we need to find the single Declaration object that's being referred to, create an IdentifierReference object and add it to that declaration object's References concurrent bag.

So that's what the IdentifierReferenceResolver class is doing. For context, here's a few lines of code from the main ResolveInternal overload:

callee = FindLocalScopeDeclaration(identifierName, localScope, parentContext, isAssignmentTarget)
      ?? FindModuleScopeProcedure(identifierName, localScope, accessorType, isAssignmentTarget)
      ?? FindModuleScopeDeclaration(identifierName, localScope)
      ?? FindProjectScopeDeclaration(identifierName, Equals(localScope, _currentScope) ? null : localScope, hasStringQualifier);


The resolver first tries to find the Declaration object for identifierName within the current (local) scope; if that doesn't work, it tries to find a procedure (member actually) in the same module that could match - otherwise FindModuleScopeDeclaration would find 2 matches when a property has a getter and a setter procedures. if that returns null, then it tries to find a match at module scope, and when all else fails, it tries to find a match at project scope - and this is where things get funky.

I had this working code:

```
private Declaration FindProjectScopeDeclaration(string identifierName, Declaration localScope = null, bool hasStringQualifier = false)
{
var matches = _declarations.Items.Where(item => !item.IsBuiltIn && item.IdentifierName == identifierName
|| item.IdentifierName == identifierName + (hasStringQualifier ? "$" : strin

Solution

try
{
    return matches.SingleOrDefault(IsUserDeclarationInProjectScope)
    ?? matches.SingleOrDefault(item => IsBuiltInDeclarationInScope(item, localScope));
}
catch (InvalidOperationException)
{
    return null;
}


This doesn't look like an appropriate use of try-catch, but one you've been forced into by the semantics of SingleOrDefault. In fact, I'm not sure the behaviour is what you want -- if two or more elements of matches satisfy IsUserDeclarationInProjectScope then an InvalidOperationException is thrown, and we never check if any elements satisfy IsBuiltInDeclarationInScope.

I think what you want is to write a method

TSource SingleOrDefault(
        this IEnumerable source,
        Func predicate,
        TSource defaultValue)


that returns

  • defaultValue if there are 0 elements satisfying predicate,



  • defaultValue if there are 2+ elements satisfying predicate,



  • The only matching element, otherwise.



With this method, we can write instead

return matches.SingleOrDefault(IsUserDeclarationInProjectScope, null)
    ?? matches.SingleOrDefault(item => IsBuiltInDeclarationInScope(item, localScope), null);

Code Snippets

try
{
    return matches.SingleOrDefault(IsUserDeclarationInProjectScope)
    ?? matches.SingleOrDefault(item => IsBuiltInDeclarationInScope(item, localScope));
}
catch (InvalidOperationException)
{
    return null;
}
TSource SingleOrDefault<TSource>(
        this IEnumerable<TSource> source,
        Func<TSource, bool> predicate,
        TSource defaultValue)
return matches.SingleOrDefault(IsUserDeclarationInProjectScope, null)
    ?? matches.SingleOrDefault(item => IsBuiltInDeclarationInScope(item, localScope), null);

Context

StackExchange Code Review Q#108090, answer score: 4

Revisions (0)

No revisions yet.