[Enhanced Switch][Record Pattern] Simplify exhaustiveness checking for switch statements with record patterns

[srikanth-sankaran]

• Fixes [Enhanced Switch][Record Patterns] Simplify exhaustiveness checking for switch statements with record patterns #3458

[stephan-herrmann]

@srikanth-sankaran we all keep getting mail notifications about github activities like this. I would really appreciate if each issue and each PR has a headline that lets me see if the notification is of interest for me. "WIP" is not such a headline.

Another anti pattern is: "fix issue 4711" etc. because in my mail client there is no hover that expands what that referenced issue is about.

And while I'm at it: if you don't add details into the issue template, why not just remove it and replace it with at least one line of a description?

[srikanth-sankaran]

@srikanth-sankaran we all keep getting mail notifications about github activities like this. I would really appreciate if each issue and each PR has a headline that lets me see if the notification is of interest for me. "WIP" is not such a headline.

Hi @stephan-herrmann (and all others), First of all, my apologies. I can certainly provide a more descriptive heading and some body comments but perhaps you have more pertinent suggestion for how I may structure my work flow given this detail below:

I often raise a PR well before the work is complete as a way of continuous/early testing. Typically I have some part of the fix in place and once that locally passes some smoke tests, I want to launch full test runs without burdening my laptop locally and so generate a PR.

Question: Is there a way I can suppress these notifications - perhaps a draft PR would trigger tests without spamming everyone ???

Question: I also end up incrementally pushing several additional commits - do these generate notifications too (I sure hope not!!!)

Another anti pattern is: "fix issue 4711" etc. because in my mail client there is no hover that expands what that referenced issue is about.

I am under the impression I am following the recommended practice! Can you point me to an instance of a PR by me that shows the problematic anti pattern and an example that shows how it ought to have been done ??

And while I'm at it: if you don't add details into the issue template, why not just remove it and replace it with at least one line of a description?

My intent is fill them at a later date - hence they are left in place, but I can certainly provide some write up in the initial stage itself and evolve it to the final form when merging the PR.

[srikanth-sankaran]

Here is a documentation of the reasoning behind the step by step simplification:

1. We may conclude that the field org.eclipse.jdt.internal.compiler.ast.SwitchStatement.RecordPatternNode.rNode is useless observing that it is never assigned to. And since there are no overrides to org.eclipse.jdt.internal.compiler.ast.SwitchStatement.NodeVisitor.visit(RNode) and the only implementation returns to, we may carry out the following actions:
   a. Delete the field org.eclipse.jdt.internal.compiler.ast.SwitchStatement.RecordPatternNode.rNode
   b. Simplify org.eclipse.jdt.internal.compiler.ast.SwitchStatement.RecordPatternNode.traverse(NodeVisitor) to be

		@Override
		public void traverse(NodeVisitor visitor) {
			if (visitor.visit(this)) {
				if (this.next != null) {
					visitor.visit(this.next);
				}
			}
			visitor.endVisit(this);
		}

and c. Simplify org.eclipse.jdt.internal.compiler.ast.SwitchStatement.RecordPatternNode.toString() to be

	@Override
		public String toString() {
	        StringBuilder sb = new StringBuilder();
	        sb.append("[RecordPattern node] {\n"); //$NON-NLS-1$
	        sb.append("    type:"); //$NON-NLS-1$
	        sb.append(this.type != null ? this.type.toString() : "null"); //$NON-NLS-1$
	        sb.append("    next:"); //$NON-NLS-1$
	        sb.append(this.next != null ? this.next.toString() : "null"); //$NON-NLS-1$
	        sb.append("\n}\n"); //$NON-NLS-1$
	        return sb.toString();
		}

2. The moment (1) is done, we notice that org.eclipse.jdt.internal.compiler.ast.SwitchStatement.RecordPatternNode does not materially differ from its superclass org.eclipse.jdt.internal.compiler.ast.SwitchStatement.PatternNode except for the toString output mentioning "[RecordPattern node]" in the former case and just [Pattern node] in the latter case. So we may collapse these into just org.eclipse.jdt.internal.compiler.ast.SwitchStatement.PatternNode by simply tweaking the toString method to read "[" + (this.type.isRecord() ? "Record" : "") + "Pattern node]" - thereby deleting the whole class org.eclipse.jdt.internal.compiler.ast.SwitchStatement.RecordPatternNode (suitably changing other places to take cognizance of this merging of classes)

3. Next turning our attention to org.eclipse.jdt.internal.compiler.ast.SwitchStatement.NodeVisitor, we may observe readily that none of the four endVisit methods are of any relevance since these are NOT overridden anywhere and the base implementations are pure boiler plate nop methods. Let us delete these endVisit methods and their invocations.

4. Next we observe that none of org.eclipse.jdt.internal.compiler.ast.SwitchStatement.NodeVisitor.visit(Node), org.eclipse.jdt.internal.compiler.ast.SwitchStatement.NodeVisitor.visit(PatternNode), and org.eclipse.jdt.internal.compiler.ast.SwitchStatement.NodeVisitor.visit(RNode) are overridden anywhere and the base implementation simply returns true. Let us inline these methods into their call site and delete the originals and also address any warnings arising out of inlining (by cleaning up dead code, wasteful looping etc). This renders the entire method org.eclipse.jdt.internal.compiler.ast.SwitchStatement.TNode.traverse(NodeVisitor) to become empty/nop and so we can delete this method too.

5. This leaves org.eclipse.jdt.internal.compiler.ast.SwitchStatement.NodeVisitor reading:

abstract class NodeVisitor {
		public boolean visit(TNode node) {
			return true;
		}
}

since the only class that extends org.eclipse.jdt.internal.compiler.ast.SwitchStatement.NodeVisitor is org.eclipse.jdt.internal.compiler.ast.SwitchStatement.CoverageCheckerVisitor and this class overrides the visit method, we may collapse these two classes into just the latter by deleting the former class and replaces all references to NodeVisitor with CoverageCheckerVisitor

6. Next we make org.eclipse.jdt.internal.compiler.ast.SwitchStatement.Node abstract.

7. Noticing that org.eclipse.jdt.internal.compiler.ast.SwitchStatement.RNode.addPattern(RecordPattern) is not overridden and has a single call site, we inline and delete the method.

8. Next simply the toString methods to just use string concatenation and not the elaborately ceremonious string builder adds.

[srikanth-sankaran]

This reduces about 100 LOC - I must say I still haven't understood this code 😆 but the above documentation is proof that new simplified code does the same thing as old code without additional 100 lines of boiler plate