Refactor resolution code in the vm to properly handle ambiguity errors.

Refactor resolution code in the vm to properly handle ambiguity errors.
Add test.

R=asiva@google.com

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

[regis, 2013-07-17 18:34:24]

Regression tests are still missing.
But please have a look and see if it makes sense.

[hausner, 2013-07-17 23:50:06]

Overall makes sense. I think you need to do the same error checking also when
looking up names in a library prefix namespace. See comments.

https://codereview.chromium.org/19662003/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/19662003/diff/1/runtime/vm/object.cc#newcode6757
runtime/vm/object.cc:6757: ASSERT(ambiguity_error_msg != NULL);
Could this not be NULL? There are places in the code where you pass NULL as an
argument, saying we don't expect any ambiguities. But is that assumption ok to
make?

https://codereview.chromium.org/19662003/diff/1/runtime/vm/object.cc#newcode7230
runtime/vm/object.cc:7230: RawClass* LibraryPrefix::LookupLocalClass(const
String& class_name) const {
I think you have to extend your checks for ambiguous names to this function as
well. The error case here is if I import 2 libraries with the same prefix P, and
the both libraries define name N, then it's ambiguous to refer to P.N.

[regis, 2013-07-18 20:00:24]

Thanks!

https://codereview.chromium.org/19662003/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/19662003/diff/1/runtime/vm/object.cc#newcode6757
runtime/vm/object.cc:6757: ASSERT(ambiguity_error_msg != NULL);
On 2013/07/17 23:50:06, hausner wrote:
> Could this not be NULL? There are places in the code where you pass NULL as an
> argument, saying we don't expect any ambiguities. But is that assumption ok to
> make?

In the case we do not expect any ambiguities, the caller could pass a non-null
string pointer and assert that no ambiguity was found, or it passes NULL and we
assert here instead. The result is the same, but passing NULL does not cost a
handle allocation in the caller.

Now, as you pointed out yesterday, we could detect such internal errors (which
are possible when the core library is under development, etc...) in release mode
as well by reporting an error instead of just asserting.

However, in many other places of the vm, we simply assert if we cannot resolve a
class of the core library. It would make sense to fix these first before
reporting errors for ambiguities in the library.

https://codereview.chromium.org/19662003/diff/1/runtime/vm/object.cc#newcode7230
runtime/vm/object.cc:7230: RawClass* LibraryPrefix::LookupLocalClass(const
String& class_name) const {
On 2013/07/17 23:50:06, hausner wrote:
> I think you have to extend your checks for ambiguous names to this function as
> well. The error case here is if I import 2 libraries with the same prefix P,
and
> the both libraries define name N, then it's ambiguous to refer to P.N.

Good point!
Done. I also renamed this function to LookupClass, since the lookup is not
local.

[regis, 2013-07-22 18:46:16]

I have added a test showing that dart API was previously failing to report
ambiguities.

[siva, 2013-07-22 22:21:41]

LGTM with one comment about NULL being passed in for tests.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/cha_test.cc
File runtime/vm/cha_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/cha_test.cc#ne...
runtime/vm/cha_test.cc:38: EXPECT(!class_a.IsNull());
This is a case where if we have some bugs the test instead of
failing will crash. I wonder if it is better to pass in a non
NULL value here.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/code_descripto...
File runtime/vm/code_descriptors_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/code_descripto...
runtime/vm/code_descriptors_test.cc:230: EXPECT(!cls.IsNull());
This is another similar case.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/code_generator...
File runtime/vm/code_generator_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/code_generator...
runtime/vm/code_generator_test.cc:276: }
Ditto case here in the places where LookupClass is used.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/compiler_test.cc
File runtime/vm/compiler_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/compiler_test....
runtime/vm/compiler_test.cc:47: EXPECT(!cls.IsNull());
Ditto comment.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_api_impl....
runtime/vm/dart_api_impl.cc:2052: isolate,
lib.LookupClassAllowPrivate(class_name, NULL));
As discussed this should probably just ASSERT(!cls.IsNull());

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_api_impl_...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_api_impl_...
runtime/vm/dart_api_impl_test.cc:8261: EXPECT(Dart_IsError(Dart_GetClass(lib,
NewString("baw"))));
Would be good to have a positive test here too.
Say add couple of names that do not clash in the two libraries
and look them up.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry.cc#...
runtime/vm/dart_entry.cc:450: NULL);  // No ambiguity error expected.
ASSERT(!function.IsNull());

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry.cc#...
runtime/vm/dart_entry.cc:482: NULL);  // No ambiguity error expected.
ASSERT(!function.IsNull());

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry.cc#...
runtime/vm/dart_entry.cc:517: NULL));  // No ambiguity error expected.
ASSERT(!function.IsNull());

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry_tes...
File runtime/vm/dart_entry_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry_tes...
runtime/vm/dart_entry_test.cc:32: EXPECT(!cls.IsNull());
Ditto comment about NULL for test cases.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/find_code_obje...
File runtime/vm/find_code_object_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/find_code_obje...
runtime/vm/find_code_object_test.cc:105: EXPECT(!clsB.IsNull());
Ditto comment about tests.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:6757: ASSERT(ambiguity_error_msg != NULL);
The one issue with asserting here for the NULL is that in the past
we would have gotten appropriate ASSERTS e.g func.IsNull or cls.IsNull
at the point where we do a lookup but now we will get a generic
ambiguity_error_msg == NULL assert here.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/parser_test.cc
File runtime/vm/parser_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/parser_test.cc...
runtime/vm/parser_test.cc:20: EXPECT(!cls.IsNull());
Ditto comment in these tests.

