Bug 365992 - [builder] [null] Change of nullness for a parameter doesn't trigger a build for the files that call the method
Summary: [builder] [null] Change of nullness for a parameter doesn't trigger a build f...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.8   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 3.8 M5   Edit
Assignee: Stephan Herrmann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 366341 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-12-08 04:19 EST by Satyam Kandula CLA
Modified: 2012-01-23 02:04 EST (History)
3 users (show)

See Also:
amj87.iitr: review+

Attachments
test & fix (11.78 KB, patch)
2011-12-13 11:05 EST, Stephan Herrmann CLA 		no flags Details | Diff
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 Satyam Kandula CLA 2011-12-08 04:19:24 EST 
Consider two classes
#######
public class Test1 {
	public void foo() {
		new Test2().bar(null);
	}
}
#######
import org.eclipse.jdt.annotation.NonNull;
public class Test2 {
	public void bar(@NonNull String str) {}		
}
#####
With 'Build Automatically' turned on, change in the nullness of the parameter 'str' in bar() should rebuild Test1. This is not happening now.
Comment 1 Srikanth Sankaran CLA 2011-12-08 04:23:21 EST 
Good catch Satyam. See that it works alright for null annotation change
on the method return type. (otherwise changes in @Deprecated wouldn't
trigger an incremental build.)
Comment 2 Stephan Herrmann CLA 2011-12-08 09:09:56 EST 
Me thinks this could be an omission in 
org.eclipse.jdt.internal.compiler.classfmt.ClassFileReader.hasStructuralMethodChanges(MethodInfo, MethodInfo)

(not handling parameter annotations when comparing methods).

Does this sound like the right spot to dive into?
Comment 3 Srikanth Sankaran CLA 2011-12-08 10:02:21 EST 
(In reply to comment #2)
> Me thinks this could be an omission in 
> org.eclipse.jdt.internal.compiler.classfmt.ClassFileReader.hasStructuralMethodChanges(MethodInfo,
> MethodInfo)
> 
> (not handling parameter annotations when comparing methods).
> 
> Does this sound like the right spot to dive into?

Yes, I believe you are on the right track.
Comment 4 Srikanth Sankaran CLA 2011-12-08 10:04:09 EST 
See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=149768
Comment 5 Stephan Herrmann CLA 2011-12-09 19:41:07 EST 
(In reply to comment #4)
> See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=149768

Thanks for the pointer. Specifically an example how this kind of issue
can best be tested comes very handy.
Comment 6 Srikanth Sankaran CLA 2011-12-11 21:24:12 EST 
*** Bug 366341 has been marked as a duplicate of this bug. ***
Comment 7 Srikanth Sankaran CLA 2011-12-11 21:26:32 EST 
Per bug# 366341 comment #0, adding annotations to return type doesn't
trigger incremental build. This was supposed to already work.

Stephan, please remember to check and add junits for this scenario too,
TIA.
Comment 8 Stephan Herrmann CLA 2011-12-13 11:05:09 EST 
Created attachment 208319 [details]
test & fix

The test checks adding/removing/changing a null annotation
on a method parameter and on the method itself (return type).

As expected a small addition in
ClassFileReader.hasStructuralMethodChanges(MethodInfo, MethodInfo)
suffices to trigger the desired rebuild.

The patch also contains a rename requested in bug 365387 comment 15
(in item (21)). I hope the new name is clearer
(the proposed name getTotalParameterCount wouldn't fit for the 
implementation in MethodInfo, which constantly returns 0).
Comment 9 Srikanth Sankaran CLA 2011-12-13 20:29:45 EST 
(In reply to comment #8)

> The patch also contains a rename requested in bug 365387 comment 15
> (in item (21)). I hope the new name is clearer
> (the proposed name getTotalParameterCount wouldn't fit for the 
> implementation in MethodInfo, which constantly returns 0).

It is better, still not totally apt, but I think we can settle for your
choice. Ayush, please review, TIA.
Comment 10 Ayushman Jain CLA 2011-12-21 01:36:09 EST 
Patch looks good. Change in defaults also causes incremental build which is good.
Comment 11 Stephan Herrmann CLA 2011-12-22 07:04:40 EST 
Released for 3.8 M5 via commit d2fd0a411735601d08912dc407d669f4cabc6555
Comment 12 Srikanth Sankaran CLA 2012-01-23 02:04:40 EST 
Verified for 3.8 M5 using build id: I20120122-2000