Community
Participate
Working Groups
BETA_JAVA8: Top of branch contains syntactic support for explicit this parameter to facilitate attaching receiver annotations. The compiler must perform several validations here. - Only at 1.8 explicit this should be allowed (done) - Only as a first parameter (done) Here is the relevant part of the spec: Only the first formal parameter may be named this, and such a parameter is only permitted on an instance method, or on a static method or constructor of an inner class. The type of the this parameter must be the same as the class that contains the method and should include type arguments if that class has any. In a method in an inner type, the receiver type can be written as (for example) either Inner or as Outer.Inner, and in the latter case annotations on both parts are possible, as in @ReadOnly Outer . @Mutable Inner. [I believe the "on the static method of an inner class" part is wrong - clarifications have been sought from the EG]
(In reply to comment #0) > [I believe the "on the static method of an inner class" part > is wrong - clarifications have been sought from the EG] We have confirmation from the spec lead that this was an error in the draft and it will be fixed in future.
Created attachment 218631 [details] Test case A simple test case with legal and illegal uses of receiver parameters.
(In reply to comment #2) > Created attachment 218631 [details] > Test case > > A simple test case with legal and illegal uses of receiver parameters. I thought the formatting will be retained if I added as an attachment instead of posting as a comment. But it looks bad even then and only way to make it look good is to copy/paste into eclipse editor :(
(In reply to comment #3) The problem is that browsers use a tab width of 8. The only fully-portable solution is to use spaces exclusively (also at the line start). (In reply to comment #0) > The type of the this parameter must be the same as the class that contains > the method and should include type arguments if that class has any. This part of the spec is contradictory and needs clarification. The "must be the same" makes raw types illegal but the "should" seems to allow them. Srikanth, can you please ask the spec lead to replace "should" with "must"?
(In reply to comment #2) > Created attachment 218631 [details] > Test case > > A simple test case with legal and illegal uses of receiver parameters. On the AnonymousInner case, I have asked for clarifications from the EG. (In reply to comment #4) > Srikanth, can you please ask the spec lead to replace "should" with "must"? Should I or must I ? :) I will, Thanks.
> AnonymousInner ai = new AnonymousInner() { > public void foobar(AnonymousInner this) {} // legal > }; This currently violates "The type of the this parameter must be the same as the class that contains the method". The class that contains this foobar method is not AnonymousInner -- it's a subtype of that interface. But an interesting question is why the spec talks about the "class" that contains the non-static method. => This must be changed to "type that contains the method". The spec even contains an example of this in A.3. Test case to add: interface AnonymousInner { public void foobar(AnonymousInner this); //legal } The test case should also contain an example of a local class, e.g.: void bar() { class Local { @Override public int hashCode(Local this) { return 0; } // legal @Override public String toString(Outer.Local this) { // illegal return super.toString(); } } } > Should I or must I ? :) I will, Thanks. According to RFC 2119, you should, i.e. you must unless you have good reasons not to ;-)
(In reply to comment #6) > This currently violates "The type of the this parameter must be the same as the > class that contains the method". The class that contains this foobar method is > not AnonymousInner -- it's a subtype of that interface. The spec doesn't mention anything about this. I think it would be better if we can clarify this with the spec owner. We don't want it to be an oversight on their side. > But an interesting question is why the spec talks about the "class" that > contains the non-static method. > => This must be changed to "type that contains the method". The spec even > contains an example of this in A.3. Aren't only non-static methods allowed to have receiver parameters? > Test case to add: > > interface AnonymousInner { > public void foobar(AnonymousInner this); //legal > } Just out of curiosity, what purpose does the 'this' serve in an abstract method?
(In reply to comment #7) > The spec doesn't mention anything about this. I think it would be better if we > can clarify this with the spec owner. We don't want it to be an oversight on > their side. Per comment#5, this has been conveyed. > > But an interesting question is why the spec talks about the "class" that > > contains the non-static method. > > => This must be changed to "type that contains the method". The spec even > > contains an example of this in A.3. > > Aren't only non-static methods allowed to have receiver parameters? I don't think Markus's comment is so much about non-static vs static as about type vs class. > Just out of curiosity, what purpose does the 'this' serve in an abstract > method? It serves as a place holder for annotations that could be a part of the published contract - then an annotation processor check if the implementations conform or only deviate in permissible ways.
(In reply to comment #6) > public String toString(Outer.Local this) { // illegal is "Outer.Local" allowed? At least, eclipse is rejecting this.
(In reply to comment #8) > I don't think Markus's comment is so much about non-static vs static > as about type vs class. > > > Just out of curiosity, what purpose does the 'this' serve in an abstract > > method? > > It serves as a place holder for annotations that could be a part of the > published contract - then an annotation processor check if the implementations > conform or only deviate in permissible ways. Thanks for the clarifications, Srikanth! It all makes sense now.
> is "Outer.Local" allowed? At least, eclipse is rejecting this. Eclipse correctly rejects this. I just thought it would be good to include this in a test case to ensure that the compiler also keeps rejecting it in the "this" parameter.
Jay, please take note of this commit http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=4c2bd5433955f4a987d71f9fd88510a6a9140d4f which was preparatory in nature for the fix for bug 383596. You can down cast the NameReference after an instanceof check and access token or tokens as the case may be, walk from right to left at each step matching token against the enclosing class's name reporting errors where needed.
> You can down cast the NameReference after an instanceof check > and access token or tokens as the case may be, walk from right Better yet, introduce a new abstract method in NameReference called getName() and override it suitably in subtypes. See org.eclipse.jdt.internal.compiler.ast.TypeReference.getTypeName() for a similar example.
Created attachment 218740 [details] Updated test case This should take care of the formatting issues.
Created attachment 218850 [details] Draft patch This patch still needs some improvements. Patch also has one big test case that reports 14 error conditions, mostly different kind of errors. Some notes: 1. The type parameter check looks a bit lame. Need to be strengthened. 2. Error messages can be combined or new kind of problem messages can be introduced if required. I don't think the wording of the new error messages are perfect. Will work on them. Srikanth, I found resolving the NamedReference and doing a type comparison easier than working with the tokens. Not really sure if this has any side effects.
Created attachment 218969 [details] Proposed fix An additional scenario has been added to the test case. Some of the checks have been simplified and error messages modified. Note that in the new scenario in the test case, there are multiple markers reported: public void foo(Inner.InnerMost<T> this, Object obj) {} - The member type Outer.Inner.InnerMost<T> must be qualified with a parameterized type, since it is not static - Illegal type for 'this' parameter. Only the immediate enclosing type is allowed here Srikanth, can you review this patch whenever you have time.
Here are the first batch of comments on the proposed fix: 0. I wonder of we are better of having just one IProblem instead of three separate IProblems for the messages: 651 = Illegal type for ''this'' parameter. Only the immediate enclosing type is allowed here 652 = Illegal type for ''this'' parameter of a constructor. Only the enclosing type of the current type is allowed here. 653 = Illegal type for ''this'' parameter. Type parameter must match the declared type {0} How about: The declared type of the explicit ''this'' parameter is expected to be {0} It is not clear to me that additional verbose discriminating text is any better than stating the expected type upfront. In fact it creates some ambiguities and confusions (see below) I haven't looked at the code that issues these errors yet, but they could perhaps benefit from simplification too if we choose that as the strategy ? 1. Likewise, are we better off having just one instead of two two separate IProblems for the messages: 654 = Unqualified reference to ''this'' is not allowed in constructors 655 = Qualifier for the ''this'' reference must match the declared type of the parameter How about: The explicit ''this'' parameter is expected to be qualified with {0} 2. Should it be "this" or ''this'' ? I see both usages - I personally prefer "", but I am OK with '' if you prefer. 3. Error 1 reads: "----------\n" + "1. ERROR in Outer.java (at line 2)\n" + " Outer(Outer Outer.this) {}\n" + " ^^^^^\n" + "Receiver parameters are not allowed here. Only non-static member classes can have \'this\' parameter in constructors\n" + "----------\n" (a) source position highlighting is incorrect. It should underline Outer.this or the full parameter including the type. (b) I think the message is better worded as "Explicit this parameter is not allowed here. Only non-static member classes can declare ..." Alternately "Receiver declaration is not allowed here ...: i.e Not "Receiver parameters", use "Explicit this parameter" or "Receiver declaration" Not "can have", use "can declare" 4. Error 2 reads: "----------\n" + "2. ERROR in Outer.java (at line 3)\n" + " Outer(Outer this, int i) {}\n" + " ^^^^\n" + "Unqualified reference to \'this\' is not allowed in constructors\n" + "----------\n" + (a) The message is too sweeping in nature and taken literally is incorrect. It is perhaps better worded as "Constructors cannot declare unqualified explicit this parameter" or "... unqualified receiver" (b) Also, actually, the more appropriate error here is that constructors of top level classes cannot declare this parameter and not that it is unqualified. If we complain about it being unqualified and the programmer now qualifies it, that does not make the program legal. So we should complain about the more egregious problem. 5. Error 4 reads: "4. ERROR in Outer.java (at line 6)\n" + " InnerMost(Outer.Inner Outer.Inner.this) {}\n" + " ^^^^^^^^^^^\n" + "Illegal type for \'this\' parameter. Type parameter must match the declared type Outer.Inner<K,V>\n" + "----------\n" + This text is a bit imprecise, I think you are trying to say "the declared type of the this parameter must match the type of the type declaring the method including type parameters." Life is simpler if we just punt and state {0} is the expected type in the declaration per comment 0 above. 6. Line 7: I would have expected two errors. We report only one. Same comment on line 8. 7. Error 10 reads: "----------\n" + "10. ERROR in Outer.java (at line 13)\n" + " public void foo(Inner<K,V> Inner.this, int i) {}\n" + " ^^^^^\n" + "Illegal type for \'this\' parameter. Only the immediate enclosing type is allowed here\n" + "----------\n" + This highlights another benefit in moving to the simpler message outlined in comment 0. Inner *is* the immediate enclosing type of *type* in line 13, but it is *not* the immediate enclosing type of the *method* in line 13. Instead of using verbose text, printing the expected type is better IMO. 8. Error 14 looks suspicious: "----------\n" + "14. ERROR in Outer.java (at line 15)\n" + " public void foo(Inner.InnerMost<T> this, Object obj) {}\n" + " ^^^^^^^^^^^^^^^\n" + "Illegal type for \'this\' parameter. Only the immediate enclosing type is allowed here\n" + "----------\n" + I think the immediate enclosing part is dubious - the real error is explained by 13 perhaps. This one could also benefit from simpler statement of expected type. Jay, if you agree, could you make the changes are repost a patch ? It is enough if only the Java8 tests + CompilerInvocationTests are run - we can defer full run until after full review is done.
(In reply to comment #17) > How about: > The declared type of the explicit ''this'' parameter is expected to be {0} cf. 328 = The declared package "{1}" does not match the expected package "{0}"
(In reply to comment #17) > 2. Should it be "this" or ''this'' ? I see both usages - I personally prefer > "", but I am OK with '' if you prefer. I remember we had some trouble with the double quote. Don't remember what exactly. If there are other instances, they should be changed to '' too.
(In reply to comment #19) > (In reply to comment #17) > > 2. Should it be "this" or ''this'' ? I see both usages - I personally prefer > > "", but I am OK with '' if you prefer. > > I remember we had some trouble with the double quote. Don't remember what > exactly. If there are other instances, they should be changed to '' too. In a separate bug please.
Created attachment 219183 [details] Updated patch I have grouped the error messages into three buckets. One each for all illegal declaration type errors, illegal this qualifiers and all other disallowed locations (anonymous, static methods, static class' constructors etc.) This also helped simplify the code a bit. So, I am okay with the suggestions.
This looks much better and closer: Here are a few more comments: 1. For consistency's sakes, the IProblem disallowedExplicitThisParameter should begin capitalized. I also wonder if it is also better renamed to match either InvalidLocationForModifiers or MisplacedTypeAnnotations 2. There is a jump in numbering at IllegalTypeForExplicitThis. 3. Sorry for giving feedback in piecemeal fashion. Is this message: 638 = Explicit ''this'' parameter is not allowed here better worded as "Explicit ''this'' parameter is allowed only in instance methods and inner class constructors" 4. My bad - There are no tests that trigger 642 & 643, since it is the same topic and you are touching these messages anyway, could you add tests ? Also see (5) below. We need tests that trigger 642 & 643 for both instance methods and inner class constructors. 5. My bad again: Message 642 reads: Only the first formal parameter of an instance method may be declared explicitly as ''this'' I think it should say "Only the first formal parameter may be declared explicitly as ''this''" 6. Line 6: I would expect to see 2 errors, we report only one - why ? The two errors are both "primary errors", i.e fixing one does not make the other one go away and as such both should be reported upfront. 7. Also in line 8 (and a few more too perhaps) - There are two errors, we report only one. Both are primary errors independent of each other and should both be reported upfront. 8. Are there any material changes in Receiver.java ? I see only white space changes - if so, let us get rid of this. 9. AbstractMethodDeclaration.resolveReceivers: (a) Rename to resolveReceiver. (b) extract this.binding.declaringClass into local (c) look for similar opportunities. 10. illegalQualifierForExplitiThis (a) Rename to illegalQualifierForExplititThis ^ (missing t) (b) There are three calls to this method - I think we need exactly one call. (Introduce abstract char [][[] getName() in NameReference - compare org.eclipse.jdt.internal.compiler.ast.TypeReference.getTypeName()) (11) I would also write a couple of tests that trigger an error based on the annotation - i.e this itself is used correctly, but annotation is missing say - one test for method and one for constructor. This list looks long - but the patch is actually in very good shape for somebody starting on compiler work - I think one more minor iteration and we should be done.
(In reply to comment #22) > (b) There are three calls to this method - I think we need exactly one call. > (Introduce abstract char [][[] getName() in NameReference - compare > org.eclipse.jdt.internal.compiler.ast.TypeReference.getTypeName()) For the record, I didn't mean we should call getTypeName() and compare what it returns - I meant along the lines of TypeReference.getTypeName() we should introduce NameReference.getName().
Created attachment 219202 [details] Updated patch This patch contains all the changes suggested by Srikanth. The fix passes all Java8 tests.
More comments: 1. I think ComplianceDiagnoseTest.test0064 should be short circuited for this.complianceLevel >= ClassFileConstants.JDK1_8 - i.e it can still be run for 1.3 and 1.4 - I think the present checks are due to copy + paste from a test that involves generics. 2. Same test: Why are escaping single quotes inside strings ??? It is just visual distraction. 3. Same test: Prune the terminal superfluous white space - also in messages.properties. 4. CompilerInvocationTests still references disallowedExplicitThisParameter with the lowercase initial letter. 5. NameReference, SingleNameReference, QualifiedNameReference need a JCP/JSR disclaimer in copyright. 6. ProblemReporter.illegalQualifierForExplititThis is misnamed ;-) 7. AMD.resolveReceiver: Comment that reads: "/* An error has already been reported on the receiver parameter, for e.g. Type can not be resolved */" is wrong. I think you want to say, "if an error is already reported, just return.", but we could simply remove this comment altogether, the code is self explanatory. 8. Suggest rename isValidQualifier to isValidQualifierForExplicitThis. 9. Suggest "int index=tokens.length-1" be written as "int index = tokens.length - 1" instead. Otherwise, all looks good. If tests are all green, please release. No need for one more round of scrutiny.
Created attachment 219223 [details] Updated patch (In reply to comment #25) > More comments: > > 1. I think ComplianceDiagnoseTest.test0064 should be short circuited > for this.complianceLevel >= ClassFileConstants.JDK1_8 - i.e it can still > be run for 1.3 and 1.4 - I think the present checks are due to copy + paste > from a test that involves generics. Actually, these were failing in 1.3 and 1.4 compliance mode due to a bug in MethodScope. I have fixed that too, in this patch. Can you verify if the change is alright too? > 2. Same test: Why are escaping single quotes inside strings ??? It is just > visual distraction. > 3. Same test: Prune the terminal superfluous white space The expected output printed on the console has the \' for some reason. And so are the spaces at the end of the line. I have removed the '\' but have left the spaces. > 8. Suggest rename isValidQualifier to isValidQualifierForExplicitThis. It doesn't matter much, but I have replaced it with isQualifierValidForType. This is not specific to explicit this, just a utility method that checks if the given qualifier represents a given type.
(In reply to comment #26) > Created attachment 219223 [details] > Updated patch Running all tests. Will release after the tests complete.
All tests are green. Released here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=31b51b3c914faa25699d3f71c6d656dfc7fc54f5
(In reply to comment #26) > > 1. I think ComplianceDiagnoseTest.test0064 should be short circuited > > for this.complianceLevel >= ClassFileConstants.JDK1_8 - i.e it can still > > be run for 1.3 and 1.4 - I think the present checks are due to copy + paste > > from a test that involves generics. > > Actually, these were failing in 1.3 and 1.4 compliance mode due to a bug in > MethodScope. I have fixed that too, in this patch. Can you verify if the change > is alright too? Looks good, thanks. > > 2. Same test: Why are escaping single quotes inside strings ??? It is just > > visual distraction. > > 3. Same test: Prune the terminal superfluous white space > > The expected output printed on the console has the \' for some reason. And so > are the spaces at the end of the line. The spaces are coming because messages.properties has the space,which is why I mentioned that in 3. but never mind, it is OK.
(In reply to comment #29) > The spaces are coming because messages.properties has the space,which is why > I mentioned that in 3. but never mind, it is OK. Those have been fixed. I was talking about the trailing spaces after '+' in each line of the expected result.
(In reply to comment #26) > > 2. Same test: Why are escaping single quotes inside strings ??? It is just > > visual distraction. > > 3. Same test: Prune the terminal superfluous white space > > The expected output printed on the console has the \' for some reason. And > so are the spaces at the end of the line. I have removed the '\' but have > left the spaces. Sorry, I hadn't realized this. In future, we don't need to fix by hand such things.
