Fix a bug where we would emit a noSuchMethod handler in the Object class for Object methods because…

Fix a bug where we would emit a noSuchMethod handler in the Object class for Object methods because of a selector with an empty receiver type, or an exact type for an abstract class.

R=ahe@google.com

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

[ahe, 2013-08-20 08:00:33]

Not sure I understand this.

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2821: } else
if (mask.isEmpty && !mask.isNullable) {
I'm a little concerned about this. Perhaps it is just a sign that I don't fully
understand the type inferrer.

We saw this bug in code using mirrors which means that type inferencing is
disabled. That makes me wonder if this is a symptom fix for something else.

Also, if this kind of type is meaningless, why is it inferred?

[ngeoffray, 2013-08-20 08:03:47]

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2821: } else
if (mask.isEmpty && !mask.isNullable) {
On 2013/08/20 08:00:33, ahe wrote:
> I'm a little concerned about this. Perhaps it is just a sign that I don't
fully
> understand the type inferrer.
> 
> We saw this bug in code using mirrors which means that type inferencing is
> disabled. That makes me wonder if this is a symptom fix for something else.
> 
> Also, if this kind of type is meaningless, why is it inferred?

The bug is not necessarily related to type inferencing. The SSA backend can also
create these selectors with empty receiver type. It just happens that it does
not do anything with it and registers  the selector to the world. Maybe that's
the point where you would want the filtering to happen?

I think it belongs in this file, filtering earlier seems more like an
optimization.

[ahe, 2013-08-20 08:11:12]

lgtm

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2821: } else
if (mask.isEmpty && !mask.isNullable) {
On 2013/08/20 08:03:48, ngeoffray wrote:
> On 2013/08/20 08:00:33, ahe wrote:
> > I'm a little concerned about this. Perhaps it is just a sign that I don't
> fully
> > understand the type inferrer.
> > 
> > We saw this bug in code using mirrors which means that type inferencing is
> > disabled. That makes me wonder if this is a symptom fix for something else.
> > 
> > Also, if this kind of type is meaningless, why is it inferred?
> 
> The bug is not necessarily related to type inferencing. The SSA backend can
also
> create these selectors with empty receiver type. It just happens that it does
> not do anything with it and registers  the selector to the world. Maybe that's
> the point where you would want the filtering to happen?
> 
> I think it belongs in this file, filtering earlier seems more like an
> optimization.

I don't understand why it would be an optimization. What is the contract between
providers of type masks and the consumers of type masks? How do we guarantee
correctness?

I would say it is reasonable for the consumer (emitter in this case) to assume
the provider (SSA backend/inferrer) only produce valid type masks.

Otherwise, you have to check all places where you consume a typemask that it
isn't bogus.

[ngeoffray, 2013-08-20 08:24:03]

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2821: } else
if (mask.isEmpty && !mask.isNullable) {
On 2013/08/20 08:11:13, ahe wrote:
> On 2013/08/20 08:03:48, ngeoffray wrote:
> > On 2013/08/20 08:00:33, ahe wrote:
> > > I'm a little concerned about this. Perhaps it is just a sign that I don't
> > fully
> > > understand the type inferrer.
> > > 
> > > We saw this bug in code using mirrors which means that type inferencing is
> > > disabled. That makes me wonder if this is a symptom fix for something
else.
> > > 
> > > Also, if this kind of type is meaningless, why is it inferred?
> > 
> > The bug is not necessarily related to type inferencing. The SSA backend can
> also
> > create these selectors with empty receiver type. It just happens that it
does
> > not do anything with it and registers  the selector to the world. Maybe
that's
> > the point where you would want the filtering to happen?
> > 
> > I think it belongs in this file, filtering earlier seems more like an
> > optimization.
> 
> I don't understand why it would be an optimization. What is the contract
between
> providers of type masks and the consumers of type masks? How do we guarantee
> correctness?
> 
> I would say it is reasonable for the consumer (emitter in this case) to assume
> the provider (SSA backend/inferrer) only produce valid type masks.
> 
> Otherwise, you have to check all places where you consume a typemask that it
> isn't bogus.

Why does it have to be bogus? There's an optimization phase that realizes the
type of an instruction is empty (take for example, a = 42; if (a is List)
a.toString();). If we don't do dead code elimination, the codegen will see this
call to a.toString, type the selector with a's type and register it to the
world. Checking if it's an empty selector seems like an optimization to me.

[ahe, 2013-08-20 08:31:44]

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2821: } else
if (mask.isEmpty && !mask.isNullable) {
On 2013/08/20 08:24:03, ngeoffray wrote:
> On 2013/08/20 08:11:13, ahe wrote:
> > On 2013/08/20 08:03:48, ngeoffray wrote:
> > > On 2013/08/20 08:00:33, ahe wrote:
> > > > I'm a little concerned about this. Perhaps it is just a sign that I
don't
> > > fully
> > > > understand the type inferrer.
> > > > 
> > > > We saw this bug in code using mirrors which means that type inferencing
is
> > > > disabled. That makes me wonder if this is a symptom fix for something
> else.
> > > > 
> > > > Also, if this kind of type is meaningless, why is it inferred?
> > > 
> > > The bug is not necessarily related to type inferencing. The SSA backend
can
> > also
> > > create these selectors with empty receiver type. It just happens that it
> does
> > > not do anything with it and registers  the selector to the world. Maybe
> that's
> > > the point where you would want the filtering to happen?
> > > 
> > > I think it belongs in this file, filtering earlier seems more like an
> > > optimization.
> > 
> > I don't understand why it would be an optimization. What is the contract
> between
> > providers of type masks and the consumers of type masks? How do we guarantee
> > correctness?
> > 
> > I would say it is reasonable for the consumer (emitter in this case) to
assume
> > the provider (SSA backend/inferrer) only produce valid type masks.
> > 
> > Otherwise, you have to check all places where you consume a typemask that it
> > isn't bogus.
> 
> Why does it have to be bogus?

Your 

> There's an optimization phase that realizes the
> type of an instruction is empty (take for example, a = 42; if (a is List)
> a.toString();). If we don't do dead code elimination, the codegen will see
this
> call to a.toString, type the selector with a's type and register it to the
> world. Checking if it's an empty selector seems like an optimization to me.

Let's talk about this face to face. Perhaps we need a whiteboard. I'm having a
really hard time understanding lines 2813-2877 (except, I understand lines
2872-2875).

[pavelj, 2013-08-20 14:51:43]

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/23225004/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2821: } else
if (mask.isEmpty && !mask.isNullable) {
Peter, that version was not using mirrors.

On 2013/08/20 08:00:33, ahe wrote:
> I'm a little concerned about this. Perhaps it is just a sign that I don't
fully
> understand the type inferrer.
> 
> We saw this bug in code using mirrors which means that type inferencing is
> disabled. That makes me wonder if this is a symptom fix for something else.
> 
> Also, if this kind of type is meaningless, why is it inferred?

[ngeoffray, 2013-08-26 10:44:24]

I incorporated a similar fix for abstract classes. PTAL.

[ahe, 2013-08-27 10:55:48]

LGTM!

[ngeoffray, 2013-08-27 11:27:57]

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