Code generate Dart classes representing the analysis server API.

Code generate Dart classes representing the analysis server API.

These classes support:

- Conversion to JSON
- Conversion from JSON, with error handling via the Request object.
- operator== and hashCode

In addition, convenience functions are generated which extract and
convert the variant contents of a Request, Response, Notification,
EditGetRefactoringResult, or EditGetRefactoringParams once the type is
known.

The generated code is not yet used; it will be used by future CL's.

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

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

[Paul Berry, 2014-08-17 14:35:35]

On Friday some folks asked me to look at what it would take to code generate
Dart classes representing the analysis server API.  Here's what I came up with.

The generated classes aren't used yet, but I have experimented with using them,
and it seems like it would be fairly trivial to replace most of the hand-written
classes representing the API with the generated ones.  Eventually the new
generated code would replace the need for much of the code in
pkg/analysis_server/lib/src/protocol.dart.

One difficulty is that the generated code lives in analysis_server, and some of
the hand-written classes we would want to replace with the generated ones live
in analysis_services.  I suspect the best way to deal with this would be to go
ahead and combine analysis_server and analysis_services into a single package
(which we have talked about doing anyhow).

[danrubel, 2014-08-17 17:10:21]

LGTM

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:6801: if
(json.containsKey("enableAsync")) {
Is the containsKey check needed here? Will the map return null if the key is not
present? Would it be equivalent and more efficient (only lookup key once instead
of twice) if  _decodeBool accepted jsonValue = "true", "false", null, and
returned true/false/null and the caller checked for a null return value if the
caller cared to do so? If yes then this could be rewritten as

bool enableAsync = _decodeBool(jsonPath + ".enableAsync", json["enableAsync"]);

and if enableAsync was required it would read

bool enableAsync = _decodeBool(jsonPath + ".enableAsync", json["enableAsync"]);
if (enableAsync == null) {
   missingKey(jsonPath, "enableAsync");
}

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol2.dart (right):

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol2.dart:142: bool _decodeBool(String
jsonPath, Object json) {
Do you want to sort the class memebers via cmd-1 -> sort unit/class members ?

[scheglov, 2014-08-17 19:21:06]

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:4370: offsets ==
other.offsets &&
This code prints "false".

main() {
  var a = [1, 2, 3];
  var b = [1, 2, 3];
  print(a == b);
}

[Paul Berry, 2014-08-17 23:47:07]

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:4370: offsets ==
other.offsets &&
On 2014/08/17 19:21:06, scheglov wrote:
> This code prints "false".
> 
> main() {
>   var a = [1, 2, 3];
>   var b = [1, 2, 3];
>   print(a == b);
> }

Whoops.  Good catch.  I'll remedy this tomorrow.

[Brian Wilkerson, 2014-08-18 14:10:16]

LGTM

> One difficulty is that the generated code lives in analysis_server

Another option is to generate all of these classes into analysis_services. I
don't have a strong opinion on this, just throwing out another idea.

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:32: String toString() =>
JSON.encode(toJson());
It would be nice to add @override annotations where appropriate.

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:3074: class Element {
The Java generator generates utility methods such as isAbstract() and isConst()
for accessing the flags. It would be nice to have corresponding getters in Dart.

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:5825: ServerGetVersionResult
decodeServerGetVersionResult(String jsonPath, Object json) {
Consider converting these methods to named constructors on the appropriate
classes (or helper methods if the top-level functions below are the only callers
of these methods).

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:8000: ServerGetVersionResult
decodeServerGetVersionResponse(Response response) {
Consider converting these functions to constructors on the appropriate classes.

[Paul Berry, 2014-08-18 21:58:03]

PTAL

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:32: String toString() =>
JSON.encode(toJson());
On 2014/08/18 14:10:16, Brian Wilkerson wrote:
> It would be nice to add @override annotations where appropriate.

Done.

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:3074: class Element {
On 2014/08/18 14:10:16, Brian Wilkerson wrote:
> The Java generator generates utility methods such as isAbstract() and
isConst()
> for accessing the flags. It would be nice to have corresponding getters in
Dart.

Acknowledged.  I will  address this in a future CL.

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:4370: offsets ==
other.offsets &&
On 2014/08/17 19:21:06, scheglov wrote:
> This code prints "false".
> 
> main() {
>   var a = [1, 2, 3];
>   var b = [1, 2, 3];
>   print(a == b);
> }

Fixed.

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:5825: ServerGetVersionResult
decodeServerGetVersionResult(String jsonPath, Object json) {
On 2014/08/18 14:10:16, Brian Wilkerson wrote:
> Consider converting these methods to named constructors on the appropriate
> classes (or helper methods if the top-level functions below are the only
callers
> of these methods).

Done.

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:6801: if
(json.containsKey("enableAsync")) {
On 2014/08/17 17:10:20, danrubel wrote:
> Is the containsKey check needed here? Will the map return null if the key is
not
> present? Would it be equivalent and more efficient (only lookup key once
instead
> of twice) if  _decodeBool accepted jsonValue = "true", "false", null, and
> returned true/false/null and the caller checked for a null return value if the
> caller cared to do so? If yes then this could be rewritten as
> 
> bool enableAsync = _decodeBool(jsonPath + ".enableAsync",
json["enableAsync"]);
> 
> and if enableAsync was required it would read
> 
> bool enableAsync = _decodeBool(jsonPath + ".enableAsync",
json["enableAsync"]);
> if (enableAsync == null) {
>    missingKey(jsonPath, "enableAsync");
> }

That suggestion would create a semantic difference: { "enableAsync": null }
would be treated as equivalent to the "enableAsync" key being absent.  That's ok
for bools, because a bool can never be null.  But we've already decided (for
better compatibility with Lisp) that the client is allowed to replace empty
lists and maps with null.  That means that for an optional list or an optional
map, there would be an ambiguity: does { "foo": null } mean that the optional
field "foo" is absent, or that the optional field "foo" is an empty list/map? 
I'd rather avoid that ambiguity.

However, I agree that it's problematic to look up each key in the map twice
(once in the .containsKey() call and once in the use of operator[]).  What would
you think about something like this as an alternative?

  bool optionalBool;
  bool requiredBool;
  json.forEach((key, value) {
    switch (key) {
      case "optionalBool":
        optionalBool = _decodeBool(jsonPath + ".optionalBool", value);
        break;
      case "requiredBool":
        requiredBool = _decodeBool(jsonPath + ".requiredBool", value);
        break;
    }
  });
  if (requiredBool == null) {
    throw _missingKey(jsonPath, "requiredBool");
  }

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:8000: ServerGetVersionResult
decodeServerGetVersionResponse(Response response) {
On 2014/08/18 14:10:16, Brian Wilkerson wrote:
> Consider converting these functions to constructors on the appropriate
classes.

Done.

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol2.dart (right):

https://codereview.chromium.org/479043002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol2.dart:142: bool _decodeBool(String
jsonPath, Object json) {
On 2014/08/17 17:10:21, danrubel wrote:
> Do you want to sort the class memebers via cmd-1 -> sort unit/class members ?

Done.

[scheglov, 2014-08-18 22:15:42]

LGTM

[Brian Wilkerson, 2014-08-18 23:41:49]

LGTM

[Paul Berry, 2014-08-18 23:44:13]

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