Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18323004:
Add lookupInScope to DeclarationMirror. (Closed)
|
Created:
7 years, 5 months ago by Johnni Winther Modified:
7 years, 5 months ago CC:
reviews_dartlang.org, janicejl, Tate Mandel Visibility:
Public. |
DescriptionAdd lookupInScope to DeclarationMirror.
BUG=
R=amouravski@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24777
Patch Set 1 #
Total comments: 35
Patch Set 2 : Updated cf. comments #Patch Set 3 : Add test + fix implementation. #
Total comments: 8
Patch Set 4 : Updated comment. #Patch Set 5 : Rebased #
Messages
Total messages: 11 (0 generated)
Mostly nits, but a few requests from clarification. lgtm otherwise. Thanks for the quick turnaround! https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:376: * Looks up the potentially qualified [name] in the scope of this declaration. Nit: I think you can omit this doc comment since it (should/will?) be imported from the super class. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:380: // TODO(johnniwinther): Support lookup of constructors. Could you file a tracking bug for this and cc me? https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:384: if (result == null) return null; Nit: How about: if (result != null && result.isPrefix()) { result = result.lookup...; } if (result == null) return null; return _convert... https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:397: return _element == other._element && owner == other.owner; Nit: I'd move the everything past the && to a new line for readability. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1417: DeclarationMirror lookupInScope(String name) { Nit: lookupInScope(String name) => parameters.firstWhere((parameter) => parameter.simpleName == name, orElse: () => super.lookupInScope(name)); https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1695: DeclarationMirror _convertElementToDeclarationMirror(Dart2JsMirrorSystem system, A doc comment would be nice for readers of the code. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1695: DeclarationMirror _convertElementToDeclarationMirror(Dart2JsMirrorSystem system, While you're here anyway, is it possible to get the DeclarationMirror from an InstanceMirror? https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1703: if (element.isLibrary()) { Nit: One line this. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1716: if (element.isClass()) { Nit: One line this. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1726: Iterable<MemberMirror> members = Nit: Also, why are these local variables not vars? https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart:110: * Looks up the potentially qualified [name] in the scope of this declaration. What does "potentially qualified" mean? Further, could you add a bit more documentation on what is meant by scope? Perhaps say "lexical scope." I would also add: /** * ... * See also: * * * [Lexical Scope](https://www.dartlang.org/docs/dart-up-and-running/contents/ch02.html#ch02-lexical-scope) in Dart Up and Running. * * [Lexical Scoping](http://www.dartlang.org/docs/spec/latest/dart-language-specification.html#h.jb82efuudrc5) in the Dart Specification. */ https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart:235: if (declaration == null) { Nit: One line this? https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/dartdoc/lib... File sdk/lib/_internal/dartdoc/lib/dartdoc.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/dartdoc/lib... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:2023: if (declaration == null) { Nit: qOne line these? https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/dartdoc/lib... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:2029: DeclarationMirror lookupResult; Nit: Why not just reuse declaration?
https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart:111: * For methods and constructors, the scope includes the parameters. For parameterized classes, does the scope include the type parameters?
lgtm
https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:376: * Looks up the potentially qualified [name] in the scope of this declaration. On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Nit: I think you can omit this doc comment since it (should/will?) be imported > from the super class. Done. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:380: // TODO(johnniwinther): Support lookup of constructors. On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Could you file a tracking bug for this and cc me? Done. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:384: if (result == null) return null; On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Nit: How about: > > if (result != null && result.isPrefix()) { > result = result.lookup...; > } > > if (result == null) return null; > return _convert... Done. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:397: return _element == other._element && owner == other.owner; On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Nit: I'd move the everything past the && to a new line for readability. Done. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1417: DeclarationMirror lookupInScope(String name) { On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Nit: > > lookupInScope(String name) => > parameters.firstWhere((parameter) => parameter.simpleName == name, > orElse: () => super.lookupInScope(name)); I find such use of untyped inlined closures very hard to read. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1695: DeclarationMirror _convertElementToDeclarationMirror(Dart2JsMirrorSystem system, On 2013/07/01 17:46:26, Andrei Mouravski wrote: > A doc comment would be nice for readers of the code. Done. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1695: DeclarationMirror _convertElementToDeclarationMirror(Dart2JsMirrorSystem system, On 2013/07/01 17:46:26, Andrei Mouravski wrote: > While you're here anyway, is it possible to get the DeclarationMirror from an > InstanceMirror? No. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1703: if (element.isLibrary()) { On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Nit: One line this. Done. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1716: if (element.isClass()) { On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Nit: One line this. Done. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1726: Iterable<MemberMirror> members = On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Nit: Also, why are these local variables not vars? In dart2js we deviate from the style guide at this point. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart:110: * Looks up the potentially qualified [name] in the scope of this declaration. On 2013/07/01 17:46:26, Andrei Mouravski wrote: > What does "potentially qualified" mean? > > Further, could you add a bit more documentation on what is meant by scope? > Perhaps say "lexical scope." > > I would also add: > > /** > * ... > * See also: > * > * * [Lexical > Scope](https://www.dartlang.org/docs/dart-up-and-running/contents/ch02.html#ch02-lexical-scope) > in Dart Up and Running. > * * [Lexical > Scoping](http://www.dartlang.org/docs/spec/latest/dart-language-specification.html#h.jb82efuudrc5) > in the Dart Specification. > */ Done. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart:111: * For methods and constructors, the scope includes the parameters. On 2013/07/01 18:57:30, sra1 wrote: > For parameterized classes, does the scope include the type parameters? Yes. Added note in the documentation. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart:235: if (declaration == null) { On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Nit: One line this? Done. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/dartdoc/lib... File sdk/lib/_internal/dartdoc/lib/dartdoc.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/dartdoc/lib... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:2023: if (declaration == null) { On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Nit: qOne line these? Done. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/dartdoc/lib... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:2029: DeclarationMirror lookupResult; On 2013/07/01 17:46:26, Andrei Mouravski wrote: > Nit: Why not just reuse declaration? Done.
lgtm https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1417: DeclarationMirror lookupInScope(String name) { On 2013/07/03 05:47:57, Johnni Winther wrote: > On 2013/07/01 17:46:26, Andrei Mouravski wrote: > > Nit: > > > > lookupInScope(String name) => > > parameters.firstWhere((parameter) => parameter.simpleName == name, > > orElse: () => super.lookupInScope(name)); > > I find such use of untyped inlined closures very hard to read. Well, you can type them, but I think the actual semantics of "firstWhere" makes more sense than "for" which is short circuited in your code. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1695: DeclarationMirror _convertElementToDeclarationMirror(Dart2JsMirrorSystem system, On 2013/07/03 05:47:57, Johnni Winther wrote: > On 2013/07/01 17:46:26, Andrei Mouravski wrote: > > While you're here anyway, is it possible to get the DeclarationMirror from an > > InstanceMirror? > > No. Aww. :[
I'll add a unittest later today. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1695: DeclarationMirror _convertElementToDeclarationMirror(Dart2JsMirrorSystem system, On 2013/07/03 07:18:57, Andrei Mouravski wrote: > On 2013/07/03 05:47:57, Johnni Winther wrote: > > On 2013/07/01 17:46:26, Andrei Mouravski wrote: > > > While you're here anyway, is it possible to get the DeclarationMirror from > an > > > InstanceMirror? > > > > No. > > Aww. :[ What use-case do you have for such feature?
Added a test - and found a lot of problems in the implementation. PTAL https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1417: DeclarationMirror lookupInScope(String name) { On 2013/07/03 07:18:57, Andrei Mouravski wrote: > On 2013/07/03 05:47:57, Johnni Winther wrote: > > On 2013/07/01 17:46:26, Andrei Mouravski wrote: > > > Nit: > > > > > > lookupInScope(String name) => > > > parameters.firstWhere((parameter) => parameter.simpleName == name, > > > orElse: () => super.lookupInScope(name)); > > > > I find such use of untyped inlined closures very hard to read. > > Well, you can type them, but I think the actual semantics of "firstWhere" makes > more sense than "for" which is short circuited in your code. Tried to change this, but I got weird type errors! Will leave this as is for now.
Mostly minor things. https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/18323004/diff/1/sdk/lib/_internal/compiler/im... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1417: DeclarationMirror lookupInScope(String name) { On 2013/07/03 12:58:01, Johnni Winther wrote: > On 2013/07/03 07:18:57, Andrei Mouravski wrote: > > On 2013/07/03 05:47:57, Johnni Winther wrote: > > > On 2013/07/01 17:46:26, Andrei Mouravski wrote: > > > > Nit: > > > > > > > > lookupInScope(String name) => > > > > parameters.firstWhere((parameter) => parameter.simpleName == name, > > > > orElse: () => super.lookupInScope(name)); > > > > > > I find such use of untyped inlined closures very hard to read. > > > > Well, you can type them, but I think the actual semantics of "firstWhere" > makes > > more sense than "for" which is short circuited in your code. > > Tried to change this, but I got weird type errors! Will leave this as is for > now. That's strange. Could I ask you to file a bug for the weird type errors? https://codereview.chromium.org/18323004/diff/17001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/18323004/diff/17001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:375: DeclarationMirror lookupInScope(String name) { This should have a doccomment, especially since it might be ambiguous which String to give it. Like, do you pass in "[:foo.bar:]" or "foo.bar". https://codereview.chromium.org/18323004/diff/17001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:380: if (index != -1) { This code got a lot more complicated. Let me see if I can parse it: // if theres no '.' // then look up id in current scope. // if there's something that looks like a prefix (what happens with "library com.google.foo"?) // then lookup the element given by that prefix. // if it is a Prefix, // then return the member given by the postfix It seems odd that this works. What does it do on library names like "com.google.foo"? I just feel like the nesting of the ifs is really hard to read or decipher. Maybe: ... lookupInScope(String name) { Element getSourceString(String name) { if (name.contains('.')) new SourceString(name.substring(0, index)); return new SourceString(name); } Element result = getSourceString(name); if (result != null && result.isPrefix()) { result = result.lookupLocalMember( new SourceString(name.substring(name.indexOf('.') + 1))) } if (result == null || result.isPrefix()) return null; return _convertElement...' } https://codereview.chromium.org/18323004/diff/17001/tests/compiler/dart2js/me... File tests/compiler/dart2js/memory_compiler.dart (right): https://codereview.chromium.org/18323004/diff/17001/tests/compiler/dart2js/me... tests/compiler/dart2js/memory_compiler.dart:7: import 'package:expect/expect.dart'; Nit: we've been formatting imports as: dart:imports \n package:imports \n local/relative dir imports https://codereview.chromium.org/18323004/diff/17001/tests/compiler/dart2js/me... tests/compiler/dart2js/memory_compiler.dart:21: Compiler compilerFor(Map<String,String> memorySourceFiles, A brief doc comment for this would be useful. https://codereview.chromium.org/18323004/diff/17001/tests/compiler/dart2js/me... tests/compiler/dart2js/memory_compiler.dart:24: Uri script = currentDirectory.resolve(nativeToUriPath(Platform.script)); I'm always bad with paths: does this require that the current directory is some specific place? https://codereview.chromium.org/18323004/diff/17001/tests/compiler/dart2js/mi... File tests/compiler/dart2js/mirrors_lookup_test.dart (right): https://codereview.chromium.org/18323004/diff/17001/tests/compiler/dart2js/mi... tests/compiler/dart2js/mirrors_lookup_test.dart:262: Too many newlines at end of file.
https://codereview.chromium.org/18323004/diff/17001/sdk/lib/_internal/compile... File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart (right): https://codereview.chromium.org/18323004/diff/17001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:375: DeclarationMirror lookupInScope(String name) { On 2013/07/03 17:49:49, Andrei Mouravski wrote: > This should have a doccomment, especially since it might be ambiguous which > String to give it. Like, do you pass in "[:foo.bar:]" or "foo.bar". Updated the doccomment on the declaration. https://codereview.chromium.org/18323004/diff/17001/sdk/lib/_internal/compile... sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:380: if (index != -1) { On 2013/07/03 17:49:49, Andrei Mouravski wrote: > This code got a lot more complicated. Let me see if I can parse it: > > // if theres no '.' > // then look up id in current scope. > // if there's something that looks like a prefix (what happens with "library > com.google.foo"?) > // then lookup the element given by that prefix. > // if it is a Prefix, > // then return the member given by the postfix > > It seems odd that this works. What does it do on library names like > "com.google.foo"? > > I just feel like the nesting of the ifs is really hard to read or decipher. > > Maybe: > > ... lookupInScope(String name) { > Element getSourceString(String name) { > if (name.contains('.')) new SourceString(name.substring(0, index)); > return new SourceString(name); > } > > Element result = getSourceString(name); > > if (result != null && result.isPrefix()) { > result = result.lookupLocalMember( > new SourceString(name.substring(name.indexOf('.') + 1))) > } > > if (result == null || result.isPrefix()) return null; > return _convertElement...' > } Lookup of libraries is not supported. They are not only ambiguous but also not in scope. The reason why the method is so complicated is a that we have to handle prefixes since are not part of the mirror system and therefore cannot be returned directly. The split with the getSourceString method doesn't work since it will allow us to lookup [: prefix.prefix :] by 'prefix'. For instance lookup the [:Future:] class through 'Future' when import like import 'dart:async' as Future;
Message was sent while issue was closed.
Committed patchset #5 manually as r24777 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
