Evaluating Parser State

Question

The "parser state" of a module in rubberduck 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!

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. — Mat's Mug♦, 2 days ago

Heslacher · Answer 1 · 2016-04-26 07:55:44Z

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;
}

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. — Mat's Mug♦, 2 days ago

RobH · Answer 2 · 2016-04-26 14:29:39Z

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.

This won't produce the same result (original Error comes before ResolverError) but nevertheless its a neat solution. — Heslacher, 2 days ago
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. — Nikita B, 2 days ago

asked	2 days ago
viewed	466 times
active	2 days ago

current community

your communities

more stack exchange communities

Evaluating Parser State

2 Answers 2

Your Answer

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

Hot Network Questions

current community

your communities

more stack exchange communities

Evaluating Parser State

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions