Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #3239: added receiver parameter to java grammar #3260

Merged
merged 1 commit into from Jun 12, 2016

Conversation

rnveach
Copy link
Member

@rnveach rnveach commented Jun 8, 2016

Issue #3239

parameterIdent was created in the grammar so everything would fall under id.

@codecov-io
Copy link

codecov-io commented Jun 8, 2016

Current coverage is 100%

Merging #3260 into master will not change coverage

@@             master      #3260   diff @@
==========================================
  Files           276        276          
  Lines         13948      13948          
  Methods           0          0          
  Messages          0          0          
  Branches       3236       3236          
==========================================
  Hits          13948      13948          
  Misses            0          0          
  Partials          0          0          

Powered by Codecov. Last updated by ae254f7...a821556

@romani
Copy link
Member

romani commented Jun 9, 2016

http://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.4.1

  1. it looks like you will allow multiple receiver parameters in methods. Let me know if I miss smth

  2. according to specification receiver parameter could have annotation, please create input test on this

from spec

The receiver parameter is an optional syntactic device for an instance method or an inner class's constructor.

"instance methods" does it work for static methods ?

side note: as we already have limitation that we work on compilable sources, ..... our grammar could be relaxed to verify structure correctly and just expect that all is fine and make a tree. What do you think ?

@rnveach
Copy link
Member Author

rnveach commented Jun 9, 2016

@romani

it looks like you will allow multiple receiver parameters in methods

That is correct. I just modified parameterDeclaration and didn't create a second parameter section just for receivers of the first parameter.
My main reason for this is I didn't want to exclude this as being recognized as a parameter for our checks, but this also causes us to not recognize it separately from a normal parameter without examining it more.

receiver parameter could have annotation ... please create input test

I will do this. We already call parameterModifier for receivers, which accepts annotations, so the current grammar should accept it already.

does it work for static methods ?

No, eclipse gives the error Explicit 'this' parameter is allowed only in instance methods of non-anonymous classes and inner class constructors for public static void m2(Test this) {}.

our grammar could be relaxed to verify structure correctly and just expect that all is fine and make a tree.

Just so I understand correctly.
Grammar accepts even wrong code. We don't care if it accepts bad code, just as long as it always accepts good code and accepts it in the correct way. Real compilers should be the one to complain about bad code.

I am fine with that. Even the JLS makes additions saying grammar will accept some things, but compile time checks must reject them after the grammar, like static methods with receivers.
Our main concern should be is this what we want the grammar tree to look like.

There are other areas where we do this.
eb3cad2#diff-bd853af40772e3573aee62fd789f11adL987
(t:typeSpec[false])?: the question mark is actually invalid for method parameters, as they always have to have a type. The reason for the question mark is we use this same grammar for lambdas, which don't have a type with their parameters. We probably did this for the same reason, so parameters are recognized as one entity, instead of 2 or more.

@rnveach
Copy link
Member Author

rnveach commented Jun 10, 2016

please create input test

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants