Initial code to generate the Java types from the spec.

Initial code to generate the Java types from the spec. A constructor, the fields, getters, toString, and equals are all implemented.  hashCode is not yet generated for each class.

This doesn't do anything with enum types.

The main source is in codegen_java_types.dart

The types are not yet used in the Java server.

R=brianwilkerson@google.com, scheglov@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39228

[Brian Wilkerson, 2014-08-13 21:10:26]

LGTM

[Paul Berry, 2014-08-13 21:39:07]

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java:44:
private final String type;
The only purpose of this field is to allow the receiver to distinguish between
AddContentOverlay, ChangeContentOverlay, and RemoveContentOverlay.  (If you look
at the spec you'll see that each of these types sets the "type" field to a
different constant string).  Since Java has runtime type information, I'm not
sure this field is needed at all.

If we do decide to keep it, we should probably clean up the generation of the
empty doc comment--it looks a little odd.

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java:50:
this.type = type;
(Assuming that we don't get rid of this field) Since type has a fixed value,
caller shouldn't be allowed to set it.  The constructor should just set it to
the fixed value, in this case "add".

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java:79:
public String toString() {
It wouldn't take much to get this method to output proper JSON (change the [] to
{}, add "" around 'type' and 'content', and JSON encode the strings).  Similar
changes could probably be made to all the other generated classes.

Not sure whether this is worth doing, but I thought I would bring it up as a
possibility.

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AnalysisError.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AnalysisError.java:64:
public AnalysisError(String severity, String type, Location location, String
message, String correction) {
Some of these fields are required and others are optional.  Should we be adding
any documentation of which are optional and which aren't?  Should we generate
code to verify that the required fields are non-null?

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_java.dart (right):

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java.dart:157: * Return true iff the
passed [TypeDecl] will represent an array in Java.
Funky indentation here.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java.dart:221: * Temporary storage for
private fields.
And here

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java.dart:226: * Temporary storage for
constructors.
    And here

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_java_types.dart (right):

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:34: void visitApi() {
There's no need for visitApi() to be the main entry point of your visitor. 
Since you only care about the contents of a single type definition, I'd
recommend moving this code into visitTypeDefinition() instead of visitApi().

As a side benefit, that means you don't have to thread typeDef through the
constructor, since it will be an argument to visitTypeDefinition().

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:63: }), width: 99,
javadocStyle: true);
(Not really a comment on this CL, just an idea for a future improvement)

Maybe we should make an override for docComment in CodegenJavaVisitor which
calls super.docComment, passing in the correct values for width and
javadocStyle, so that we don't have to repeat these parameters everywhere in the
Java codegen.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:82: // write out the
assignments in the
Incomplete comment?

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:150: super.visitApi();
This will call HierarchicalApiVisitor.visitApi(), which will then recurse
through the entire API doing nothing.  I don't think that's what you want.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:201: return
visitor.collectCode(visitor.visitApi);
Assuming you take my first suggestion in this file, this would change to:

