Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9007019:
Revised Mirrors proposal/take 3. (Closed)
|
Created:
9 years ago by gbracha Modified:
8 years, 11 months ago Reviewers:
iposva, dgrove, mattsh, lars.bak, Siggi Cherem (dart-lang), hausner, Ben Laurie (Chromium), jimhug, ahe CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionRevised Mirrors proposal/take 3.
Patch Set 1 #
Total comments: 52
Messages
Total messages: 11 (0 generated)
Hi, Here is the latest draft of the mirror API. A document will also be forthcoming by Xmas.
Forgot to add you to the list. This is the latest take on mirrors. Comments welcome. A document should arrive by Xmas.
Here are some comments and questions. http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart File mirrors/debug.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode2 mirrors/debug.dart:2: #import("introspection.dart"); Can you import a source file? Should this be #source instead of #import? http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode22 mirrors/debug.dart:22: Call the function at the current point in the calling frame, and stop at the first pc in that function. In this description you are implying that there is always a call at a breakpoint. That may be true for the actual generated code, but not at the source level. A more general question is: what is the granularity of step and next? BTW, I like the more familiar names stepInto and stepOver better. http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode33 mirrors/debug.dart:33: If it's null, we use TOS. Alternatively, step and next could be methods of the ActivationMirror class, returning a StackMirror object. http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode76 mirrors/debug.dart:76: Token pc(); I don't see Token defined anywhere. What can I do with a Token object? Also, implicitly generated code does not have a natural token position. For example, implicit constructors, or code generated in for-in loops to initialize and advance the implicit iterator object. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart File mirrors/introspection.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:146: Map<Object, InterfaceDeclarationMirror> interfaces(); Why do you use Object as the first type argument on all the maps? Why not String? http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:350: String source(); //{return vmMirror.sourceFor(declaringClass.reflectee, simpleName);} What's the rationale to have access to the source code?
http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart File mirrors/debug.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode2 mirrors/debug.dart:2: #import("introspection.dart"); On 2011/12/21 21:53:19, hausner wrote: > Can you import a source file? Should this be #source instead of #import? introspection should be a library. http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode22 mirrors/debug.dart:22: Call the function at the current point in the calling frame, and stop at the first pc in that function. On 2011/12/21 21:53:19, hausner wrote: > In this description you are implying that there is always a call at a > breakpoint. That may be true for the actual generated code, but not at the > source level. > I like stepInto and stepOver as well. As for stepping into, if there is no call at the current pc, one should specify what happens - for instance, nothing. > A more general question is: what is the granularity of step and next? for stepOver/next, perhaps there needs to be a distinction between evaluating the next subexpression and the next method invocation. > > BTW, I like the more familiar names stepInto and stepOver better. http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode33 mirrors/debug.dart:33: If it's null, we use TOS. On 2011/12/21 21:53:19, hausner wrote: > Alternatively, step and next could be methods of the ActivationMirror class, > returning a StackMirror object. Yes, but why? http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode76 mirrors/debug.dart:76: Token pc(); On 2011/12/21 21:53:19, hausner wrote: > I don't see Token defined anywhere. Indeed. It should be much the same as in the parser. What can I do with a Token object? Ask it for its start and end indices in the source code, what (file)stream it came from. > > Also, implicitly generated code does not have a natural token position. For > example, implicit constructors, or code generated in for-in loops to initialize > and advance the implicit iterator object. Yes, but should we be able to break there? http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart File mirrors/introspection.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:146: Map<Object, InterfaceDeclarationMirror> interfaces(); On 2011/12/21 21:53:19, hausner wrote: > Why do you use Object as the first type argument on all the maps? Why not > String? Because in some cases, the key should not be a standard String, but something else, like an index into the source code stream. Which may mean that we should use an abstraction like MirrorGroup to encapsulate this (as we did in thebeginning). http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:350: String source(); //{return vmMirror.sourceFor(declaringClass.reflectee, simpleName);} On 2011/12/21 21:53:19, hausner wrote: > What's the rationale to have access to the source code? IDEs
http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart File mirrors/debug.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode76 mirrors/debug.dart:76: Token pc(); On 2011/12/21 21:53:19, hausner wrote: > I don't see Token defined anywhere. What can I do with a Token object? > > Also, implicitly generated code does not have a natural token position. For > example, implicit constructors, or code generated in for-in loops to initialize > and advance the implicit iterator object. I'm hoping in Dart we can make an nice UI for stepping through things like foo(bar(baz() + 1)), and be able to highlight exactly the right tokens, as you progress through subexpressions of a larger statement. This makes me think that returning a single token for the pc (or even a range) is not going to be sufficient. We may need to be able to express a discontiguous range of tokens.
http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart File mirrors/debug.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode33 mirrors/debug.dart:33: If it's null, we use TOS. On 2011/12/22 18:29:59, gbracha wrote: > On 2011/12/21 21:53:19, hausner wrote: > > Alternatively, step and next could be methods of the ActivationMirror class, > > returning a StackMirror object. > > Yes, but why? Because then you don't have the issue you mention in the comment. The ActivationMirror would be the implicit argument (because it's the receiver). If we limit step/next to the top of stack frame, then they belong in StackMirror. http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode76 mirrors/debug.dart:76: Token pc(); On 2011/12/22 18:29:59, gbracha wrote: > On 2011/12/21 21:53:19, hausner wrote: > > I don't see Token defined anywhere. > > Yes, but should we be able to break there? > No, but there can be ActivationFrames on the stack that correspond to this generated code. An implicit constructor (which does not have source test associated with it) can call a user-defined constructor, and I can set a breakpoint there.
http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart File mirrors/debug.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode33 mirrors/debug.dart:33: If it's null, we use TOS. On 2011/12/23 00:22:16, hausner wrote: > On 2011/12/22 18:29:59, gbracha wrote: > > On 2011/12/21 21:53:19, hausner wrote: > > > Alternatively, step and next could be methods of the ActivationMirror class, > > > returning a StackMirror object. > > > > Yes, but why? > > Because then you don't have the issue you mention in the comment. The > ActivationMirror would be the implicit argument (because it's the receiver). > Good point. > If we limit step/next to the top of stack frame, then they belong in > StackMirror. I definitely do NOT want to limit step/next to TOS. http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode76 mirrors/debug.dart:76: Token pc(); On 2011/12/22 21:00:05, mattsh wrote: > On 2011/12/21 21:53:19, hausner wrote: > > > I'm hoping in Dart we can make an nice UI for stepping through things like > foo(bar(baz() + 1)), Of course. >>and be able to highlight exactly the right tokens, as you > progress through subexpressions of a larger statement. This makes me think that > returning a single token for the pc (or even a range) is not going to be > sufficient. We may need to be able to express a discontiguous range of tokens. So Token is probably the wrong word here. I assumed Tokens know where they begin and where they end, but apparently that isn't always the case. What I mean is a source location (and I will rename this to Location) which can tell you beginning and ending indices, the string to which those indices relate, and the URI where the string came from, if there is one. I don't see why a range is insufficient. I've built such tools before and a range has always been adequate. What is not adequate is line number! http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode76 mirrors/debug.dart:76: Token pc(); On 2011/12/23 00:22:16, hausner wrote: > On 2011/12/22 18:29:59, gbracha wrote: > > On 2011/12/21 21:53:19, hausner wrote: > > > I don't see Token defined anywhere. > > > > > Yes, but should we be able to break there? > > > > No, but there can be ActivationFrames on the stack that correspond to this > generated code. An implicit constructor (which does not have source test > associated with it) can call a user-defined constructor, and I can set a > breakpoint there. If no source code is available, one should be able to detect this and show the frame with something like "No source code available". Should we have a way of identifying auto-generated frames vai the API?
http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart File mirrors/debug.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/debug.dart#newcode76 mirrors/debug.dart:76: Token pc(); On 2011/12/23 00:41:23, gbracha wrote: > On 2011/12/22 21:00:05, mattsh wrote: > > On 2011/12/21 21:53:19, hausner wrote: > > > > > I'm hoping in Dart we can make an nice UI for stepping through things like > > foo(bar(baz() + 1)), > > Of course. > > >>and be able to highlight exactly the right tokens, as you > > progress through subexpressions of a larger statement. This makes me think > that > > returning a single token for the pc (or even a range) is not going to be > > sufficient. We may need to be able to express a discontiguous range of > tokens. > > So Token is probably the wrong word here. I assumed Tokens know where they begin > and where they end, but apparently that isn't always the case. What I mean is a > source location (and I will rename this to Location) which can tell you > beginning and ending indices, the string to which those indices relate, and the > URI where the string came from, if there is one. > > I don't see why a range is insufficient. I've built such tools before and a > range has always been adequate. What is not adequate is line number! > Right. I guess I was picturing that in this expression: foo(baz() + 1) first baz() will be highlighted, then, after step, "baz() + 1" will be highlighted, then the question is, do you want to highlight the whole continuous range "foo(baz() + 1)"? I suppose the answer is "yes", in which case a single continuous range is sufficient. But, if at that point you wanted to just highlight the "foo(" and the closing ")", without highlighting the baz() + 1 expression in the middle (because it has already executed), the we'd need discontinuous ranges.
Hi Gilad, I think we should just chat about this on Wednesday. Basically, I think we need three different things in relation to interfaces/classes: * An interface declaration aka InterfaceDeclarationMirror * An interface type invocation aka InterfaceTypeMirror * An interface mirror aka InterfaceMirror If we didn't have generic types, we wouldn't need both InterfaceDeclarationMirror and InterfaceTypeMirror. You might think that first-class libraries forces us to have both InterfaceDeclarationMirror and InterfaceMirror. However, as I mention below, the editor guys requested a CompilationUnitMirror in dartc, so there is still a need to be able to distinguish between interface Foo defined in file Foo.dart and the two different instance of Foo that arise when Foo.dart is sourced into both library1.dart and library2.dart. So in fact, supporting first-class libraries are not adding complexity that doesn't already exist in the domain. Fortunately, we don't have to dump this complexity on the casual user. If we move ObjectMirror and all its subclasses to its own library, most users of reflection would be able to stay in this simpler world. Cheers, Peter http://codereview.chromium.org/9007019/diff/1/mirrors/corelib_mirrors.dart File mirrors/corelib_mirrors.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/corelib_mirrors.dart#ne... mirrors/corelib_mirrors.dart:7: final List<Dynamic> positionalArguments; Nit: I see no value to using List<Dynamic> over List. http://codereview.chromium.org/9007019/diff/1/mirrors/corelib_mirrors.dart#ne... mirrors/corelib_mirrors.dart:18: the results without type warnings. Is this the best choice? Should we be more accurate and use Object instead? I prefer Map<String, Dynamic> over Map<String, Object>. http://codereview.chromium.org/9007019/diff/1/mirrors/corelib_mirrors.dart#ne... mirrors/corelib_mirrors.dart:23: be implemented by user code. I don't see a reason to prevent this interface from being implemented by user code. A compiler to JS can simply warn and say that this requires an extra symbol table. http://codereview.chromium.org/9007019/diff/1/mirrors/corelib_mirrors.dart#ne... mirrors/corelib_mirrors.dart:77: // typedef StackTraceMirror = List<InvocationMirror> I don't see how this typedef would make sense if we supported this kind of typedefs. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart File mirrors/introspection.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:1: #library("introspection"); According to merriam-webster, "introspection" means "a reflective looking inward". Later you say the primary gateway into the mirror system is the IsolateMirror, and as far as I can tell, there is no mechanism for reflecting on oneself. So "introspection" doesn't seem to be a good fit. "observation" seems to be a better fit. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:8: Should Mirror implement Hashable? Yes :-) http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:13: The keys will often by strings, but this is not required. All we require is that a key by -> be http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:26: // Dart language level A concrete example: a compiler may provide a mirror-based API for static analysis of source code. A bonus question: wouldn't a reflectee violate the principle of "stratification"? http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:34: final String kind; I'd prefer having a MirrorKind interface. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:69: StackMirror stack(); I think this name is problematic. The name should make it obvious that it suspends the isolate. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:76: /* Return a list of (mirrors for) all ports of the reflectee */ Which end of the ports? http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:93: I'd say G<Dynamic, ..., Dynamic> but I admit it seems weird. TBD. I'm not sure I understand this. A declaration of the class Foo corresponds to one class (per library). It does not matter how many type variables Foo has. However, I see that ObjectMirror.getClass() returns an InterfaceMirror instead of a TypeMirror. The following things should return a type mirror: * A supertype (superclass or super-interface) * The "class" of an ObjectMirror. A (generic) class (or interface) declaration with n type variables is a n-ary function from n-tuples of types to types. So like a MethodMirror has an InvocationMirror, an InterfaceMirror has a InterfaceTypeMirror that corresponds to a concrete invocation of a generic interface (or class). One might think that InterfaceTypeMirrors should be canonicalized, but in my experience, that is not necessary. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:123: Mirror lookupName(String s); Doesn't this method lookup a mirror? Using a String here is fine as long as the Maps doesn't. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:136: String simpleName(); Object may be better here. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:161: Map<Object, Mirror> declarations(); I think this method is important. Preferably, you'll replace all these maps with "mirror-groups". If not, it would ideal if we could specify that the Map preserves declaration order (for example, using a LinkedHashMap). As long as it can be implemented efficiently in the VM. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:171: // Includes? Not semantically meaningful, let's skip for now Everything in source code is important. Also, I'd like to have a library have a list of compilation units. This is something the editor team was requesting. Preferably a class declaration would know its CompilationUnit (mirror) which is how it knows about its LibraryDeclarationMirror. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:209: of these things? Do we raise an exception? Or provide a class (such as DartClass, DartInterface or DartLibrary)? Return null :-) http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:211: interface InterfaceMirror extends ObjectMirror, TypeMirror { From the perspective of using this library for static code analysis, I'd like to move ObjectMirror and all its subclasses to a separate library. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:211: interface InterfaceMirror extends ObjectMirror, TypeMirror { An InterfaceMirror is not a TypeMirror. See comments above. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:222: Map<Object, InterfaceMirror> superinterfaces(); See above comments about how to separate applications of generic types of their corresponding declaration. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:226: InterfaceMirror superclass(); This should return a TypeMirror. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:232: Map<Object, TypeMirror> typeArguments(); This does not belong here. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:392: InterfaceMirror getClass(); This should return a TypeMirror.
http://codereview.chromium.org/9007019/diff/1/mirrors/corelib_mirrors.dart File mirrors/corelib_mirrors.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/corelib_mirrors.dart#ne... mirrors/corelib_mirrors.dart:9: ArgumentDescriptor(List<Dynamic> this.positionalArguments, Map<String, Dynamic> this.namedArguments); make namedArguments an optional parameter? http://codereview.chromium.org/9007019/diff/1/mirrors/corelib_mirrors.dart#ne... mirrors/corelib_mirrors.dart:31: Would it be appropriate to add 'isConstructor' ? http://codereview.chromium.org/9007019/diff/1/mirrors/corelib_mirrors.dart#ne... mirrors/corelib_mirrors.dart:43: final _kind; why not type this as a String? http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart File mirrors/introspection.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:146: Map<Object, InterfaceDeclarationMirror> interfaces(); On 2011/12/22 18:29:59, gbracha wrote: > On 2011/12/21 21:53:19, hausner wrote: > > Why do you use Object as the first type argument on all the maps? Why not > > String? > > Because in some cases, the key should not be a standard String, but something > else, like an index into the source code stream. Which may mean that we should > use an abstraction like MirrorGroup to encapsulate this (as we did in > thebeginning). IMHO, I don't think using Object as a key is very helpful. This is a bit of a hack, but if you need to represent either dart identifiers or numeric offsets, you could just encode the number as a string. There would be no conflict, as leading numeric digits are not valid in identifiers. http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:263: //{return '{library.name}{simpleName}';} {libraryPrefix}.{simpleName}? Surely there is some kind of delimiter between the two name components. Also, I'm curious as to how we can be so sure the interface has a string name when before the code declared the map of interfaces with a key of 'Object'. (line 103, 108) http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:366: bool isFactory(); This may be evident from the naming convention, but what about isPrivate? http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:399: We distinguish getetr and setter calls based on the number of arguments (0/1). getetr -> getter
http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart File mirrors/introspection.dart (right): http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:12: One can always go through their values to look for mirrors with specific properties. line too long here and a couple other places http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:61: bool isActive(); Might be better to reverse this to be isSuspended (since the suspended state is less common, and it matches the terminology below) http://codereview.chromium.org/9007019/diff/1/mirrors/introspection.dart#newc... mirrors/introspection.dart:103: Map<Object, InterfaceMirror> classes(); Why do these methods return a Map, rather than simply a List of the mirrors? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
