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 #5364: changed RequireThis kept track of the frame being examined #5364

Merged
merged 1 commit into from Dec 25, 2017

Conversation

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Dec 18, 2017

Identified at #5307 (comment) .
We assign current on a visit, but we don't reset it when we leave a token. So check thinks we are still in the previous frame, when we are back in the parent.

Here is proof:

$ cat TestClass.java
class NestedFrames {
    int a = 0;

    public int oneReturnInMethod2() {
        for (int i = 0; i < 10; i++) {
            int a = 1;
            if (a != 2 && true) {
                if (true | false) {
                    if (a - a != 0) {
                        a += 1;
                    }
                }
            }
        }
        return a + a * a;
    }
}

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

Expecting a violation for return a + a * a as a is not the local variable a, but the class field and thus should be a violation for this.


Regression: http://rveach.no-ip.org/checkstyle/regression/reports/147/

We are seeing alot of differences.
http://rveach.no-ip.org/checkstyle/regression/reports/147/sevntu-checkstyle/index.html#A7
These are new violations, added in the test.
http://rveach.no-ip.org/checkstyle/regression/reports/147/guava-mvnstyle/index.html#A259
These violations are removed as list is a local variable, not the anonymous class' same variable name.

All other differences are changes in violation messages from InnerClass.this to just this.

rnveach added a commit to rnveach/checkstyle that referenced this pull request Dec 18, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 18, 2017

Member

[ERROR] Forbidden field access: java.lang.System#out [prints to System.out; should only be used for debugging, not in production code]
[ERROR] in com.puppycrawl.tools.checkstyle.checks.coding.requirethis.NestedFrames (InputRequireThisEnumInnerClassesAndBugs.java:158)

I'm not sure why this is still coming up when we disabled all input files.
I assume it is going by the class name and not the file name.

Member

rnveach commented Dec 18, 2017

[ERROR] Forbidden field access: java.lang.System#out [prints to System.out; should only be used for debugging, not in production code]
[ERROR] in com.puppycrawl.tools.checkstyle.checks.coding.requirethis.NestedFrames (InputRequireThisEnumInnerClassesAndBugs.java:158)

I'm not sure why this is still coming up when we disabled all input files.
I assume it is going by the class name and not the file name.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 18, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5364   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16203   16208    +5     
  Branches     3700    3701    +1     
======================================
+ Hits        16203   16208    +5
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 b02eed8...9fe97c9. Read the comment docs.

codecov-io commented Dec 18, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5364   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16203   16208    +5     
  Branches     3700    3701    +1     
======================================
+ Hits        16203   16208    +5
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 b02eed8...9fe97c9. Read the comment docs.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 25, 2017

Member

@rnveach ,

if this is kind of bug fix, why not to report it as usually do for bugs: "javac ... cat .... cat ... java -jar ...."

from regression
http://rveach.no-ip.org/checkstyle/regression/reports/147/sevntu-checkstyle/index.html#A3

- Reference to instance variable 'enumConstants' needs "Frame.this.".
+ Reference to instance variable 'enumConstants' needs "this.".

Why do not think new behavior is better ?

Member

romani commented Dec 25, 2017

@rnveach ,

if this is kind of bug fix, why not to report it as usually do for bugs: "javac ... cat .... cat ... java -jar ...."

from regression
http://rveach.no-ip.org/checkstyle/regression/reports/147/sevntu-checkstyle/index.html#A3

- Reference to instance variable 'enumConstants' needs "Frame.this.".
+ Reference to instance variable 'enumConstants' needs "this.".

Why do not think new behavior is better ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 25, 2017

Member

why not to report it as usually do for bugs

Updated first description with such example.

http://rveach.no-ip.org/checkstyle/regression/reports/147/sevntu-checkstyle/index.html#A3
Reference to instance variable 'enumConstants' needs "Frame.this.".

This change is correct.
enumConstants on line 836 is not in an anonymous class. It is in the same class as the field enumConstants so referencing the class name prior to this is completely redundant. If user wants to define it as such, check shouldn't prevent them from doing that.

Why do not think new behavior is better ?

I do think new behavior is better.

Member

rnveach commented Dec 25, 2017

why not to report it as usually do for bugs

Updated first description with such example.

http://rveach.no-ip.org/checkstyle/regression/reports/147/sevntu-checkstyle/index.html#A3
Reference to instance variable 'enumConstants' needs "Frame.this.".

This change is correct.
enumConstants on line 836 is not in an anonymous class. It is in the same class as the field enumConstants so referencing the class name prior to this is completely redundant. If user wants to define it as such, check shouldn't prevent them from doing that.

Why do not think new behavior is better ?

I do think new behavior is better.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 25, 2017

Member

this fix will make reopen on #2239 , where such context was introduced. So we have conflict of interests.

Your issue fix should not be related change of output message.
Can we keep context for message reporting?

Member

romani commented Dec 25, 2017

this fix will make reopen on #2239 , where such context was introduced. So we have conflict of interests.

Your issue fix should not be related change of output message.
Can we keep context for message reporting?

@romani

discussion ...

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 25, 2017

Member

@romani

this fix will make reopen on #2239

Please look over class structure more closely.

#2239

field foo is inside class Test, but referenced inside inner class Nested.

http://rveach.no-ip.org/checkstyle/regression/reports/147/sevntu-checkstyle/index.html#A3

field enumConstants is inside inner class Frame and referenced from same inner class Frame.
There is nothing being referenced from StaticMethodCandidateCheck in violation.

These 2 instances are un-related and so this fix will not reopen #2239 .

My change is related to not knowing we switched frames. Old processing thought we were inside an inner class of Frame because it didn't know we left the anonymous class Predicate, hence why it wanted that violation message. Specifying the class name when referencing this in this scenario is not needed since we are in the same class.

Member

rnveach commented Dec 25, 2017

@romani

this fix will make reopen on #2239

Please look over class structure more closely.

#2239

field foo is inside class Test, but referenced inside inner class Nested.

http://rveach.no-ip.org/checkstyle/regression/reports/147/sevntu-checkstyle/index.html#A3

field enumConstants is inside inner class Frame and referenced from same inner class Frame.
There is nothing being referenced from StaticMethodCandidateCheck in violation.

These 2 instances are un-related and so this fix will not reopen #2239 .

My change is related to not knowing we switched frames. Old processing thought we were inside an inner class of Frame because it didn't know we left the anonymous class Predicate, hence why it wanted that violation message. Specifying the class name when referencing this in this scenario is not needed since we are in the same class.

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

@romani romani merged commit 36fdb1b into checkstyle:master Dec 25, 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 5788 status is SUCCESS.
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to b02eed8
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

@rnveach rnveach deleted the rnveach:pr_5307_5 branch Dec 25, 2017

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