7
\$\begingroup\$

The "parser state" of a module in can be one of several values:

//note: ordering of the members is important
public enum ParserState
{
    /// <summary>
    /// Parse was requested but hasn't started yet.
    /// </summary>
    Pending,
    /// <summary>
    /// Project references are being loaded into parser state.
    /// </summary>
    LoadingReference,
    /// <summary>
    /// Code from modified modules is being parsed.
    /// </summary>
    Parsing,
    /// <summary>
    /// Parse tree is waiting to be walked for identifier resolution.
    /// </summary>
    Parsed,
    /// <summary>
    /// Resolving identifier references.
    /// </summary>
    Resolving,
    /// <summary>
    /// Parser state is in sync with the actual code in the VBE.
    /// </summary>
    Ready,
    /// <summary>
    /// Parsing could not be completed for one or more modules.
    /// </summary>
    Error,
    /// <summary>
    /// Parsing completed, but identifier references could not be resolved for one or more modules.
    /// </summary>
    ResolverError,
}

Now, Rubberduck uses each modules' state, and determines what the "overall" state is - and then displays that value in a status bar.

The rules are:

  • If all modules have the same state, overall state is that value.
  • If any module is in an error state, overall state is that error value.
  • Overall state can only be "ready" when all modules are "ready".
  • Overall state can only be "parsed" when all modules are "parsed".
  • Otherwise return the state of the most advanced non-ready module.

The code started simple, got complicated, then too simple, and now looks like this - basically it went back to "too complicated":

private static readonly ParserState[] States = Enum.GetValues(typeof(ParserState)).Cast<ParserState>().ToArray();
private ParserState EvaluateParserState()
{
    var moduleStates = _moduleStates.Values.ToList();
    var state = States.SingleOrDefault(value => moduleStates.All(ps => ps == value));

    if (state != default(ParserState))
    {
        // if all modules are in the same state, we have our result.
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", state, Thread.CurrentThread.ManagedThreadId);
        return state;
    }

    // error state takes precedence over every other state
    if (moduleStates.Any(ms => ms == ParserState.Error))
    {
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", ParserState.Error,
        Thread.CurrentThread.ManagedThreadId);
        return ParserState.Error;
    }
    if (moduleStates.Any(ms => ms == ParserState.ResolverError))
    {
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", ParserState.ResolverError,
        Thread.CurrentThread.ManagedThreadId);
        return ParserState.ResolverError;
    }

    // intermediate states are toggled when *any* module has them.
    var result = moduleStates.Min();
    if (moduleStates.Any(ms => ms == ParserState.Parsing))
    {
        result = ParserState.Parsing;
    }
    if (moduleStates.Any(ms => ms == ParserState.Resolving))
    {
        result = ParserState.Resolving;
    }

    if (result == ParserState.Ready && moduleStates.Any(item => item != ParserState.Ready))
    {
        result = moduleStates.Except(new[] {ParserState.Ready}).Max();
    }

    Debug.Assert(result != ParserState.Ready || moduleStates.All(item => item == ParserState.Ready));

    Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", result,
    Thread.CurrentThread.ManagedThreadId);
    return result;
}

Does anything stick out? Could it be simplified? Any/all feedback welcome!

\$\endgroup\$
  • \$\begingroup\$ LoadingReferences shouldn't be a member of that enum. And it needs a AcquiringDeclarations state, for the "first pass" walk; ResolvingReferences is really about the "second pass" that resolves identifier references to acquired declarations. \$\endgroup\$ – Mathieu Guindon Apr 26 '16 at 16:29
5
\$\begingroup\$
  • If I understand the rules correctly, the last rule "Otherwise return the state of the most advanced non-ready module." isn't met by your method. Having states {ParserState.LoadingReference, ParserState.Parsed, ParserState.Parsing, ParserState.Ready} I would expect the returned state equals to ParserState.Parsed but your method results in ParserState.Parsing.

  • The rules 3 and 4 are superflous because they are satisfied by rule 1.

  • Because any error state takes precedence over every other state you should chewck this first. If all states are of the same error state it will result in the same result, but checking first the error states will be faster.

  • this

    if (result == ParserState.Ready && moduleStates.Any(item => item != ParserState.Ready))
    {
        result = moduleStates.Except(new[] {ParserState.Ready}).Max();
    }  
    

    looks strange. result equals to the Min() of the modulStates, so let us assume it would equal to ParserState.Ready then it is garanteed that the second condition will result in true because you already checked if all states are the same. Because of the the check of the same values the second condition of Debug.Assert(result != ParserState.Ready || moduleStates.All(item => item == ParserState.Ready)); is superflous as well.

  • You should use the same variables name for the same type when using lambdas. One time you have ps => ps == value and the other times you use ms => ms == ParserState.Error.

  • The method relies on a class variable which I would change. Add a parameter to that method declaration so it can stand on its own.

Implementing the mentioned points will lead to

private ParserState EvaluateParserState(IList<ParserState> moduleStates)
{
    // error state takes precedence over every other state
    if (moduleStates.Any(ms => ms == ParserState.Error))
    {
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", ParserState.Error,
        Thread.CurrentThread.ManagedThreadId);
        return ParserState.Error;
    }
    if (moduleStates.Any(ms => ms == ParserState.ResolverError))
    {
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", ParserState.ResolverError,
        Thread.CurrentThread.ManagedThreadId);
        return ParserState.ResolverError;
    }

    var state = States.SingleOrDefault(value => moduleStates.All(ms => ms == value));

    if (state != default(ParserState))
    {
        // if all modules are in the same state, we have our result.
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", state, Thread.CurrentThread.ManagedThreadId);
        return state;
    }

    var result = moduleStates.Except(new[] { ParserState.Ready }).Max();

    Debug.Assert(result != ParserState.Ready);

    Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", result,
    Thread.CurrentThread.ManagedThreadId);
    return result;
}
\$\endgroup\$
  • \$\begingroup\$ Gah, forgot to mention, I should probably remove LoadingReferences from that enum at this point, it's not really a "parser state" - it was just a hack to get the status bar to display "Loading references..." (moot, now that I have a way to send a text message to the status bar)... there's not a single chance that {LoadingReferences,Parsed,Ready} could ever happen, we load all references from all projects at once. The other states aren't really predictable though - depends on how the runtime decides to dispatch the threads, and whether parsing/resolving works for each module. \$\endgroup\$ – Mathieu Guindon Apr 26 '16 at 14:07
5
\$\begingroup\$

I don't know whether I've missed a nuance of this, but if you only consider the distinct states in the system the code gets a lot easier.

This is typed into my browser and unchecked but I think it should work:

private ParserState EvaluateParserState()
{
    var moduleStates = _moduleStates.Values.Distinct().ToList();

    // TODO - what should happen if moduleStates.Count == 0?        

    var computedState = ParserState.Pending;

    // Rules 1, 3 and 4
    if (moduleStates.Count == 1)
    {
        computedState = moduleStates[0];
    }
    else
    {
        var allowableStates = moduleStates.Except(new[] { ParserState.Parsed, ParserState.Ready });

        // Rules 2 and 5
        // As Heslacher notes, `ResolverError` will take precedence over `Error`.
        // Spec doesn't specify which is more important but this is not the same
        // behaviour as the current code.
        computedState = allowableStates
            .DefaultIfEmpty(ParserState.Parsed) // empty means all parsed or ready.
            .Max();
    }

    LogParserState(computedState);

    return computedState;
}

As for your code, you're repeating very similar logging code - that should be refactored to a method IMO.


Your code doesn't follow your own rules. Just FYI:

new[] { ParserState.Parsed, ParserState.Parsed, ParserState.Ready } returns ParserState.Parsed which I think is wholly sensible (and what my suggestion does) but violates the wording of your rule 3.

\$\endgroup\$
  • 2
    \$\begingroup\$ This won't produce the same result (original Error comes before ResolverError) but nevertheless its a neat solution. \$\endgroup\$ – Heslacher Apr 26 '16 at 8:44
  • 1
    \$\begingroup\$ Shouldn't you also check for Parsed state in Where method to satisfy rule 3? Otherwise your method returns Parsed for { ParserState.Pending, ParserState.Parsed, ParserState.Ready } sequence. \$\endgroup\$ – Nikita B Apr 26 '16 at 13:39
  • \$\begingroup\$ @NikitaB - yes! Good spot \$\endgroup\$ – RobH Apr 26 '16 at 14:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.