GROOVY-11263: Analyze dead code

[daniellansun]

https://issues.apache.org/jira/browse/GROOVY-11263

[blackdrag]

why do you need the source unit? I think there is also a conceptional mistake. I may be wrong though, but do we not reuse the Verifier for multiple source units? If yes, then having the source unit in the constructor there is wrong. If you really need the source unit you should use a setter in case of the Verifier I think

[daniellansun]

The method org.codehaus.groovy.ast.ClassCodeVisitorSupport#addError needs source unit, I use addError to collect and report errors.

    @Override
    public void addError(final String error, final ASTNode node) {
        getSourceUnit().addErrorAndContinue(new SyntaxException(error + '\n', node));
    }

[eric-milles]

Besides the extra class bytecode, is there a strong reason for this? The issue ticket pops up out of the blue without much explanation.

There are so many possible paths that I think this will be another feature that gets you a quick 80% of the way and then will have issue tickets forever with uncovered scenarios. Would it be easier to use a bytecode path analyzer? Something that has been through research and proving phases?

I think ASM does some of this analysis already but does not provide any API to ask if a path is dead -- but it does know internally using path analysis. It replaces dead code with NOP instructions. Maybe just a NOP pruning is in order.

[eric-milles]

I think you can get to the SourceUnit through the ClassNode without having to change Verifier all around.

[daniellansun]

@eric-milles
As we all know, source code is meant for developers to read, and the less redundant code there is, the more developer-friendly it becomes.

Admittedly, it's challenging to cover all scenarios of dead code, but at least we can support some common ones and gradually improve, which aligns with the principle of program evolution.

As for ASM's analysis of dead code, at least I haven't seen any relevant API it offers. If you have any new findings, please share them with us. Thanks in advance.

[eric-milles]

source code is meant for developers to read, and the less redundant code there is, the more developer-friendly it becomes.

So now you have a benefit statement that you can put in the ticket. It would be nice to have a period of review on the problem statement and cost-benefit analysis before being forced to review code in hand.

If a user does not care about the extra code to read and the extra bytecode generated, is there a way to turn this off?

Have you still left it as a compiler error or did you soften it to a warning? If analysis mis-identifies some code as dead code and fails compilation, what can a user do to get their previously-working code to compile?

What if a post-verify transform changes the code's structure? You have chosen a specific compile phase to do the analysis with no explanation as to why this is the step to use.

If you do not address such concerns, then why not leave this out as a global transform that you apply to your own code? Why must it be core code from the very start?

It's a -1 from me unless you can more carefully spell out the problem definition and the solution ramifications.

[daniellansun]

We can add an option to switch off dead code analysis. Also, dead code analysis, as its name shown, it just traverses AST and does not change the AST, so no tranforming changes involved.

Here is the table to show how some mainstream programming languages to handle dead code. Most of them prevent or warn the dead code, so I think it's good choice for Groovy to align with most of them.

Language	Allows Consecutive return (Dead Code)	Compiler/Interpreter Behavior
Java	No	Marks as unreachable code, may cause a compile error.
C++	Yes	Might warn but allows compilation.
C#	No	Identifies unreachable code, causes a compile error.
Python	Yes	Does not prevent dead code, allows execution.
Scala	Yes	Might issue a warning but allows compilation.
Kotlin	No	Detects unreachable code, causes a compile error.
JavaScript	Yes	Does not check for dead code.

[eric-milles]

Can you move this adjacent to the other optimization options and add an @since tag?

I'm partial to naming it DEAD_CODE_ANALYSIS and "deadCodeAnalysis".

[eric-milles]

"groovy.branch.analysis" or "groovy.deadCode.analysis"? Are you sure we want to default to enabled right out the gate?

Spurious extra line added.

[eric-milles]

dead code analysis, as its name shown, it just traverses AST and does not change the AST, so no tranforming [sic] changes involved.

This was understood. The point is that if you add a compiler error you may fail code that previously compiled. A compiler warning allows the user to be notified but continue using code unchanged. Also, you scan the AST at a specific point in time. If later AST transformations occur that address the dead code scenarios, you have false positive.

I'm just trying to have you describe your reasoning for when to do the analysis. Even the class generator does a bit of instruction re-ordering that may or may not introduce dead code paths. It seems the goal of this change is to identify any dead statements explicitly represented in the source file. If that is indeed the case, it would be good to state all of this in the original problem description.