Bug 330445 - [1.4][1.5][compiler] Properties doesn't match Map<String, String> in 1.4 compliance mode
Summary: [1.4][1.5][compiler] Properties doesn't match Map<String, String> in 1.4 comp...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 3.7 M4   Edit
Assignee: Srikanth Sankaran CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-17 07:55 EST by Jeff McAffer CLA
Modified: 2010-12-07 12:02 EST (History)
8 users (show)

See Also:


Attachments
First draft (953 bytes, patch)
2010-11-17 14:03 EST, Olivier Thomann CLA
no flags Details | Diff
Proposed fix + regression test (2.95 KB, patch)
2010-11-17 15:07 EST, Olivier Thomann CLA
no flags Details | Diff
Revised patch under test (2.94 KB, patch)
2010-11-18 02:57 EST, Srikanth Sankaran CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff McAffer CLA 2010-11-17 07:55:36 EST
in i1116-0800

in the PDE target code there is a call as follows
	PermissiveSlicer slicer = new PermissiveSlicer(profile, new Properties(), true, false, true, false, false);
In the past (a week ago) this compiled fine.  Something has changed such that the use of new Properties now causes a compile error because it apparently does not match Map<String, String>.  I'm assuming that this runs fine since that PDE code is in HEAD but do wonder what the path here is supposed to be and why this is suddenly showing as a compile error.  

