fix some tests

Most of these tests appear to be relying on checking code that will never be run.  My model of dart is that these tests may not happen - unless compiled with a special --compile_all flag - and that we should fix the tests by adding some small amount of code to ensure that the broken code is reached.

I have some more detailed notes on a few of the tests which have different issues.

With these changes, the vm and dartc both still pass all tests (except for some confusing behavior in dartc on ThirdTest).  However, these tests should match the expected semantics of dart more closely and allow for greater flexibility in deciding exactly when correctness must be verified.

Note: We still need to add a separate test for the --compile_all flag that will test the mode where we insist on compiling/checking all input code.

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

[jimhug, 2011-10-31 23:36:31]

Hi Kasper,

I'd appreciate it if you could take a look at these proposed test changes and
let me know whether or not they fit with your view of Dart.  It wouldn't be very
hard to make these pass in frog - but I feel that would be a mistake in their
current state.

Thanks for your feedback - Jim

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library2Nega...
File tests/language/src/Library2NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library2Nega...
tests/language/src/Library2NegativeTest.dart:11: #import("library2.dart");
This seems totally broken to me without this line.  Imports are not transitive
so members that are imported by library3 or library4 should not be exposed to
classes that import them.  If anything, I think it could be valuable to keep the
old version of this test around as a positive test since both dartc and the vm
appear to be generating an error here.

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideMeth...
File tests/language/src/OverrideMethodWithFieldTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideMeth...
tests/language/src/OverrideMethodWithFieldTest.dart:25: print(s.instanceMethod);
This instanceMethod field overriding the super method is not necessarily an
error until it is referred to.

http://codereview.chromium.org/8431016/diff/1/tests/language/src/ThirdTest.dart
File tests/language/src/ThirdTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/ThirdTest.da...
tests/language/src/ThirdTest.dart:6: class A {
This appeared totally broken to me according to the spec.  A and B both define
static var s and static method foo.  When A extends B, these static fields will
shadow each other which appears illegal to me when reading the spec on Static
Methods and Static Fields.  Like the other case that looks truly broken, it may
be worth keeping this around as a negative test to verify that this is
forbidden.

http://codereview.chromium.org/8431016/diff/1/tests/language/src/TypeVariable...
File tests/language/src/TypeVariableScopeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/TypeVariable...
tests/language/src/TypeVariableScopeTest.dart:42: new I(null);
Without this call, error #04 may not be detected because the factory is never
invoked and so it is legal not to check its body.

[ngeoffray, 2011-11-01 08:17:34]

Nice catches Jim/Frog :) These are really corner cases of the language, and I
hope the few answers I give make sense.

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library2Nega...
File tests/language/src/Library2NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library2Nega...
tests/language/src/Library2NegativeTest.dart:11: #import("library2.dart");
On 2011/10/31 23:36:31, jimhug wrote:
> This seems totally broken to me without this line.  Imports are not transitive
> so members that are imported by library3 or library4 should not be exposed to
> classes that import them.  If anything, I think it could be valuable to keep
the
> old version of this test around as a positive test since both dartc and the vm
> appear to be generating an error here.

The problem is that the libraries are not imported with a prefix. So if you do a
full analysis of what is in the scope of that file, you'll end up with two
definitions of 'foo'.

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library3Nega...
File tests/language/src/Library3NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library3Nega...
tests/language/src/Library3NegativeTest.dart:8: // "fooC".
The comment is right, and you should not need to replace fooE to foo (which I
believe is not defined?)

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
File tests/language/src/OverrideFieldMethod1NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
tests/language/src/OverrideFieldMethod1NegativeTest.dart:16: new B().foo();
I think that's OK, considering that we allow the engine to report errors as
lazily as possible.

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
File tests/language/src/OverrideFieldMethod2NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
tests/language/src/OverrideFieldMethod2NegativeTest.dart:16: new B().foo();
ditto

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
File tests/language/src/OverrideFieldMethod3NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
tests/language/src/OverrideFieldMethod3NegativeTest.dart:16: new B().foo(10);
ditto

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
File tests/language/src/OverrideFieldMethod4NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
tests/language/src/OverrideFieldMethod4NegativeTest.dart:16: print(new B().foo);
is the print to make sure the code is not dead?

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
File tests/language/src/OverrideFieldMethod5NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
tests/language/src/OverrideFieldMethod5NegativeTest.dart:16: print(new B().foo);
same question

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideMeth...
File tests/language/src/OverrideMethodWithFieldTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideMeth...
tests/language/src/OverrideMethodWithFieldTest.dart:25: print(s.instanceMethod);
On 2011/10/31 23:36:31, jimhug wrote:
> This instanceMethod field overriding the super method is not necessarily an
> error until it is referred to.

Because you've allocated a Sub, I'm not sure here. Shouldn't we analyze the
class at the point an instance is allocated?

