Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid trapping exceptions by using reflection #59

Merged
merged 2 commits into from
May 10, 2017
Merged

Avoid trapping exceptions by using reflection #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5463b64
Avoid trapping exceptions by using reflection
jcollins-g May 8, 2017
2af6c5b
Run dartfmt on all files and fix if statement to hide reflection when…
jcollins-g May 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions lib/mustache_context.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,30 @@ class _MustacheContext extends MustacheToString implements MustacheContext {
}

dynamic _getActualValue(String key) {
try {
return ctx[key];
} catch (NoSuchMethodError) {
//Try to make dart2js understand that when we define USE_MIRRORS = false
//we do not want to use any reflector
return (useMirrors && USE_MIRRORS) ? ctxReflector[key] : null;
//Try to make dart2js understand that when we define USE_MIRRORS = false
//we do not want to use any reflector as that inflates the generated
//javascript.
if (useMirrors && USE_MIRRORS) {
if (reflect(ctx).type.instanceMembers.containsKey(new Symbol("[]"))) {
MethodMirror m = reflect(ctx).type.instanceMembers[new Symbol("[]")];
TypeMirror reflectedString = reflectType(String);
if (reflectedString.isAssignableTo(m.parameters[0].type)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like dart2js has some problems with this one:

00:26 +8 -10: Deep subcontext test [E]                                                             
  UnimplementedError
  dart:_internal                                      dart<.JsTypeMirror.isAssignableTo
  package:mustache4dart/mustache_context.dart 257:9   dart<._ObjectReflector.fieldValue
  package:mustache4dart/mustache_context.dart 241:26  dart<._ObjectReflector.$index
  package:mustache4dart/mustache_context.dart 123:14  dart<._MustacheContext._getActualValue
  package:mustache4dart/mustache_context.dart 100:13  dart<._MustacheContext._getContextForKey
  package:mustache4dart/mustache_context.dart 69:18   dart<._MustacheContext._getInThisOrParent
  package:mustache4dart/mustache_context.dart 65:12   dart<._MustacheContext.field
  test/mustache_context_test.dart 192:12              dart<.main.<fn>

I'm afraid I have to trap some exception after all. Do you mind sharing your usecase? Maybe I can try the exception trapping deeper in order not to hit the performance .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That exception wouldn't be trapped in any case, because UnimplementedError is not a subclass of NoSuchMethodError. So it seems unlikely that reverting this change would correct that test. Does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, never mind... isAssignableTo isn't implemented. Ouch.

Well, trapping the exception isn't the problem, it is catching it that impacts performance in the vm. As long as catching it doesn't introduce a performance problem in dart2js, it should be safe to do that from my case's perspective.

try {
return ctx[key];
} catch (NoSuchMethodError) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe, here a null should be returned. I am not sure though, I have to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, if (useMirrors && USE_MIRRORS) is true and the attempt to use ctx[key] resulted in an exception, we returned ctxReflector[key]. So a noop here falls through to the base case of returning ctxReflector[key]. The other two if statements just try to get us to return ctxReflector[key] if we can predict in advance that calling ctx[key] will raise an exception.

//This should never happen unless we were trapping a lower level
//NoSuchMethodError before. Continue to do so to be bug-for-bug
//compatible.
}
}
}
return ctxReflector[key];
} else {
try {
return ctx[key];
} catch (NoSuchMethodError) {
return null;
}
}
}

Expand Down
5 changes: 1 addition & 4 deletions lib/src/mustache.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ render(String template, Object context,
bool errorOnMissingProperty: false,
bool assumeNullNonExistingProperty: true}) {
return compile(template,
partial: partial,
delimiter: delimiter,
ident:
ident)(context,
partial: partial, delimiter: delimiter, ident: ident)(context,
out: out,
errorOnMissingProperty: errorOnMissingProperty,
assumeNullNonExistingProperty: assumeNullNonExistingProperty);
Expand Down