Change path-observer to lookup properties aggressively and report errors

Change path-observer to lookup properties aggressively and report errors
asynchronously. Add patch in smoke to work around a mirrors bug revealed by
these changes.

BUG=
R=jmesserly@google.com

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

[Siggi Cherem (dart-lang), 2014-03-25 21:44:24]

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:490: abstract class StringIndexer {
I was not sure about the name. Other ideas/suggestions?

[Jennifer Messerly, 2014-03-25 22:32:44]

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:84: /// A dot-delimieted property path
such as "foo.bar" or "foo.10.bar".
this was an issue before but: do you mind adding a newline after this intro
sentence?

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:89: /// or an [StringIndexer], while
integer values are read as indexes in an array.
should be "a [StringIndexer]"

also, this was an issue with the old comment too, but I think "indexes in a
[List]" is more accurate

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:254: var name =
smoke.symbolToName(property);
move this inside the if, below? and make _MAP_PROPERTIES use symbols then?

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:271: new Completer().completeError(e, s);
hmmm. Upon second look I'm not sure this is the right place to throw the error
async. _getObjectProperty is used by PropertyPath, which is just a normal sync
API, and therefore should throw errors synchronously, IMO. So any
catch-rethrow-async must happen at a higher level. 

I think it would be better to leave this code rethrowing, and we can file a bug
if we find cases where template_binding is not rendering other things after a
binding throws. 

BTW, I'm really sorry for leading you down a wild goose chase of async-ifying
this :-(. It didn't click for me until seeing the code in _getObjectProperty why
I had intentionally made it a synchronous throw.

I went ahead and filed https://code.google.com/p/dart/issues/detail?id=17789

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:490: abstract class StringIndexer {
On 2014/03/25 21:44:25, Siggi Cherem (dart-lang) wrote:
> I was not sure about the name. Other ideas/suggestions?

Indexable<K, V>? then do the is check for Indexable<String, dynamic>.

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/test/path_ob...
File pkg/observe/test/path_observer_test.dart (right):

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/test/path_ob...
pkg/observe/test/path_observer_test.dart:74: new Future(() {}).then((_) {
return the Future?

also does it work to start with:

return new Future(() {
  ...
}).then(...

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/test/path_ob...
pkg/observe/test/path_observer_test.dart:77: () => expect(new PathObserver(123,
'foo.bar.baz').value, null));
as noted earlier, I'd expect this error to be sync.

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/test/path_ob...
pkg/observe/test/path_observer_test.dart:85: return _asyncError('foo',
when i'm changing behavior w.r.t. JS I usually put in a note like:

Dart note: getting invalid properties is an error, unlike in JS where it returns
undefined.

maybe just one note at the top since it's common to several tests

https://codereview.chromium.org/211763002/diff/30001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/211763002/diff/30001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:173: operator[]=(String name, Object
value) {
spacing here seems a bit off

https://codereview.chromium.org/211763002/diff/30001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:174: throw new UnimplementedError('[]= is
not supported in Scope.');
UnsupportedError? in other words, the lack of a setter is intentional right?

[Siggi Cherem (dart-lang), 2014-03-26 00:32:07]

thanks John. PTAL (btw, the test might be easier to look at by ignoring
patch-set 1)

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:84: /// A dot-delimieted property path
such as "foo.bar" or "foo.10.bar".
On 2014/03/25 22:32:44, John Messerly wrote:
> this was an issue before but: do you mind adding a newline after this intro
> sentence?

Done.

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:89: /// or an [StringIndexer], while
integer values are read as indexes in an array.
On 2014/03/25 22:32:44, John Messerly wrote:
> should be "a [StringIndexer]"
> 
> also, this was an issue with the old comment too, but I think "indexes in a
> [List]" is more accurate

Done.

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:254: var name =
smoke.symbolToName(property);
On 2014/03/25 22:32:44, John Messerly wrote:
> move this inside the if, below? and make _MAP_PROPERTIES use symbols then?

Done.

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:271: new Completer().completeError(e, s);
On 2014/03/25 22:32:44, John Messerly wrote:
> hmmm. Upon second look I'm not sure this is the right place to throw the error
> async. _getObjectProperty is used by PropertyPath, which is just a normal sync
> API, and therefore should throw errors synchronously, IMO. So any
> catch-rethrow-async must happen at a higher level. 
> 
> I think it would be better to leave this code rethrowing, and we can file a
bug
> if we find cases where template_binding is not rendering other things after a
> binding throws. 
> 
> BTW, I'm really sorry for leading you down a wild goose chase of async-ifying
> this :-(. It didn't click for me until seeing the code in _getObjectProperty
why
> I had intentionally made it a synchronous throw.
> 
> I went ahead and filed https://code.google.com/p/dart/issues/detail?id=17789

No worries, changed it back to sync. Thanks for filing the bug.

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:490: abstract class StringIndexer {
On 2014/03/25 22:32:44, John Messerly wrote:
> On 2014/03/25 21:44:25, Siggi Cherem (dart-lang) wrote:
> > I was not sure about the name. Other ideas/suggestions?
> 
> Indexable<K, V>? then do the is check for Indexable<String, dynamic>.

Done.

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/test/path_ob...
File pkg/observe/test/path_observer_test.dart (right):

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/test/path_ob...
pkg/observe/test/path_observer_test.dart:74: new Future(() {}).then((_) {
On 2014/03/25 22:32:44, John Messerly wrote:
> return the Future?
> 
> also does it work to start with:
> 
> return new Future(() {
>   ...
> }).then(...

(obsolete)

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/test/path_ob...
pkg/observe/test/path_observer_test.dart:77: () => expect(new PathObserver(123,
'foo.bar.baz').value, null));
On 2014/03/25 22:32:44, John Messerly wrote:
> as noted earlier, I'd expect this error to be sync.

Done.

https://codereview.chromium.org/211763002/diff/30001/pkg/observe/test/path_ob...
pkg/observe/test/path_observer_test.dart:85: return _asyncError('foo',
On 2014/03/25 22:32:44, John Messerly wrote:
> when i'm changing behavior w.r.t. JS I usually put in a note like:
> 
> Dart note: getting invalid properties is an error, unlike in JS where it
returns
> undefined.
> 
> maybe just one note at the top since it's common to several tests

Done (made the note on the source code too)

https://codereview.chromium.org/211763002/diff/30001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/211763002/diff/30001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:173: operator[]=(String name, Object
value) {
On 2014/03/25 22:32:44, John Messerly wrote:
> spacing here seems a bit off

Done.

https://codereview.chromium.org/211763002/diff/30001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:174: throw new UnimplementedError('[]= is
not supported in Scope.');
On 2014/03/25 22:32:44, John Messerly wrote:
> UnsupportedError? in other words, the lack of a setter is intentional right?

Done.

[Jennifer Messerly, 2014-03-26 00:37:14]

Nice! LGTM

[Siggi Cherem (dart-lang), 2014-03-26 00:43:58]

Committed patchset #2 manually as r34399 (presubmit successful).