First cut at idl changes

First cut at idl changes

R=paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/9e62d501e60d3f0ec5121dfccb73f5dc3a886e13

[Paul Berry, 2017-02-28 17:06:48]

paulberry@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2017-02-28 17:06:49]

lgtm

https://codereview.chromium.org/2719353002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/2719353002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/idl.dart:2889: bool get isGenericFunctionType;
Question: How do you intend to represent a new-style typedef that does not
denote a generic function type, e.g.

typedef F = int Function(bool);

Would this flag be `true` or `false` in this case?

My two cents: I think the flag should be `true` in this case, since it will make
the code that generates summaries more uniform (any use of the form X
Function(Y, Z, ...) gets encoded in the same way, regardless of whether it has
type parameters and regardless of whether it appears in a typedef).

Assuming that's the case, I think we should rename this flag to something like
"isNewStyleTypedef" to avoid confusion.  (Otherwise, someone might assume that
this flag will be `true` only when the function type in question is truly
generic).

https://codereview.chromium.org/2719353002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/idl.dart:2911: * Return type of the typedef.
To reduce confusion, let's change this comment to something like:

If [isGenericFunctionType] is `false` return type of the typedef.  If
[isGenericFunctionType] is `true`, the function type being defined.

[Brian Wilkerson, 2017-02-28 17:19:57]

https://codereview.chromium.org/2719353002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/2719353002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/idl.dart:2889: bool get isGenericFunctionType;
> Question: How do you intend to represent a new-style typedef that does not
> denote a generic function type, e.g.
> 
> typedef F = int Function(bool);
> 
> Would this flag be `true` or `false` in this case?

That question reveals a terminology issue. The name "generic function type" is
the only name I know to give to the new syntax that uses "Function" in place of
a function name (and requires types but not names for parameters). As I've been
using it, it does *not* mean, a function type that actually has type parameters,
it means a function type that could have type parameters because of being
expressed using this new syntax.

Hence, the example you gave is, in my understanding, still a generic function
type, so the flag would be true.

> Assuming that's the case, I think we should rename this flag to something like
> "isNewStyleTypedef" to avoid confusion.  (Otherwise, someone might assume that
> this flag will be `true` only when the function type in question is truly
generic).

I'm fine with renaming this if there's a better name, but I won't use "NewStyle"
because if we later decide to support another style of typedef (and I've seen at
least two being discussed) they will be the "new" styles and I don't want to end
up with names like "NewerStyle" and "NewerThanNewStyle" :-)

In the absence of a better name, how about better documentation?

https://codereview.chromium.org/2719353002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/idl.dart:2911: * Return type of the typedef.
> To reduce confusion, let's change this comment ...

Good idea; will do.

[Paul Berry, 2017-02-28 17:38:57]

https://codereview.chromium.org/2719353002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/2719353002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/idl.dart:2889: bool get isGenericFunctionType;
On 2017/02/28 17:19:57, Brian Wilkerson wrote:
> > Question: How do you intend to represent a new-style typedef that does not
> > denote a generic function type, e.g.
> > 
> > typedef F = int Function(bool);
> > 
> > Would this flag be `true` or `false` in this case?
> 
> That question reveals a terminology issue. The name "generic function type" is
> the only name I know to give to the new syntax that uses "Function" in place
of
> a function name (and requires types but not names for parameters). As I've
been
> using it, it does *not* mean, a function type that actually has type
parameters,
> it means a function type that could have type parameters because of being
> expressed using this new syntax.
> 
> Hence, the example you gave is, in my understanding, still a generic function
> type, so the flag would be true.
> 
> > Assuming that's the case, I think we should rename this flag to something
like
> > "isNewStyleTypedef" to avoid confusion.  (Otherwise, someone might assume
that
> > this flag will be `true` only when the function type in question is truly
> generic).
> 
> I'm fine with renaming this if there's a better name, but I won't use
"NewStyle"
> because if we later decide to support another style of typedef (and I've seen
at
> least two being discussed) they will be the "new" styles and I don't want to
end
> up with names like "NewerStyle" and "NewerThanNewStyle" :-)
> 
> In the absence of a better name, how about better documentation?

Documentation is good, but it's even nicer when the variable name itself doesn't
suggest something counter to reality.  So let's not give up quite yet :)

I don't have a better name for the syntactic construct in question (perhaps we
should ask Leaf and/or the language team to come up with one), but I think we
could still come up with a name for this variable that makes the intention
clearer.

For example how about "usesGenericFunctionTypeSyntax"?  Or
"inGenericFunctionTypeForm"?

Alternatively, we could make this an enum called "style", and then the possible
values could be "cStyle" and "genericFunctionTypeStyle".  (Enums default to the
zeroth possible value, so this would still have the backwards compatibility
behavior we need).

I don't mind if you want to take one of those suggestions and wordsmith it
further to make it clearer; I just want to avoid having a variable name that
seems pretty clearly to mean something it doesn't.

[Brian Wilkerson, 2017-02-28 19:43:16]

Added a style enum. PTAL.

[Brian Wilkerson, 2017-02-28 20:28:49]

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
pkg/analyzer/lib/src/summary/idl.dart:2929: * [isGenericFunctionType] is `true`,
the function type being defined.
Just noticed that this comment is out of date. Will fix it.

[Paul Berry, 2017-02-28 20:43:56]

lgtm

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
pkg/analyzer/lib/src/summary/idl.dart:1120: * A typedef that defines a generic
function type. The syntax is
Nit: I would describe this as "A typedef that defines a function type that may
be generic."

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
pkg/analyzer/lib/src/summary/idl.dart:1122: * 'typedef' identifier
typeParameters? '=' returnType? 'Function' typeParameters? parameterTypeList ';'
Nit: long line.

[Brian Wilkerson, 2017-03-07 17:20:32]

I updated a few comments and ran the generator (no structural changes). PTAL

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
pkg/analyzer/lib/src/summary/idl.dart:1120: * A typedef that defines a generic
function type. The syntax is
In the process of shortening the line below I think I motivated the current form
of the comment. Let me know if this isn't sufficient.

[Paul Berry, 2017-03-07 17:56:21]

lgtm

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
pkg/analyzer/lib/src/summary/idl.dart:1120: * A typedef that defines a generic
function type. The syntax is
On 2017/03/07 17:20:32, Brian Wilkerson wrote:
> In the process of shortening the line below I think I motivated the current
form
> of the comment. Let me know if this isn't sufficient.

I still think it's confusing to say "A typedef that defines a generic function
type", when the fact is that the function type it defines may or may not be
generic, depending on whether the user supplied generic function type
parameters.  To me it's as confusing as if we had described a class declaration
as "a declaration that defines a generic class".

I don't understand why we can't just change the text to say "A typedef that
defines a function type that may be generic."  I could also live with "A typedef
expressed using generic function type syntax", since "generic function type
syntax" is a concept which I've learned does not necessarily imply that the
function type is generic.

But in the interest of forward progress, I won't pursue the issue further.

[Brian Wilkerson, 2017-03-07 18:21:40]

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/2719353002/diff/20001/pkg/analyzer/lib/src/su...
pkg/analyzer/lib/src/summary/idl.dart:1120: * A typedef that defines a generic
function type. The syntax is
> I could also live with "A typedef expressed using generic function type
syntax" ...

Ok, that seems like a better alternative, so I updated the comment.

[Brian Wilkerson, 2017-03-07 18:21:53]

Description was changed from

==========
First cut at idl changes
==========

to

==========
First cut at idl changes

R=paulberry@google.com

Committed:
https://github.com/dart-lang/sdk/commit/9e62d501e60d3f0ec5121dfccb73f5dc3a886e13
==========

[Brian Wilkerson, 2017-03-07 18:21:55]

Committed patchset #4 (id:60001) manually as
9e62d501e60d3f0ec5121dfccb73f5dc3a886e13 (presubmit successful).