Note that there is a similar problem with IProfileRegistry.addProfile(String, Map<String, String>).  Seems calling with a Properties object used to compile fine but not any more.
Comment 1 DJ Houghton CLA 2010-11-17 11:38:13 EST
Strange, that class hasn't changed since last March. Any ideas Curtis?
Comment 2 Curtis Windatt CLA 2010-11-17 12:09:42 EST
My guess is this is more weirdness due to the 1.4/1.5 gap between PDE and p2.  Was this actually a compile error that affected things at runtime or was it a development time error?  There are several places where we use p2 APIs that use generics and errors show up in PDE.
Comment 3 Curtis Windatt CLA 2010-11-17 12:26:13 EST
(In reply to comment #2)
> My guess is this is more weirdness due to the 1.4/1.5 gap between PDE and p2. 
> Was this actually a compile error that affected things at runtime or was it a
> development time error?  There are several places where we use p2 APIs that use
> generics and errors show up in PDE.

Looks like the compile error breaks things at runtime.  cc'ing Olivier as he may be able to tell us what has changed in the compiler in the past week.
Comment 4 Olivier Thomann CLA 2010-11-17 12:31:20 EST
(In reply to comment #0)
> in i1116-0800
Could you please provide the jdt.core id?

(In reply to comment #0)
> In the past (a week ago) this compiled fine.  Something has changed such that
Could you please provide the jdt.core id?

I would need the jdt.core id when it used to work and when it stopped working.
Thanks.
Comment 5 Curtis Windatt CLA 2010-11-17 12:40:03 EST
Worked in 3.7.0.v_B21
Broken in 3.7.0.v_B24

I'll have to try some other versions to narrow it down further.
Comment 6 Olivier Thomann CLA 2010-11-17 12:46:11 EST
Might be related to the changes released in B23 for 1.4/1.5 mixed mode issues.
Comment 7 Olivier Thomann CLA 2010-11-17 13:03:47 EST
Moving to JDT/Core as these errors occur when compiling the code using a 1.6 library.
Comment 8 Olivier Thomann CLA 2010-11-17 13:56:32 EST
The problem comes from:
new Properties() not matching the Map<String, String> argument type from the constructor.

If you have:

import java.util.Map;

public class Y {
		static void foo(Map<String, String> map) {
		}
}

and this is compiled in 1.5.

Then you define:
import java.util.Properties;

public class X {
    static void bar(Object[] args) {
        Y.foo(new Properties());
    }
}

If you compile using javac 1.6:
javac X.java -source 1.4

it works.
But in source 1.5 or source 1.6, it doesn't compile:
X.java:12: foo(java.util.Map<java.lang.String,java.lang.String>) in Y cannot be applied to (java.util.Properties)
        Y.foo(new Properties());
         ^
1 error

So I think the error makes sense if the code is compiled using 1.5 compliance or 1.6 compliance.
But when compile in 1.4, it should work.
Comment 9 Olivier Thomann CLA 2010-11-17 14:03:04 EST
Created attachment 183330 [details]
First draft

This seems to fix this issue.
I'll run all tests to double-check previous fixes around 1.4/1.5 issues.
Comment 10 Olivier Thomann CLA 2010-11-17 14:18:07 EST
I believe pde.code should fix their code.
The API that is called is expecting a map of string, string and not a Properties object that can potentially contains any kind of Object.

All 3 errors I am getting by compiling pde.core in 1.6 mode are related to this issue.

So yes, the code should compile in 1.4 mode, but it might blow up at runtime. In your code, you are only putting strings inside the properties keys and values so it should work.

We should fix our code to make sure that code compiles fine if the compliance is 1.4 (source <= 1.4 in fact).
Comment 11 Curtis Windatt CLA 2010-11-17 14:47:46 EST
(In reply to comment #10)
> I believe pde.code should fix their code.
> The API that is called is expecting a map of string, string and not a
> Properties object that can potentially contains any kind of Object.

PDE core/ui is 1.4 compliant, so we cannot use generics to specify the contents of the map.
Comment 12 Curtis Windatt CLA 2010-11-17 15:02:16 EST
PDE Code was updated to use a HashMap rather than the Properties class and now will compile in both 1.4 and 1.6.
Comment 13 Olivier Thomann CLA 2010-11-17 15:07:49 EST
Created attachment 183337 [details]
Proposed fix + regression test

Same patch with a regression test.
Comment 14 Jeff McAffer CLA 2010-11-17 15:32:45 EST
Thanks Curtis for tracking down the build info.  Olivier, should we move this to JDT? Feel free to take it if so.
Comment 15 Curtis Windatt CLA 2010-11-17 15:35:56 EST
(In reply to comment #14)
> Thanks Curtis for tracking down the build info.  Olivier, should we move this
> to JDT? Feel free to take it if so.

You're about 6 comments late :)
Comment 16 Olivier Thomann CLA 2010-11-17 15:36:41 EST
(In reply to comment #14)
> Thanks Curtis for tracking down the build info.  Olivier, should we move this
> to JDT? Feel free to take it if so.
Already done :-). The bug we have is that we don't compile in 1.4 mode. In 1.5 or 1.6 our behavior is right.
Comment 17 Jeff McAffer CLA 2010-11-17 15:44:05 EST
sorry about that move comment.  I was reading the thread in segregated mail folders and the thread jumped to a different bucket...  Doh
Comment 18 Srikanth Sankaran CLA 2010-11-18 02:57:16 EST
Created attachment 183366 [details]
Revised patch under test

Olivier, Thanks for the patch, I think it is the right fix.
In the revised patch I just eliminated the erasure operation
on the method argument, while continuing to erase the formal
parameter.
Comment 19 Srikanth Sankaran CLA 2010-11-18 05:46:43 EST
Released in HEAD for 3.7 M4
Comment 20 Markus Keller CLA 2010-11-18 19:15:18 EST
(In reply to comment #18)
> Created an attachment (id=183366) [details] [diff]

This change is causing test failures in JDT/UI (e.g. in MarkOccurrenceTest).

To reproduce that setup, put /org.eclipse.jdt.ui.tests/testresources/rtstubs15.jar on the classpath of a Java project with 1.4 compiler compliance. I don't say that our test project setup is flawless, but this worked fine up to v_B24. It's getting late for me now -- I'll have a closer look tomorrow.
Comment 21 Markus Keller CLA 2010-11-18 19:36:42 EST
(In reply to comment #20)
Sorry, I messed up. The problems in the JDT/UI tests are actually due to the fix for bug 330347. I've filed bug 330631 for that.
Comment 22 Olivier Thomann CLA 2010-12-07 12:02:55 EST
Verified using I20101207-0250 (4.1 I-build)