return visitor.collectCode(() { visitor.visitTypeDefinition(typeDef); }

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_tools.dart (right):

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:28:
upcasedParts.add(parts[i][0].toUpperCase() + parts[i].substring(1));
Change this to call capitalize()

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:376: bool check() {
We should also list the directory and make sure it doesn't contain any
unexpected files.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:396: void generate() {
We should also delete any files that are in the directory but not in the map.

[scheglov, 2014-08-13 22:05:59]

lgtm

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java:85:
builder.append(content.toString() + ", ");
We probably don't want to call toString() for String.

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AnalysisError.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AnalysisError.java:38:
private final String correction;
Would be good to keep fields in the same order as they are in spec (and in
constructor).
Order matters for understanding logical grouping and importance.

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/Error.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/Error.java:32:
public class Error {
Do we want to name it ServerError?

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/Parameter.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/Parameter.java:31:
public class Parameter {
This looks as a too generic name to me.
ExtractMethodParameter maybe?

In the future we could create "Change Method Signature" refactoring.

I will create a CL for the spec.

[jwren, 2014-08-13 22:29:16]

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java:44:
private final String type;
On 2014/08/13 21:39:06, Paul Berry wrote:
> The only purpose of this field is to allow the receiver to distinguish between
> AddContentOverlay, ChangeContentOverlay, and RemoveContentOverlay.  (If you
look
> at the spec you'll see that each of these types sets the "type" field to a
> different constant string).  Since Java has runtime type information, I'm not
> sure this field is needed at all.
> 
> If we do decide to keep it, we should probably clean up the generation of the
> empty doc comment--it looks a little odd.

Acknowledged.

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java:50:
this.type = type;
On 2014/08/13 21:39:06, Paul Berry wrote:
> (Assuming that we don't get rid of this field) Since type has a fixed value,
> caller shouldn't be allowed to set it.  The constructor should just set it to
> the fixed value, in this case "add". 

Acknowledged.

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java:79:
public String toString() {
Acknowledged.  Yes.  The toString is used only as a "human readable" method for
debugging.  Getting to/from Json generated might be worth pursuing later.

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AnalysisError.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AnalysisError.java:64:
public AnalysisError(String severity, String type, Location location, String
message, String correction) {
On 2014/08/13 21:39:06, Paul Berry wrote:
> Some of these fields are required and others are optional.  Should we be
adding
> any documentation of which are optional and which aren't?  Should we generate
> code to verify that the required fields are non-null?

The documentation on the getters does cover it in most cases.  As for using the
property from the spec input for an annotation, I'd be up for it.  We didn't do
this when we wrote the types by hand.  We can come back and add it if we want
it.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_java.dart (right):

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java.dart:157: * Return true iff the
passed [TypeDecl] will represent an array in Java.
Fixed. This is me fussing with the Dart Editor :)

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java.dart:221: * Temporary storage for
private fields.
On 2014/08/13 21:39:06, Paul Berry wrote:
> And here

Done.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java.dart:226: * Temporary storage for
constructors.
On 2014/08/13 21:39:06, Paul Berry wrote:
>     And here

Done.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_java_types.dart (right):

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:34: void visitApi() {
On 2014/08/13 21:39:07, Paul Berry wrote:
> There's no need for visitApi() to be the main entry point of your visitor. 
> Since you only care about the contents of a single type definition, I'd
> recommend moving this code into visitTypeDefinition() instead of visitApi().
> 
> As a side benefit, that means you don't have to thread typeDef through the
> constructor, since it will be an argument to visitTypeDefinition().

Done.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:63: }), width: 99,
javadocStyle: true);
Subclasses can't override with different default values.  What I can do is
introduce a new method in the super class for convenience which has the same
effect:

void javadocComment(List<dom.Node> docs) {
    docComment(docs, width: 99, javadocStyle: true);
  }


On 2014/08/13 21:39:06, Paul Berry wrote:
> (Not really a comment on this CL, just an idea for a future improvement)
> 
> Maybe we should make an override for docComment in CodegenJavaVisitor which
> calls super.docComment, passing in the correct values for width and
> javadocStyle, so that we don't have to repeat these parameters everywhere in
the
> Java codegen.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:82: // write out the
assignments in the
On 2014/08/13 21:39:06, Paul Berry wrote:
> Incomplete comment?

Done.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:150: super.visitApi();
Fixed.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:201: return
visitor.collectCode(visitor.visitApi);
On 2014/08/13 21:39:06, Paul Berry wrote:
> Assuming you take my first suggestion in this file, this would change to:
> 
> return visitor.collectCode(() { visitor.visitTypeDefinition(typeDef); }

Done.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_java_types.dart:201: return
visitor.collectCode(visitor.visitApi);
On 2014/08/13 21:39:06, Paul Berry wrote:
> Assuming you take my first suggestion in this file, this would change to:
> 
> return visitor.collectCode(() { visitor.visitTypeDefinition(typeDef); }

Done.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_tools.dart (right):

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:28:
upcasedParts.add(parts[i][0].toUpperCase() + parts[i].substring(1));
On 2014/08/13 21:39:07, Paul Berry wrote:
> Change this to call capitalize()

Done.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:376: bool check() {
On 2014/08/13 21:39:07, Paul Berry wrote:
> We should also list the directory and make sure it doesn't contain any
> unexpected files.

Acknowledged.  Meant to add a TODO, adding now.

https://codereview.chromium.org/473533003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:396: void generate() {
On 2014/08/13 21:39:07, Paul Berry wrote:
> We should also delete any files that are in the directory but not in the map.

Acknowledged.  Meant to add a TODO, adding now.

[jwren, 2014-08-13 22:34:28]

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AddContentOverlay.java:85:
builder.append(content.toString() + ", ");
On 2014/08/13 22:05:58, scheglov wrote:
> We probably don't want to call toString() for String.

Will add a TODO...  I thought of this, but it is sufficient, and generated code.
 :)

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AnalysisError.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/AnalysisError.java:38:
private final String correction;
Agreed.  For methods there is logic to sort alphabetically, I copied over the
logic, but I don't think we want to resort it.  Will change in a separate CL.

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/Error.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/Error.java:32:
public class Error {
Probably, especially since we also have AnalysisError....

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/Parameter.java
(right):

https://codereview.chromium.org/473533003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/generated/types/Parameter.java:31:
public class Parameter {
Awesome, thanks!

[jwren, 2014-08-13 22:37:32]

Committed patchset #3 manually as 39228 (presubmit successful).