Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GROOVY-11263: Analyze dead code #2023

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

GROOVY-11263: Analyze dead code #2023

wants to merge 6 commits into from

Conversation

daniellansun
Copy link
Contributor


@Override
protected SourceUnit getSourceUnit() {
return sourceUnit;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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.

Comment on lines 314 to 316
if (null == sourceUnit) return;
GroovyClassVisitor visitor = new DeadCodeAnalyzer(sourceUnit);
visitor.visitClass(node);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@daniellansun
Copy link
Contributor Author

@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
Copy link
Member

eric-milles commented Jan 3, 2024

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
Copy link
Contributor Author

daniellansun commented Jan 3, 2024

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.

Comment on lines +69 to +71
/** Optimization Option for enabling dead code analysis */
public static final String ANALYZE_DEAD_CODE = "analyzeDeadCode";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Comment on lines +475 to +476
handleOptimizationOption(ANALYZE_DEAD_CODE, getSystemPropertySafe("groovy.analyze.deadcode", "true"));

Copy link
Member

@eric-milles eric-milles Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants