You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "daniellansun (via GitHub)" <gi...@apache.org> on 2023/12/31 20:02:56 UTC
[PR] GROOVY-11263: Analyze dead code [groovy]
daniellansun opened a new pull request, #2023:
URL: https://github.com/apache/groovy/pull/2023
https://issues.apache.org/jira/browse/GROOVY-11263
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "eric-milles (via GitHub)" <gi...@apache.org>.
eric-milles commented on code in PR #2023:
URL: https://github.com/apache/groovy/pull/2023#discussion_r1445353482
##########
src/main/java/org/codehaus/groovy/control/CompilerConfiguration.java:
##########
@@ -469,6 +472,8 @@ public CompilerConfiguration() {
handleOptimizationOption(GROOVYDOC, getSystemPropertySafe("groovy.attach.groovydoc"));
handleOptimizationOption(RUNTIME_GROOVYDOC, getSystemPropertySafe("groovy.attach.runtime.groovydoc"));
handleOptimizationOption(PARALLEL_PARSE, getSystemPropertySafe("groovy.parallel.parse", "true"));
+ handleOptimizationOption(ANALYZE_DEAD_CODE, getSystemPropertySafe("groovy.analyze.deadcode", "true"));
+
Review Comment:
`"groovy.branch.analysis"` or `"groovy.deadCode.analysis"`? Are you sure we want to default to enabled right out the gate?
Spurious extra line added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "eric-milles (via GitHub)" <gi...@apache.org>.
eric-milles commented on PR #2023:
URL: https://github.com/apache/groovy/pull/2023#issuecomment-1881861904
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "eric-milles (via GitHub)" <gi...@apache.org>.
eric-milles commented on PR #2023:
URL: https://github.com/apache/groovy/pull/2023#issuecomment-1873409213
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "blackdrag (via GitHub)" <gi...@apache.org>.
blackdrag commented on code in PR #2023:
URL: https://github.com/apache/groovy/pull/2023#discussion_r1438940188
##########
src/main/java/org/codehaus/groovy/classgen/DeadCodeAnalyzer.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.classgen;
+
+import org.codehaus.groovy.ast.ClassCodeVisitorSupport;
+import org.codehaus.groovy.ast.stmt.BlockStatement;
+import org.codehaus.groovy.ast.stmt.BreakStatement;
+import org.codehaus.groovy.ast.stmt.ContinueStatement;
+import org.codehaus.groovy.ast.stmt.ReturnStatement;
+import org.codehaus.groovy.ast.stmt.Statement;
+import org.codehaus.groovy.control.SourceUnit;
+
+/**
+ * Analyze AST for dead code
+ *
+ * @since 5.0.0
+ */
+public class DeadCodeAnalyzer extends ClassCodeVisitorSupport {
+
+ private final SourceUnit sourceUnit;
+
+ public DeadCodeAnalyzer(final SourceUnit sourceUnit) {
+ this.sourceUnit = sourceUnit;
+ }
+
+ @Override
+ protected SourceUnit getSourceUnit() {
+ return sourceUnit;
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on PR #2023:
URL: https://github.com/apache/groovy/pull/2023#issuecomment-1875821378
We can add a option to prevent or warn dead code or disable check totally.
Here is the table to show how mainstream programming language 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 at compile time. |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on code in PR #2023:
URL: https://github.com/apache/groovy/pull/2023#discussion_r1438949628
##########
src/main/java/org/codehaus/groovy/classgen/DeadCodeAnalyzer.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.classgen;
+
+import org.codehaus.groovy.ast.ClassCodeVisitorSupport;
+import org.codehaus.groovy.ast.stmt.BlockStatement;
+import org.codehaus.groovy.ast.stmt.BreakStatement;
+import org.codehaus.groovy.ast.stmt.ContinueStatement;
+import org.codehaus.groovy.ast.stmt.ReturnStatement;
+import org.codehaus.groovy.ast.stmt.Statement;
+import org.codehaus.groovy.control.SourceUnit;
+
+/**
+ * Analyze AST for dead code
+ *
+ * @since 5.0.0
+ */
+public class DeadCodeAnalyzer extends ClassCodeVisitorSupport {
+
+ private final SourceUnit sourceUnit;
+
+ public DeadCodeAnalyzer(final SourceUnit sourceUnit) {
+ this.sourceUnit = sourceUnit;
+ }
+
+ @Override
+ protected SourceUnit getSourceUnit() {
+ return sourceUnit;
Review Comment:
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));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "eric-milles (via GitHub)" <gi...@apache.org>.
eric-milles commented on PR #2023:
URL: https://github.com/apache/groovy/pull/2023#issuecomment-1875712756
> 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?
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "eric-milles (via GitHub)" <gi...@apache.org>.
eric-milles commented on code in PR #2023:
URL: https://github.com/apache/groovy/pull/2023#discussion_r1445353482
##########
src/main/java/org/codehaus/groovy/control/CompilerConfiguration.java:
##########
@@ -469,6 +472,8 @@ public CompilerConfiguration() {
handleOptimizationOption(GROOVYDOC, getSystemPropertySafe("groovy.attach.groovydoc"));
handleOptimizationOption(RUNTIME_GROOVYDOC, getSystemPropertySafe("groovy.attach.runtime.groovydoc"));
handleOptimizationOption(PARALLEL_PARSE, getSystemPropertySafe("groovy.parallel.parse", "true"));
+ handleOptimizationOption(ANALYZE_DEAD_CODE, getSystemPropertySafe("groovy.analyze.deadcode", "true"));
+
Review Comment:
`"groovy.branch.analysis"` or `"groovy.dead.code.analysis"`? Are you sure we want to default to enabled right out the gate?
Spurious extra line added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun closed pull request #2023: GROOVY-11263: Analyze dead code
URL: https://github.com/apache/groovy/pull/2023
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "eric-milles (via GitHub)" <gi...@apache.org>.
eric-milles commented on code in PR #2023:
URL: https://github.com/apache/groovy/pull/2023#discussion_r1445349235
##########
src/main/java/org/codehaus/groovy/control/CompilerConfiguration.java:
##########
@@ -66,6 +66,9 @@ public class CompilerConfiguration {
/** Joint Compilation Option for enabling generating stubs in memory. */
public static final String MEM_STUB = "memStub";
+ /** Optimization Option for enabling dead code analysis */
+ public static final String ANALYZE_DEAD_CODE = "analyzeDeadCode";
+
Review Comment:
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"`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "eric-milles (via GitHub)" <gi...@apache.org>.
eric-milles commented on code in PR #2023:
URL: https://github.com/apache/groovy/pull/2023#discussion_r1439064503
##########
src/main/java/org/codehaus/groovy/classgen/Verifier.java:
##########
@@ -302,6 +310,12 @@ private void checkFinalVariables(final ClassNode node) {
visitor.visitClass(node);
}
+ private void checkDeadCode(final ClassNode node) {
+ if (null == sourceUnit) return;
+ GroovyClassVisitor visitor = new DeadCodeAnalyzer(sourceUnit);
+ visitor.visitClass(node);
Review Comment:
I think you can get to the `SourceUnit` through the `ClassNode` without having to change `Verifier` all around.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11263: Analyze dead code [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on PR #2023:
URL: https://github.com/apache/groovy/pull/2023#issuecomment-1873540757
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org