Github, generate report

Please read https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#basic-difference-report-with-label-and-custom-projects-list
We need to put link to config and link to list of projects to PR description

Github, generate report

Regression-Oct-03:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/725c1e1_2023135533/reports/diff/index.html

We don't need a separate stack for prevScopeUninitializedVariables. We can track those variables within ScopeData and rely on the current check's logic to pop from scopeStack.

Explain to me how this code is the same to the previous code, and there is no logical differences.

Also, does this fix #13184 (comment) ?

Explain to me how this code is the same to the previous code, and there is no logical differences.

The prevScopeUninitializedVariables was used for two tokens: EXPR and SLIST.

EXPR

visitToken stays unchanged. storePrevScopeUninitializedVariableData() stores the current state of uninitialized variables into scopeData.prevScopeUninitializedVariables instead of pushing it into the stack prevScopeUninitializedVariables.push(prevScopeUninitializedVariableData);.

leaveToken logically stays the same. updateAllUninitializedVariables() iterates over all scopes trying to update uninitialized variables updateUninitializedVariables(scopeData.prevScopeUninitializedVariables). If you check the old logic, it did very similar thing. It tried to update uninitialized variables from the last scope and then iterated via all scopes (including the last scope again):

private void updateAllUninitializedVariables(
             Deque<DetailAST> prevScopeUninitializedVariableData) {
...
            // Check for only previous scope
             updateUninitializedVariables(prevScopeUninitializedVariableData);
             // Check for rest of the scope
             prevScopeUninitializedVariables.forEach(this::updateUninitializedVariables);
...
}

SLIST

visitToken stays unchanged. Uninitialized variables are stored in the curent scope and then a new scope is pushed into the stack:

storePrevScopeUninitializedVariableData();
scopeStack.push(new ScopeData());

leaveToken logically stays the same. The current scope is first popped scope = scopeStack.pop().scope; and then updateAllUninitializedVariables() iterates over all scopes trying to update uninitialized variables. Because of the ordering of operations in the visitToken the uninitialized variables are not lost here. Same was with the old logic where it explicitly popped the element from the prevScopeUninitializedVariables.

Also, does this fix #13184 (comment) ?

I haven't checked the state of the scopeStack between analysis of multiple files, similar issue may be present as with the removed prevScopeUninitializedVariables stack for EXPR branch, but I think it should be investigated and handled as a separate issue.

@rdiachenko Let me know if I missed it, but what about the fact that we are switching from a double array to a single?

This means we are no longer returning to a previous state when we would have pop. storePrevScopeUninitializedVariableData always did a push on both tokens you described, and then did a pop on leaving them. It acted as a save/store of the current state, before moving into a new, nested state.

Now with your changes, when we call updateAllUninitializedVariables we seem to be removing things in the array when shouldRemove is true. So instead of doing a "return" to a previous state, we almost clear out the array assuming we are starting fresh and lose the previous values.

Because of the ordering of operations in the visitToken the uninitialized variables are not lost here.

It seems we are, as I described above.

@rnveach

but what about the fact that we are switching from a double array to a single?

We are not switching from a doubly array. Instead, we are moving the second dimention into ScopeData:

// before
private final Deque<ScopeData> scopeStack = new ArrayDeque<>();
private final Deque<Deque<DetailAST>> prevScopeUninitializedVariables = new ArrayDeque<>();

// after
private final Deque<ScopeData> scopeStack = new ArrayDeque<>();
private static final class ScopeData {
  private Deque<DetailAST> prevScopeUninitializedVariables = new ArrayDeque<>();
}

Because we reuse scopeStack which is a stack with each element ScopeData containing an array of prevScopeUninitializedVariables, we are still in two dimentions.

This means we are no longer returning to a previous state when we would have pop.

Because we still use double array and push / pop operations are equivalent to what they used to be, we do return to the previous state. Let's check this:

EXPR

// before
public void visitToken(DetailAST ast) {
  case TokenTypes.EXPR:
    if (ast.getParent().getType() == TokenTypes.SWITCH_RULE) {
      // store `Deque<DetailAST> prevScopeUninitializedVariableData` 
      // into `prevScopeUninitializedVariables`
      storePrevScopeUninitializedVariableData(); 
    }
}

public void leaveToken(DetailAST ast) {
  case TokenTypes.EXPR:
    if (parentAst.getType() == TokenTypes.SWITCH_RULE) {
      // no pop happens
      prevScopeUninitializedVariableData = prevScopeUninitializedVariables.peek(); 
      if (shouldUpdateUninitializedVariables(parentAst)) {
        updateAllUninitializedVariables(prevScopeUninitializedVariableData);
      }
    }
}


// after
public void visitToken(DetailAST ast) {
  case TokenTypes.EXPR:
    if (ast.getParent().getType() == TokenTypes.SWITCH_RULE) {
      // store `Deque<DetailAST> prevScopeUninitializedVariableData` into `scopeStack`
      storePrevScopeUninitializedVariableData(); 
    }
}

public void leaveToken(DetailAST ast) {
  case TokenTypes.EXPR:
    if (parentAst.getType() == TokenTypes.SWITCH_RULE ....) {
      // no pop happens, similar iteration over all scopes trying to update
      // uninitialized variables as before
      updateAllUninitializedVariables(); 
    }
}

SLIST

// before
public void visitToken(DetailAST ast) {
  case TokenTypes.SLIST:
    if (ast.getParent().getType() != TokenTypes.CASE_GROUP....) {
      // store `Deque<DetailAST> prevScopeUninitializedVariableData` 
      // into `prevScopeUninitializedVariables `
      storePrevScopeUninitializedVariableData();
      scopeStack.push(new ScopeData());
    }
}

public void leaveToken(DetailAST ast) {
  case TokenTypes.SLIST:
    prevScopeUninitializedVariableData = prevScopeUninitializedVariables.peek();
      if (parentAst.getType() != TokenTypes.CASE_GROUP....) {
        scope = scopeStack.pop().scope;
        prevScopeUninitializedVariables.pop();
      }
      if (containsBreak || shouldUpdateUninitializedVariables(parentAst)) {
        // use the last element from `prevScopeUninitializedVariables` which was popped
        updateAllUninitializedVariables(prevScopeUninitializedVariableData);
      }
}


// after
public void visitToken(DetailAST ast) {
  case TokenTypes.SLIST:
    if (ast.getParent().getType() != TokenTypes.CASE_GROUP....) {
      // store `Deque<DetailAST> prevScopeUninitializedVariableData` into `scopeStack`
      storePrevScopeUninitializedVariableData();
      scopeStack.push(new ScopeData());
    }
}

public void leaveToken(DetailAST ast) {
  case TokenTypes.SLIST:
    if (parentAst.getType() != TokenTypes.CASE_GROUP......) {
      // it is safe to pop because `prevScopeUninitializedVariableData` is contained
      // in the next element due to the push ordering in the `visitToken`
      scope = scopeStack.pop().scope;
    }
    if (containsBreak || shouldUpdateUninitializedVariables(parentAst)) {
      // at this point `scopeStack.peek` contains `prevScopeUninitializedVariableData` 
      // which was stored during `visitToken`
      updateAllUninitializedVariables(); 
    }
}