Handle type variables.

Resolve type variables of generic methods.

R=sigmund@google.com

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

[ahe, 2017-02-03 20:46:02]

ahe@google.com changed reviewers:
+ johnniwinther@google.com, sigmund@google.com

[ahe, 2017-02-03 20:46:03]

I'm not happy with the names in this CL. Please make suggestions.

Also, please don't hesitate to ask for additional documentation. I always find
it easier to write documentation when someone tells me what they find unclear.

[ahe, 2017-02-03 20:46:28]

Description was changed from

==========
Handle type variables.
==========

to

==========
Resolve type variables of generic methods.
==========

[Siggi Cherem (dart-lang), 2017-02-06 18:05:11]

lgtm

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:71:
BuilderScope<T> innerScope;
+dartdoc:

/// Type-scope for the current builder (current class or current method?)

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:83:
BuilderScope<T> get builderScope => innerScope ?? libraryScope;
I'm not against this name, another idea could be `currentScope`.

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:83:
BuilderScope<T> get builderScope => innerScope ?? libraryScope;
thinking more about it - here is an alternative idea:

delete builderScope,  rename innerScope => currentScope and make it always
non-null (set it to libraryScope at first).

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:85: Map<String,
MemberBuilder> get classMembers {
+dartdoc, when currently within a class, this returns the builder for each
member in the class.

it will be good to highlight also what's the invariant here: is it all members
of the class, or only members that has been parsed?

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:88: return
builderScope.members;
since you are anyways assuming this is only called when innerScope ==
builderScope, consider skipping the indirection here?
return innerScope.members;?

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:91: List<T> get
declarationTypes {
+dartdoc: the type variables declared within the current class or generic
method.

consider adding an example of each:
class:
  [T,R] for class C<T, R> { ... }

top-level method:
  [G,H] for void m<G, H>() { ... }

generic method within class:
  [R] for class C<T> { m<R>() { ... } }
?

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:94: return
builderScope.types;
ditto => innerScope.types;

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:102:
builderScope.addType(type);
+ assert(innerScope != null) ?

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:360: class
BuilderScope<T extends TypeBuilder> {
Another idea - TypeScope or TypeBuilderScope ?

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:388:
parent.types.addAll(types);
it might be worth adding some comments to explain in what contexts this happens

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:401:
parent.addType(type);
here too.

[ahe, 2017-02-08 13:31:41]

I have a bunch of stuff on top of this, so I prefer to do the renaming in a
follow-up.

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:71:
BuilderScope<T> innerScope;
On 2017/02/06 18:05:11, Siggi Cherem (dart-lang) wrote:
> +dartdoc:
> 
> /// Type-scope for the current builder (current class or current method?)

Added a bit more information.

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:83:
BuilderScope<T> get builderScope => innerScope ?? libraryScope;
On 2017/02/06 18:05:11, Siggi Cherem (dart-lang) wrote:
> I'm not against this name, another idea could be `currentScope`.

I like that idea. Perhaps `currentDeclaration`.

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:85: Map<String,
MemberBuilder> get classMembers {
On 2017/02/06 18:05:11, Siggi Cherem (dart-lang) wrote:
> +dartdoc, when currently within a class, this returns the builder for each
> member in the class.
> 
> it will be good to highlight also what's the invariant here: is it all members
> of the class, or only members that has been parsed?

Done.

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:88: return
builderScope.members;
On 2017/02/06 18:05:11, Siggi Cherem (dart-lang) wrote:
> since you are anyways assuming this is only called when innerScope ==
> builderScope, consider skipping the indirection here?
> return innerScope.members;?

Yes, but you suggestion above makes this unnecessary.

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:91: List<T> get
declarationTypes {
On 2017/02/06 18:05:11, Siggi Cherem (dart-lang) wrote:
> +dartdoc: the type variables declared within the current class or generic
> method.
> 
> consider adding an example of each:
> class:
>   [T,R] for class C<T, R> { ... }
> 
> top-level method:
>   [G,H] for void m<G, H>() { ... }
> 
> generic method within class:
>   [R] for class C<T> { m<R>() { ... } }
> ?

I already have CL on top of this that removes this getter.

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:102:
builderScope.addType(type);
On 2017/02/06 18:05:11, Siggi Cherem (dart-lang) wrote:
> + assert(innerScope != null) ?

I don't think that will work right now. I don't enter a nested scope when
parsing fields.

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:360: class
BuilderScope<T extends TypeBuilder> {
On 2017/02/06 18:05:11, Siggi Cherem (dart-lang) wrote:
> Another idea - TypeScope or TypeBuilderScope ?

Not a bad idea, but the class does more than handle types. What about
"DeclarationBuilder"? This class helps build declarations.

[ahe, 2017-02-08 15:11:26]

Description was changed from

==========
Resolve type variables of generic methods.
==========

to

==========
Resolve type variables of generic methods.

R=sigmund@google.com

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

[ahe, 2017-02-08 15:11:28]

Committed patchset #3 (id:40001) manually as
a881e826dc890daa159f7cc4a41bb5f605976b88 (presubmit successful).

[ahe, 2017-02-08 16:57:21]

FYI

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:83:
BuilderScope<T> get builderScope => innerScope ?? libraryScope;
On 2017/02/08 13:31:40, ahe wrote:
> On 2017/02/06 18:05:11, Siggi Cherem (dart-lang) wrote:
> > I'm not against this name, another idea could be `currentScope`.
> 
> I like that idea. Perhaps `currentDeclaration`.

Done in CL 2682993002.

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:388:
parent.types.addAll(types);
On 2017/02/06 18:05:11, Siggi Cherem (dart-lang) wrote:
> it might be worth adding some comments to explain in what contexts this
happens

Done.

https://codereview.chromium.org/2672993003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:401:
parent.addType(type);
On 2017/02/06 18:05:11, Siggi Cherem (dart-lang) wrote:
> here too.

Done.