[regis, 2013-07-22 23:51:26]

Thanks!

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/cha_test.cc
File runtime/vm/cha_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/cha_test.cc#ne...
runtime/vm/cha_test.cc:38: EXPECT(!class_a.IsNull());
On 2013/07/22 22:21:46, siva wrote:
> This is a case where if we have some bugs the test instead of
> failing will crash. I wonder if it is better to pass in a non
> NULL value here.

Done.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/code_descripto...
File runtime/vm/code_descriptors_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/code_descripto...
runtime/vm/code_descriptors_test.cc:230: EXPECT(!cls.IsNull());
On 2013/07/22 22:21:46, siva wrote:
> This is another similar case.

Done.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/code_generator...
File runtime/vm/code_generator_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/code_generator...
runtime/vm/code_generator_test.cc:276: }
On 2013/07/22 22:21:46, siva wrote:
> Ditto case here in the places where LookupClass is used.

Done.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/compiler_test.cc
File runtime/vm/compiler_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/compiler_test....
runtime/vm/compiler_test.cc:47: EXPECT(!cls.IsNull());
On 2013/07/22 22:21:46, siva wrote:
> Ditto comment.

Done.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_api_impl....
runtime/vm/dart_api_impl.cc:2052: isolate,
lib.LookupClassAllowPrivate(class_name, NULL));
On 2013/07/22 22:21:46, siva wrote:
> As discussed this should probably just ASSERT(!cls.IsNull());

Done.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_api_impl_...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_api_impl_...
runtime/vm/dart_api_impl_test.cc:8261: EXPECT(Dart_IsError(Dart_GetClass(lib,
NewString("baw"))));
On 2013/07/22 22:21:46, siva wrote:
> Would be good to have a positive test here too.
> Say add couple of names that do not clash in the two libraries
> and look them up.

Done.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry.cc#...
runtime/vm/dart_entry.cc:450: NULL);  // No ambiguity error expected.
On 2013/07/22 22:21:46, siva wrote:
> ASSERT(!function.IsNull());

Done here and at other locations in this source.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry.cc#...
runtime/vm/dart_entry.cc:482: NULL);  // No ambiguity error expected.
On 2013/07/22 22:21:46, siva wrote:
> ASSERT(!function.IsNull());

Done.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry.cc#...
runtime/vm/dart_entry.cc:517: NULL));  // No ambiguity error expected.
On 2013/07/22 22:21:46, siva wrote:
> ASSERT(!function.IsNull());

Done.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry_tes...
File runtime/vm/dart_entry_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/dart_entry_tes...
runtime/vm/dart_entry_test.cc:32: EXPECT(!cls.IsNull());
On 2013/07/22 22:21:46, siva wrote:
> Ditto comment about NULL for test cases.

Done.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/find_code_obje...
File runtime/vm/find_code_object_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/find_code_obje...
runtime/vm/find_code_object_test.cc:105: EXPECT(!clsB.IsNull());
On 2013/07/22 22:21:46, siva wrote:
> Ditto comment about tests.

Done.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:6757: ASSERT(ambiguity_error_msg != NULL);
On 2013/07/22 22:21:46, siva wrote:
> The one issue with asserting here for the NULL is that in the past
> we would have gotten appropriate ASSERTS e.g func.IsNull or cls.IsNull
> at the point where we do a lookup but now we will get a generic
> ambiguity_error_msg == NULL assert here.

As suggested, I changed the callers in tests to pass a non-null value, so that
bad tests fail, but do not crash.

I have added a comment explaining what could have triggered the assert for other
cases where the caller did not expect an ambiguity error and passed NULL to
spare the allocation of a handle.

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/parser_test.cc
File runtime/vm/parser_test.cc (right):

https://codereview.chromium.org/19662003/diff/32001/runtime/vm/parser_test.cc...
runtime/vm/parser_test.cc:20: EXPECT(!cls.IsNull());
On 2013/07/22 22:21:46, siva wrote:
> Ditto comment in these tests.

Done.

[regis, 2013-07-23 00:19:55]

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