Bug 335780 - Compiler says a method can be potentially static but this method contains 'this'
Summary: Compiler says a method can be potentially static but this method contains 'this'
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 3.7 M6   Edit
Assignee: Ayushman Jain CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-29 19:28 EST by Rémi Forax CLA
Modified: 2011-03-07 11:24 EST (History)
2 users (show)

See Also:
srikanth_sankaran: review+

Attachments
proposed fix v1.0 + regression tests (4.29 KB, patch)
2011-02-01 04:04 EST, Ayushman Jain CLA 		no flags Details | Diff
updated patch (4.25 KB, patch)
2011-02-04 03:19 EST, Ayushman Jain CLA 		no flags Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Rémi Forax CLA 2011-01-29 19:28:52 EST 
Build Identifier: A method using 'this' can't be potentially static

The latest beta of indigo introduces a new static analysis that detects
is a method can be potentially static.
But if I use 'this' in that method, it should not be tagged as potentially static.

public class MethodStaticEclipseBug {
  public void m() {
    Foo.m(this);
  }
  
  static class Foo {
    static void m(MethodStaticEclipseBug bug) {
      // do something cool here
    }
  }
}


Reproducible: Always

Steps to Reproduce:
Type the above code in eclipse.
the warning at line 3 is a false positive.
Comment 1 Srikanth Sankaran CLA 2011-01-30 22:55:59 EST 
Ayush, Please take a look.
Comment 2 Ayushman Jain CLA 2011-02-01 04:04:27 EST 
Created attachment 188029 [details]
proposed fix v1.0 + regression tests

This patch makes sure that the warning is not given for methods where an explicit reference to 'this' takes place.

Running all tests
Comment 3 Ayushman Jain CLA 2011-02-03 01:47:39 EST 
Srikanth, please review. Thanks!
Comment 4 Srikanth Sankaran CLA 2011-02-04 02:55:31 EST 
Consistent with the rest of the code in ThisReference.java,
you could have used the method isImplicitThis() instead of
using its internals.

In the test case comment that says "// warn" is misplaced.

Otherwise it looks good.
Comment 5 Ayushman Jain CLA 2011-02-04 03:19:35 EST 
Created attachment 188296 [details]
updated patch

patch incorporating above comments
Comment 6 Ayushman Jain CLA 2011-02-04 03:21:39 EST 
Released in HEAD for 3.7M6
Comment 7 Rémi Forax CLA 2011-02-08 10:35:20 EST 
(In reply to comment #5)
> Created attachment 188296 [details]
> updated patch
> 
> patch incorporating above comments

I think that this slight variation is not cover by the proposed patch:
public class EclipseImplicitThisBug {
  class A {
  }
  
  void m() {
    new A();   // here this is captured to create the non static inner class
  }
}

Rémi
Comment 8 Rémi Forax CLA 2011-02-08 10:38:42 EST 
(In reply to comment #7)
> (In reply to comment #5)
> > Created attachment 188296 [details] [details]
> > updated patch
> > 
> > patch incorporating above comments
> 
> I think that this slight variation is not cover by the proposed patch:
> public class EclipseImplicitThisBug {
>   class A {
>   }
> 
>   void m() {
>     new A();   // here this is captured to create the non static inner class
>   }
> }
> 
> Rémi

Already reported as 335845.
https://bugs.eclipse.org/bugs/show_bug.cgi?id=335845

Rémi
Comment 9 Olivier Thomann CLA 2011-03-07 11:24:24 EST 
Verified for 3.7M6.