http://codereview.chromium.org/8431016/diff/1/tests/language/src/ThirdTest.dart
File tests/language/src/ThirdTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/ThirdTest.da...
tests/language/src/ThirdTest.dart:6: class A {
On 2011/10/31 23:36:31, jimhug wrote:
> This appeared totally broken to me according to the spec.  A and B both define
> static var s and static method foo.  When A extends B, these static fields
will
> shadow each other which appears illegal to me when reading the spec on Static
> Methods and Static Fields.  Like the other case that looks truly broken, it
may
> be worth keeping this around as a negative test to verify that this is
> forbidden.

Agree about the negative test. And it's great that frog found an error on this
test. Yay for conformance!

http://codereview.chromium.org/8431016/diff/1/tests/language/src/TypeVariable...
File tests/language/src/TypeVariableScopeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/TypeVariable...
tests/language/src/TypeVariableScopeTest.dart:42: new I(null);
On 2011/10/31 23:36:31, jimhug wrote:
> Without this call, error #04 may not be detected because the factory is never
> invoked and so it is legal not to check its body.

OK.

[kasperl, 2011-11-01 11:11:28]

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library2Nega...
File tests/language/src/Library2NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library2Nega...
tests/language/src/Library2NegativeTest.dart:11: #import("library2.dart");
On 2011/10/31 23:36:31, jimhug wrote:
> This seems totally broken to me without this line.  Imports are not transitive
> so members that are imported by library3 or library4 should not be exposed to
> classes that import them.  If anything, I think it could be valuable to keep
the
> old version of this test around as a positive test since both dartc and the vm
> appear to be generating an error here.

To me it looks like importing library4 alone is enough to give us the issue
(library4 defines foo and imports foo from library2). Shouldn't this fail the
same way if you remove the library3 importing?

[jimhug, 2011-11-01 15:07:10]

Thanks for your feedback!

It looks like there may be some open questions around the two
Library*NegativeTests as well as the OverrideMethodWithFieldTest.  I tried to
explain my thoughts more clearly, but these may be harder to work through.  If
we can't get a resolution on those quickly, I propose to remove them from this
CL and create a new CL in order to discuss them at more leisure.

I'm happy to resolve the use of print in any way that Nicolas or Kasper prefers.
 I think that it is best to do this in a way that doesn't generate an obvious
unused field reference warning in the future - but even that is okay with me.

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library2Nega...
File tests/language/src/Library2NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library2Nega...
tests/language/src/Library2NegativeTest.dart:11: #import("library2.dart");
Here's my take:

Importing just library4 and then accessing foo from within library4 would be an
error.  However, just importing library4 and then accessing foo from this file
should work correctly and return the foo defined in library4.  Am I missing
something?  Related: The library3 import in this file is not needed and I just
left it in in order to keep my changes minimal (hi, Nicolas <smile>)  the change
would be better if that was removed too.

On 2011/11/01 11:11:29, kasperl wrote:
> On 2011/10/31 23:36:31, jimhug wrote:
> > This seems totally broken to me without this line.  Imports are not
transitive
> > so members that are imported by library3 or library4 should not be exposed
to
> > classes that import them.  If anything, I think it could be valuable to keep
> the
> > old version of this test around as a positive test since both dartc and the
vm
> > appear to be generating an error here.
> 
> To me it looks like importing library4 alone is enough to give us the issue
> (library4 defines foo and imports foo from library2). Shouldn't this fail the
> same way if you remove the library3 importing?

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library3Nega...
File tests/language/src/Library3NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library3Nega...
tests/language/src/Library3NegativeTest.dart:8: // "fooC".
This is a typo in my code and I agree that this should be fooE as you say. 
However, there is a key change that I made in libraryE - that I believe should
be required to ensure the checks happen.

On 2011/11/01 08:17:34, ngeoffray wrote:
> The comment is right, and you should not need to replace fooE to foo (which I
> believe is not defined?)

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library3Nega...
tests/language/src/Library3NegativeTest.dart:17: Expect.equals(0, foo);
Fix this back to fooE.

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
File tests/language/src/OverrideFieldMethod4NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
tests/language/src/OverrideFieldMethod4NegativeTest.dart:16: print(new B().foo);
On 2011/11/01 08:17:34, ngeoffray wrote:
> is the print to make sure the code is not dead?

It was just my hack for a way to write a field reference that didn't look
totally silly.  This should work just fine without the print - but I anticipate
a near-term future where code like that will generate warnings from the compiler
about field accesses whose values are being ignored.

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
File tests/language/src/OverrideFieldMethod5NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideFiel...
tests/language/src/OverrideFieldMethod5NegativeTest.dart:16: print(new B().foo);
same answer <smile>
On 2011/11/01 08:17:34, ngeoffray wrote:
> same question

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideMeth...
File tests/language/src/OverrideMethodWithFieldTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/OverrideMeth...
tests/language/src/OverrideMethodWithFieldTest.dart:25: print(s.instanceMethod);
That's the key question here.  When I allocate a Sub do I have to verify that
all of its members are correctly defined, or am I allowed to wait until the
first reference of such a member to perform the checks.  I would be happiest if
we could adopt that rule in order to allow the compiler to do the minimum amount
of work truly required to make a given program run.  I would then like to have
--compile_all be the flag that can force all of these checks to be done if the
programmer would prefer.

On 2011/11/01 08:17:34, ngeoffray wrote:
> On 2011/10/31 23:36:31, jimhug wrote:
> > This instanceMethod field overriding the super method is not necessarily an
> > error until it is referred to.
> 
> Because you've allocated a Sub, I'm not sure here. Shouldn't we analyze the
> class at the point an instance is allocated?

http://codereview.chromium.org/8431016/diff/3002/tests/language/src/libraryE....
File tests/language/src/libraryE.dart (right):

http://codereview.chromium.org/8431016/diff/3002/tests/language/src/libraryE....
tests/language/src/libraryE.dart:10: var fooE = fooC;
This is needed since importers of libraryE rely on the fact that fooC is an
ambiguous reference in this library.

[ngeoffray, 2011-11-01 15:18:31]

I'm fine with keeping the print, as well as being as flexible as we want for
error reporting, and I think the spec does allow really late error reporting.

It may happen that we don't want this late error reporting for the VM in some
situations though. So do we want the same behavior as the VM. Don't know.

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library2Nega...
File tests/language/src/Library2NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/1/tests/language/src/Library2Nega...
tests/language/src/Library2NegativeTest.dart:11: #import("library2.dart");
On 2011/11/01 15:07:11, jimhug wrote:
> Here's my take:
> 
> Importing just library4 and then accessing foo from within library4 would be
an
> error.  However, just importing library4 and then accessing foo from this file
> should work correctly and return the foo defined in library4.  Am I missing
> something?  Related: The library3 import in this file is not needed and I just
> left it in in order to keep my changes minimal (hi, Nicolas <smile>)  the
change
> would be better if that was removed too.
> 
> On 2011/11/01 11:11:29, kasperl wrote:
> > On 2011/10/31 23:36:31, jimhug wrote:
> > > This seems totally broken to me without this line.  Imports are not
> transitive
> > > so members that are imported by library3 or library4 should not be exposed
> to
> > > classes that import them.  If anything, I think it could be valuable to
keep
> > the
> > > old version of this test around as a positive test since both dartc and
the
> vm
> > > appear to be generating an error here.
> > 
> > To me it looks like importing library4 alone is enough to give us the issue
> > (library4 defines foo and imports foo from library2). Shouldn't this fail
the
> > same way if you remove the library3 importing?
> 

OK, I re-checked the source files. I think Kasper's right. library4 can not be
compiled properly because of duplicate definition of 'foo' and 'foo1'. I don't
know wrt the spec if we can delay the error reporting.

[jimhug, 2011-11-01 15:25:07]

PTAL

I think this addresses your comments.

[ngeoffray, 2011-11-01 15:25:50]

LGTM

[jimhug, 2011-11-01 15:35:17]

On 2011/11/01 15:25:50, ngeoffray wrote:
> LGTM

Thanks!  I'm running tests now using this command from the dart directory.  I
assume that if this passes I am good to commit?

./tools/test.py --arch=ia32 --component=vm,dartc -p color language

[ngeoffray, 2011-11-01 15:56:01]

On 2011/11/01 15:35:17, jimhug wrote:
> On 2011/11/01 15:25:50, ngeoffray wrote:
> > LGTM
> 
> Thanks!  I'm running tests now using this command from the dart directory.  I
> assume that if this passes I am good to commit?
> 
> ./tools/test.py --arch=ia32 --component=vm,dartc -p color language

Yes, that's the right command. You can drop the --arch, ia32 is taken by
default.

[Ivan Posva, 2012-01-11 01:10:03]

Some of these fixes are broken or seem very unmotivated. We will fix them back.

-Ivan

http://codereview.chromium.org/8431016/diff/10019/tests/language/src/Library2...
File tests/language/src/Library2NegativeTest.dart (right):

http://codereview.chromium.org/8431016/diff/10019/tests/language/src/Library2...
tests/language/src/Library2NegativeTest.dart:10: #import("library2.dart");
Why was this test changed? This was carefully crafted to not complain about
library3.dart, but about library4.dart and its import.

http://codereview.chromium.org/8431016/diff/10019/tests/language/src/ThirdTes...
File tests/language/src/ThirdTest.dart (right):

http://codereview.chromium.org/8431016/diff/10019/tests/language/src/ThirdTes...
tests/language/src/ThirdTest.dart:6: class A {
Why was this test changed?

http://codereview.chromium.org/8431016/diff/10019/tests/language/src/libraryE...
File tests/language/src/libraryE.dart (right):

http://codereview.chromium.org/8431016/diff/10019/tests/language/src/libraryE...
tests/language/src/libraryE.dart:10: var fooE = fooC;
This is not correct. The two libraries do already collide on top-level names.