Community
Participate
Working Groups
We must provide illegal use support for method references: 1. be able to detect illegal references 2. be able to detect illegal instantiation (in the MyObject::new case?)
This bug requires the Java 8-compatible version of ASM - since no references can be extracted from the bytecodes otherwise.
Once we have the new version of ASM up and running we need to consider the four kinds of method references: 1. reference to a static method - ContainingClass::staticMethodName 2. reference to an instance method of a particular object - ContainingObject::instanceMethodName 3. reference to an instance method of an arbitrary object of a particular type - ContainingType::methodName 4. reference to a constructor - ClassName::new I would assume that in all of the above cases we would flag the illegal use (reference) of any of those method if they are restricted.
I will start on this now.
With bug 431290 fixed we can now detect and mark anything that uses invokedynamic. As a POC I implemented marking restricted constructor method references as an example for you Vikas: http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=05fca1cb102ff1121cf42b849bc027211faa4d24 Now that we properly extract and process invokedynamic instructions we *should* get illegal references marked on the enclosing types for free, with the only updates required to properly place the error markers - as you'll notice that is all that was required to mark restricted constructor refs.
Created attachment 241344 [details] screen shot Here is a screen shot showing the marking of illegal constructor method references (for reference)
Created attachment 241510 [details] Method reference marker update Thanks Michael ! I have added some more code to flag the method reference. I had at least 4 other types ( see snap) that were not marked earlier but with some effort, all of them are now marked. I am currently working on this and I hope to have a generic implementation of this today/tomorrow.
Created attachment 241546 [details] Patch to detect method reference - includes Michael's POC also Here is a patch that works well for all the known scenarios for me. But there can be more scenarios. So we need to write the test case also. This patch covers 1) assignment of method reference 2) assignment of constructor new 3) method ref and contructor new in arguments. There are few minor things that needs to be polished out. Next steps 1) Create test cases 2) Polish and finalize this patch. For test cases, I have some some samples but I need to put that in our PDE test framework.
Created attachment 241547 [details] Patch
I have managed to utilise Java7MethodUsageTests to create Java8MethodReferenceUsageTests. This suite currently has only 1 test case which passes both for incremental and full build. I plan to add all the relevant test cases. I will attach to this bug as soon as I complete it. Also I plan to improve the patch attached for fringe cases ( like space between :: and new in some cases - spaces between :: and new work fine for assignment case with the patch attached). They should be trivial to handle. Also refactoring some part of functionality. Meanwhile if there are feedback from the already attached patch, it will also be incorporated.
I have added 5 case of illegal reference for method reference. There is one issue where the problem is reported on different constructors with different parameters whereas the problem message create just mentions the class. Changing the problem message creates further issue because apparently JDT's Problem also uses the same problem message in a different way. I am looking at it.
The issue was to give expected args in the same way as done by api problem creation. For method reference { "MethodReference", typename, "method1()" }, //$NON-NLS-1$ //$NON-NLS-2$ 111 = {1} illegally references method {0}.{2} For constructor I had given "ConstructorReference, typename, "ConstructorReference(String)"}, But given 110 = {1} illegally references constructor {0} I had to modify it to "ConstructorReference(String)", typename, null }, //$NON-NLS-1$ Next steps 1) Make test cases for examples in https://bugs.eclipse.org/bugs/show_bug.cgi?id=431290 where method references can be in arguments for example Arrays.sort(array, MR::mrCompare); 2. See if the patch needs to be changed to accommodate any fringe case.
Created attachment 241632 [details] Method reference patch with test cases 1) Test cases are added now. 2) Junits run fine with the new code. It was expected that way since we do additional manipulation based new java 8 items. Hence earlier calculation should be unaffected. Both ApiToolsPluginTestSuite and ApiToolsTestSuite. The new tests were confirmed to be added. 3) There was an issue about marking the errors. Now only the method would be marked ( in consistent with normal method calls) except in case of constructor::new where the entire string from classname to new will be marked. Just marking ::new or <String>new would be confusing. 4) Lot of code from previous patch was deemed to be unnecessary and hence removed - eg changing the start to include the classname and determining if it is constructor. ( we already have a field which tells us that).
License etc added for few files but few changes in comment. This is the gerrit patch https://git.eclipse.org/r/#/c/24523/
The only thing I can think of we should add the test only if Java 8 environment is running and not + if (ProjectUtils.isJava8Compatible()) { + classes.add(Java8MethodConstRefUsageTests.class); + + } We need the java 8 env identification code similar to that added in https://git.eclipse.org/r/#/c/23772/ Else this test will fail on Java 7.
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=a642552ccc53ba8536424fdde41b181be8502a9a I rebased the change manually as our changes to fix the test failures caused a lot of conflicts. The tests are passing locally and with the new test structure they shouldn't be run if the whole suite is run on J2SE-1.7 or lower. I need to do some additional review to ensure the problem markers and messages are all correct.
Everything is good when using @noreference restrictions. Note that the tests only cover restrictions on individual methods. I tested with a @NoReference annotation on the type and the problems were generated correctly. However, because you can use method reference to invoke a constructor, we should also be checking for @noinstantiate restrictions. Taking an example from the tests, if ConstructorReferences had @noinstantiate while the constructors had no restrictions (currently they are noreference) we should be marking the :::new() as illegal use. Vikas, please try to catch these cases and add tests for them.
I have done this code change but there is an issue. Line 1 InterfaceA<ConstructorReference> inter= ConstructorReference::new; Line 2 inter.m1(); If we mark illegal use at Line 1 what should the error message be? Because at that position only there is a reference to the constructor but no instantiation. My recommendation would be to use another error message. "MainClassNew illegally refers to ConstructorReference which cannot be instantiated" This would be driven by no instantiation tag on class. The actual instantiation may happen on line 2 or line 88 in same or different file/plugin and based on many runtime/dynamic conditions. So it may be impossible to track it at the compile time. Please let me know your views on this.
(In reply to Vikas Chandra from comment #17) > If we mark illegal use at Line 1 what should the error message be? Because > at that position only there is a reference to the constructor but no > instantiation. I think that a reference to the constructor is enough justification for a warning about illegal instantiation. Even if the instantiation will happen elsewhere (or not at all), it is likely that the code is attempting to use API in an improper way. Warning the user makes them aware they are using an API incorrectly (most likely), and if they have good reason to do so, it is trivial for them to filter the problem. So I suggest leaving the message as is and mark it on the line referencing the constructor.
I have implemented this. I have also created new test cases. All the existing test case pass. I will attach the patch in next couple of hour.
Created attachment 242009 [details] Patch to detect the constructor reference of classes with noinstantiate For constructor reference, we get a single reference of name "<init>" and method type as virtual ( even if I add IReference.REF_CONSTRUCTORMETHOD in IllegalInstantiateProblemDetector in reference kinds) . For normal constructor, there are 2 reference that are processed - one with name <init> and type as constructor method (( which is ignored)) and another of type class - which is actually marked as error. So for our case, constructor reference's name and type should be checked to create an API problem. I have created test case to trap these scenarios. All tests seem to run fine.
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=03a3e20baac4ebdb57e1c3b0824674d6e03bf953 Applied the latest patch to master I made a few changes to make the test setup more like the existing java 8 tests and updating copyrights Thanks Vikas. I also tested combinations of @noreference/@noinstantiate and multiple problems were created as expected.
(In reply to Curtis Windatt from comment #21) > http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/ > ?id=03a3e20baac4ebdb57e1c3b0824674d6e03bf953 > Applied the latest patch to master > I made a few changes to make the test setup more like the existing java 8 > tests and updating copyrights > > Thanks Vikas. I also tested combinations of @noreference/@noinstantiate and > multiple problems were created as expected. Are there plans to add tests that use the annotations? Was any smoke testing done with the annotations?
Created attachment 242043 [details] A patch to add annotation method ref test cases The patch adds the test cases for annotation cases. I have checked noreference and noinstatiate annotation wrt method reference and they work as expected. Curtis, can you please put in these test cases ( as given in method_ref_annotation.patch) as well ?
(In reply to Vikas Chandra from comment #23) > Created attachment 242043 [details] > A patch to add annotation method ref test cases > > The patch adds the test cases for annotation cases. I have checked > noreference and noinstatiate annotation wrt method reference and they work > as expected. > > Curtis, can you please put in these test cases ( as given in > method_ref_annotation.patch) as well ? Thanks Vikas. We should probably be using annotations over javadoc tags in the future as users should be encouraged to make the switch over time. We should also try to expand the tests to look for reference inside of secondary types (inner, nested, anonymous types). I will file a separate bug for that.
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=1c6fe2c59ce4b6a609a1b8fd7b59df473639e2e5 Patch applied
Verified for Eclipse(4.4) Luna M7 using build eclipse-SDK-I20140429-2000-win32-x86_64
