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

Pull #5392: fixed RequireThisCheck and for loop variable handling #5392

Merged
merged 1 commit into from Dec 26, 2017

Conversation

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Dec 26, 2017

Identified at #5307 (comment)
For loop variables are seen in the wrong scope. They are thought to be in the base method definition when they are actually scoped inside the loop itself.

You can clearly see the scope change at rnveach@85c1003#diff-6b1f89f1bde0470d4bd42580d1aa8bfb .

$ cat TestClass.java
public class TestClass {
    int b = 0;

    void method() {
        for (int b = 0; b < 10; b++) {
        }
        return b + b * b;
    }
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
<module name="RequireThisCheck">
<property name="validateOnlyOverlapping" value="false" />
</module>
    </module>
</module>

$ java -jar checkstyle-8.5-all.jar -c TestConfig.xml TestClass.java
Starting audit...
Audit done.

No violation even though variable b is the field variable.

First regression: http://rveach.no-ip.org/checkstyle/regression/reports/148/
Showed a new NPE which I fixed.

I will provide newer regression.

@@ -617,7 +627,8 @@ private static boolean isReturnedVariable(AbstractFrame currentFrame, DetailAST
private boolean canBeReferencedFromStaticContext(DetailAST ident) {
AbstractFrame variableDeclarationFrame = findFrame(ident, false);
boolean staticInitializationBlock = false;
while (variableDeclarationFrame.getType() == FrameType.BLOCK_FRAME) {
while (variableDeclarationFrame.getType() == FrameType.BLOCK_FRAME
|| variableDeclarationFrame.getType() == FrameType.FOR_FRAME) {

This comment has been minimized.

@rnveach

rnveach Dec 26, 2017

Member

This code is for NPE noticied in regression. I will need to see if catch frames should be added to this as well.

@rnveach

rnveach Dec 26, 2017

Member

This code is for NPE noticied in regression. I will need to see if catch frames should be added to this as well.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 26, 2017

Codecov Report

Merging #5392 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5392   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16218   16227    +9     
  Branches     3707    3708    +1     
======================================
+ Hits        16218   16227    +9
Impacted Files Coverage Δ
...ols/checkstyle/checks/coding/RequireThisCheck.java 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36fdb1b...e477113. Read the comment docs.

codecov-io commented Dec 26, 2017

Codecov Report

Merging #5392 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5392   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16218   16227    +9     
  Branches     3707    3708    +1     
======================================
+ Hits        16218   16227    +9
Impacted Files Coverage Δ
...ols/checkstyle/checks/coding/RequireThisCheck.java 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36fdb1b...e477113. Read the comment docs.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 26, 2017

Member

New regression: http://rveach.no-ip.org/checkstyle/regression/reports/149/

Some of the violation differences weren't expected, but they seem right as I understand the rules.
Example:
http://rveach.no-ip.org/checkstyle/regression/reports/149/elasticsearch/index.html#A2
http://rveach.no-ip.org/checkstyle/regression/reports/149/elasticsearch/xref/elasticsearch/src/main/java/org/elasticsearch/percolator/QueryCollector.java.html#L84
This variable isn't part of the for loop, or in different scopes by the for loop, but it shouldn't require this right?

Member

rnveach commented Dec 26, 2017

New regression: http://rveach.no-ip.org/checkstyle/regression/reports/149/

Some of the violation differences weren't expected, but they seem right as I understand the rules.
Example:
http://rveach.no-ip.org/checkstyle/regression/reports/149/elasticsearch/index.html#A2
http://rveach.no-ip.org/checkstyle/regression/reports/149/elasticsearch/xref/elasticsearch/src/main/java/org/elasticsearch/percolator/QueryCollector.java.html#L84
This variable isn't part of the for loop, or in different scopes by the for loop, but it shouldn't require this right?

@rnveach rnveach requested a review from romani Dec 26, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
Member

romani commented Dec 26, 2017

@romani romani merged commit 253de63 into checkstyle:master Dec 26, 2017

9 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
IDEA Inspections Pull Request (Checkstyle) TeamCity build finished
Details
Shippable Run 5850 status is SUCCESS.
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 36fdb1b
Details
continuous-integration/Distelli
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
wercker/build Wercker pipeline passed
Details

@romani romani added this to the 8.6 milestone Dec 26, 2017

@rnveach rnveach deleted the rnveach:pr_5307_6 branch Dec 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment