Handle assignability for Send and SendSet

Handle assignability for Send and SendSet.

R=karlklose@google.com

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

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

[karlklose, 2013-05-17 09:36:53]

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:342: static const
METHOD = "method";
Could we move these to Member or a class MemberKind?

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:447: if (error &&
element != null) {
When is element == null?

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:483: var memberKind)
{
memberKind is a String (and could be a MemberKind, see comment above).

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:544: // foo where foo
is a type.
Perhaps "The literal `Foo` where Foo is a type or the name of a type variable."?

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:599:
assert(invariant(node, type != null, message: 'No type set for $node'));
Don't we need the same invariant for 'is'? Also, analyze will already throw if
we return `null`, so is this adding enough value to duplicate the check?

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:801: // e++
or 'e--'.

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:974: //
TODO(johnniwinther): Remove this.
Where do you intent to check for unresolved/malformed types then?

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/an...
File tests/compiler/dart2js/analyze_api_test.dart (right):

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/an...
tests/compiler/dart2js/analyze_api_test.dart:89: if (message != null &&
message.contains(messagePart)) {
Where are empty messages coming from? Is this necessary?

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
File tests/compiler/dart2js/type_checker_test.dart (right):

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:514: Operators operator+(Operators
other) => this;
Add a space between 'operator' and the symbol.

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:571: bool z;
It would be nice if the name 'b' was available for this variable :-)

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:572: Operators a = new
Operators();
Why do we need the 'new Operator()'?

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:579: analyze('$header c = a + b;
}');
Perhaps add a local function
  check(text) => analyze('$header $text')

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:632: // `0` is not assignable to
operator+ on `a`.
Actually "The type of `0` is not assignable to the argument type of operator +
on `a`", but that is quite long. Can you find something shorter?

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:673: analyze('$header z = z || z;
}');
Perhaps you can combine l.664-671 and l.673-680 in one loop iterating over the
operator symbol.

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:853: String CORE_SOURCE =
DEFAULT_CORELIB
Should we make sure that classNum and ClassInt are actually found in the string?

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:940: int end =
endToken.charOffset+endToken.slowCharCount;
Add spaces around +.

[Johnni Winther, 2013-05-17 11:46:33]

PTAL

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:342: static const
METHOD = "method";
On 2013/05/17 09:36:53, karlklose wrote:
> Could we move these to Member or a class MemberKind?

Done.

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:447: if (error &&
element != null) {
On 2013/05/17 09:36:53, karlklose wrote:
> When is element == null?

Never at this point. Added an assertion.

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:483: var memberKind)
{
On 2013/05/17 09:36:53, karlklose wrote:
> memberKind is a String (and could be a MemberKind, see comment above).

Done.

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:544: // foo where foo
is a type.
On 2013/05/17 09:36:53, karlklose wrote:
> Perhaps "The literal `Foo` where Foo is a type or the name of a type
variable."?

Done.

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:599:
assert(invariant(node, type != null, message: 'No type set for $node'));
On 2013/05/17 09:36:53, karlklose wrote:
> Don't we need the same invariant for 'is'? Also, analyze will already throw if
> we return `null`, so is this adding enough value to duplicate the check?

Removed.

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:801: // e++
On 2013/05/17 09:36:53, karlklose wrote:
> or 'e--'.

Done.

https://codereview.chromium.org/13084013/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:974: //
TODO(johnniwinther): Remove this.
On 2013/05/17 09:36:53, karlklose wrote:
> Where do you intent to check for unresolved/malformed types then?

It will (eventually) only be used for FunctionExpressions which already handle
this.

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/an...
File tests/compiler/dart2js/analyze_api_test.dart (right):

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/an...
tests/compiler/dart2js/analyze_api_test.dart:89: if (message != null &&
message.contains(messagePart)) {
On 2013/05/17 09:36:53, karlklose wrote:
> Where are empty messages coming from? Is this necessary?

Not anymore. Removed.

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
File tests/compiler/dart2js/type_checker_test.dart (right):

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:514: Operators operator+(Operators
other) => this;
On 2013/05/17 09:36:53, karlklose wrote:
> Add a space between 'operator' and the symbol.

Done.

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:572: Operators a = new
Operators();
On 2013/05/17 09:36:53, karlklose wrote:
> Why do we need the 'new Operator()'?

We don't.

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:579: analyze('$header c = a + b;
}');
On 2013/05/17 09:36:53, karlklose wrote:
> Perhaps add a local function
>   check(text) => analyze('$header $text')

Done.

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:632: // `0` is not assignable to
operator+ on `a`.
On 2013/05/17 09:36:53, karlklose wrote:
> Actually "The type of `0` is not assignable to the argument type of operator +
> on `a`", but that is quite long. Can you find something shorter?

Added a comment on the terminology.

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:673: analyze('$header z = z || z;
}');
On 2013/05/17 09:36:53, karlklose wrote:
> Perhaps you can combine l.664-671 and l.673-680 in one loop iterating over the
> operator symbol.

Done.

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:853: String CORE_SOURCE =
DEFAULT_CORELIB
On 2013/05/17 09:36:53, karlklose wrote:
> Should we make sure that classNum and ClassInt are actually found in the
string?

We do - in the lines above.

https://codereview.chromium.org/13084013/diff/11001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:940: int end =
endToken.charOffset+endToken.slowCharCount;
On 2013/05/17 09:36:53, karlklose wrote:
> Add spaces around +.

Done.

[karlklose, 2013-05-17 11:49:44]

LGTM.

https://codereview.chromium.org/13084013/diff/22001/tests/compiler/dart2js/ty...
File tests/compiler/dart2js/type_checker_test.dart (right):

https://codereview.chromium.org/13084013/diff/22001/tests/compiler/dart2js/ty...
tests/compiler/dart2js/type_checker_test.dart:636: // We brevity we misuse the
terminology in comments:
'For [the sake of] brevity...'

[Johnni Winther, 2013-05-17 12:01:24]

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

[Johnni Winther, 2013-05-21 09:18:23]

PTAL

[karlklose, 2013-05-21 09:23:41]

LGTM. Is there a place where we could assert that the tree-elements are not
null?

[Johnni Winther, 2013-05-21 09:26:32]

On 2013/05/21 09:23:41, karlklose wrote:
> LGTM. Is there a place where we could assert that the tree-elements are not
> null?

I will look into it in another CL.

[Johnni Winther, 2013-05-21 09:40:44]

Committed patchset #5 manually as r22925 (presubmit successful).