Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10947024:
Made dart2js constructor lookup logic "private"-aware, fixed 4740 bug. (Closed)
|
Created:
8 years, 3 months ago by aam-me Modified:
8 years, 2 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionMade dart2js constructor lookup logic "private"-aware, fixed 4740 bug.
BUG=4740
TEST=dart2js_extra/4740_test.dart
Committed: https://code.google.com/p/dart/source/detail?r=13557
Patch Set 1 #
Total comments: 4
Patch Set 2 : lookupConstructor now uses Selector. Introduced SelectorName class. #Patch Set 3 : Fixed indentation. #
Total comments: 7
Patch Set 4 : Another try in implementing constructor lookup, without SelectorName this time. #
Total comments: 25
Patch Set 5 : Another attempt at implementing private-aware constructor lookup logic - with normalized constructo… #
Total comments: 3
Patch Set 6 : Rebased. #Patch Set 7 : Use resolver.enclosingElement. Fixed line wrapping. #
Total comments: 14
Patch Set 8 : Removed normalizedConstructorName from Selector. #
Total comments: 6
Patch Set 9 : Removed class name parameter from Selector.callDefaultConstructor() constructor. #Patch Set 10 : Fixed co19-dart2js.status merge error. #
Total comments: 28
Patch Set 11 : Incorporated Peter's comments and feedback. #Messages
Total messages: 34 (0 generated)
Karl, hopefully you don't mind me looking at the issue 4740 you own. I was interested in this bug, now am curious what you think about proposed solution. Thanks!
First impressions: this approach looks good. I haven't carefully reviewed the changes yet. Adding Johnni as he is thinking about this area.
I haven't looked closely at the details of how you compute the library in all cases (that's the tricky part), but I thought I'd share my idea on where we should be heading with this. The suggestions here do not necessarily have to be addressed in this CL. I think we should start using selectors for looking things up. They contain the name and the library and they are intended to be used as the key for all kinds of method lookups. Furthermore, I think it would be nice if we could aim for creating all selectors in the resolver. Thoughts? https://chromiumcodereview.appspot.com/10947024/diff/1/lib/compiler/implement... File lib/compiler/implementation/compile_time_constants.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/1/lib/compiler/implement... lib/compiler/implementation/compile_time_constants.dart:756: superClass.lookupConstructor(enclosingClass.getLibrary(), Could we start using the selector to lookup the constructor instead? We compute the selector below and it contains all the information we need. https://chromiumcodereview.appspot.com/10947024/diff/1/lib/compiler/implement... File lib/compiler/implementation/elements/elements.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/1/lib/compiler/implement... lib/compiler/implementation/elements/elements.dart:1278: Element lookupConstructor(LibraryElement fromLibrary, In general, I think we should start using selectors as keys for these kind of lookups. https://chromiumcodereview.appspot.com/10947024/diff/1/tests/compiler/dart2js... File tests/compiler/dart2js_extra/4740_int_library.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/1/tests/compiler/dart2js... tests/compiler/dart2js_extra/4740_int_library.dart:12: FooInterface._internal(); That's a whole lot of indentation. https://chromiumcodereview.appspot.com/10947024/diff/1/tests/compiler/dart2js... File tests/compiler/dart2js_extra/4740_test.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/1/tests/compiler/dart2js... tests/compiler/dart2js_extra/4740_test.dart:10: FooBar() : super._internal(); /// 01: compile-time error Less indentation.
On 2012/09/19 05:59:17, kasperl wrote: > I think we should start using selectors for looking things up. They contain the > name and the library and they are intended to be used as the key for all kinds > of method lookups. Furthermore, I think it would be nice if we could aim for > creating all selectors in the resolver. Thoughts? That's a great idea. Alexander, are you interested in doing that now?
LGTM. Thank you for doing this, Alexander. As Kasper said, using selectors would be nicer. If you don't intend to change it in this CL, please add a TODO on ClassElement.lookupConstructor.
Thank you, guys, for the feedback!
I will look into changing ClassElement.lookupConstructor(LibraryElement
fromLibrary, SourceString className, [SourceString constructorName = const
SourceString(''),...) to ClassElement.lookupConstructor(Selector selector,...).
Please, take a look at lookupConstructor Selector-related changes when you have a chance. Could very well be that I went a little too far with creating SelectorName, Selector.callConstructor. Let me know what you think. Motivation behind SelectorName is to represent composite cosntructor name(class1.constructorName1). Also use it in the future to support external library references (library1.someName or library1.class1.ConstructorName). One thing I needed right away was isPrivate() working for such constructor names(Uri._fromMatch), so that was what led to SelectorName. Thanks!
Thanks a lot for working on this, Alexander -- and sorry for the long response time. In general, I don't really like the SelectorName construct and I'm not sure I understand why we need it for "external" library references long term. The selector isn't supposed to select a specific constructor across the code base, but rather to be used as a key for looking up a constructor in a specific class (at least that's how it works with the other selectors). Maybe the best thing is to just have a different kind of selector (CALL_CONSTRUCTOR) and let the name in such constructor be the constructor name and not have anything to do with the class name? Of course you can also encode the class name in the selector name if that makes it easier (see specific comments below). https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... File lib/compiler/implementation/elements/elements.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... lib/compiler/implementation/elements/elements.dart:1279: SourceString className = selector.selectorName.className; This makes it look like all you really need is to have the name you stuff in the selector be pre-normalized somehow so that it contains everything you need to do the lookup? I understand there may be some complications in figuring out a selector is private or not (based on the name alone), but maybe we could change Selector.isPrivate to say yes if Selector.library != null and let the name in a selector for a private named constructor be something like 'C._internal'? https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... File lib/compiler/implementation/universe/universe.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:97: class SelectorName { I'm not quite happy with this abstraction. Why do all selectors (indirectly) need a class name? Why do we have both a library name and the real library in all selectors? It seems a bit ... weird to me :-) https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:104: SelectorName(name, {libName: const SourceString('')}) It makes me a bit uncomfortable to have these defaults for library names that just happen to usually be empty. If we can make the resolver build all these selectors, it would be nice if it could always pass the current library into the factory methods -- not using defaults of any kind. It's fine if the selector ignores the library unless the name indicates that the selector is private. https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:213: Selector.callConstructor(SelectorName name, This looks nice and it looks like a good place to maybe take two arguments -- one with the class name and one with the constructor name. If you're given a constructor name you can check that too see if the selector is private. You could even add a Selector.callDefaultConstructor that only takes a class name.
Kasper, thank you for the review. I might have misunderstood how Selectors supposed to handle cases where library is part of name in the source code. Please see responses to your comments. Hopefully once this clarified I will get on the same page and fix the solution. We can have a [video]chat if that would be more productive.
Here are the responses to the comments I intended to send with the message. https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... File lib/compiler/implementation/elements/elements.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... lib/compiler/implementation/elements/elements.dart:1279: SourceString className = selector.selectorName.className; On 2012/09/21 12:12:06, kasperl wrote: > This makes it look like all you really need is to have the name you stuff in the > selector be pre-normalized somehow so that it contains everything you need to do > the lookup? Right. > I understand there may be some complications in figuring out a selector is > private or not (based on the name alone), but maybe we could change > Selector.isPrivate to say yes if Selector.library != null and let the name in a > selector for a private named constructor be something like 'C._internal'? Right, we can do that. My thoughts we that storing name normalized (lib name, class name, constructor name) is better than parsing name every time we do lookup. But then again, parsing here is just splitting by '.', so it is not that expensive. https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... File lib/compiler/implementation/universe/universe.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:97: class SelectorName { On 2012/09/21 12:12:06, kasperl wrote: > I'm not quite happy with this abstraction. Why do all selectors (indirectly) > need a class name? Why do we have both a library name and the real library in > all selectors? It seems a bit ... weird to me :-) Main reason for having library in SelectorName and library in Selector is that first one is part of the name(as in lib.A), while second one is context from where you are looking for that lib.A. Second "context" library name in Selector only makes sense for private names selectors. https://chromiumcodereview.appspot.com/10947024/diff/11001/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:104: SelectorName(name, {libName: const SourceString('')}) This libName represents names which have library name explicitly specified as in "var x = new lib1.clsClass();" or in "var y = new lib2.clsClass.namedConstructor();". Right?
Thanks, Alexander. I should be available for a quick chat next week. For now, I'll try to explain how (I think) we should deal with constructor calls for the complicated case where we're doing something like: new lib.C.foo() Internally, we deal with the lib.C.foo part as a send. We statically resolve the send to a reference to a constructor (a function element) and we associate the constructor with the send of the new expression (using useElement in the resolver). This is also where we should be creating a selector that will tell the SSA builder how to deal with the arguments for the constructor call (we already do this). If we let the constructor call selector contain the name of the constructor (if any) so we can tell if it's private or not and the library from which the selector is used, then we can let the either the resolver or the builder (resolver is probably the best option) see if the resolved constructor (the function element) is in the same library as the selector and disallow the call if not. The point is really that once we've resolved a new expression to be associated with a certain function element, there's no need to carry around enough information to do the resolution again (the lib.C.foo part). You're only really interested in how to pass the arguments (that's part of the selector) because the later stages of the compiler will just deal with a new expression as a static call to a resolved function. Does that make sense?
Thank you for detailed response, Kasper. I think it does make sense and I uploaded new patch to reflect my understanding of what you are describing. Devil as usual is in the details, so I'm looking forward to hear your feedback. Thanks!
LGTM. Having looked through your code, I think it would be better to get rid of the class prefix in the name of the selector and just construct the full constructor name in the resolver when you need it. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/dart_backend/placeholder_collector.dart:449: new Selector.callConstructor(receiver.source, // class name Maybe add two local variables (className and constructorName) to avoid having to add these end-of-line comments? 4 space indent. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/elements/elements.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/elements/elements.dart:1594: static SourceString constructConstructorNameFromOneName(SourceString name) { Somehow it would be simpler if the name stored in the selector was the normalized one. Where do we need it to contain the dot? https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/elements/elements.dart:1595: var strName = name.slowToString(); Try to avoid abbreviations. I'd go for dotIndex and nameString in this case. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/resolver.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/resolver.dart:112: constructor.getLibrary())); I'm a bit worried about getting the library from various different elements. It is so easy to get the wrong library which will end up messing with our privacy mechanism. Would it make sense to have a helper function in the resolver for creating new call-constructor (and other) selectors that automatically get the library from: resolver.enclosingElement.getLibrary() WDYT? https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/resolver.dart:619: new Selector.callConstructor(className, 4 space indent. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/resolver.dart:2662: selector.name, So there are two places where you need the full name of a constructor including the class prefix (here and in line 627). In both places you know what the class is. Maybe just create a helper function in the resolver that takes a class (or maybe a className) and a call-constructor selector and constructs the human readable full constructor name? At least this shouldn't be the reason why you store "full" names in the selectors (which is a bit complicated in the selector implementation and when "normalizing" it for lookup). https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/universe/universe.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:140: : <SourceString>[]; Somewhere you lost the assert(!name.isPrivate() || library != null); assertion. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:191: Selector.callConstructor(SourceString className, I know I sort of led you to a design where the selector name for call-constructor selectors would be of the form "C" or "C.named", but maybe it would be simpler just to have it be "" or "named". That way you can always tell if the selector is private by looking at the first character of the name. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:195: (constructorName === const SourceString('')) Can't you make it so this will never be called with an empty constructor name (so users will be forced to use callDefaultConstructor instead)? That would simplify the logic a bit here. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:200: constructorName.isPrivate()? library: null, Space before ? and before :.
I'm not sure about this. The string manipulation concerns me. I think it is error prone and inefficient. I'm not sure it is necessary except for dealing with default implementation of interfaces that are going away anyways. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/dart_backend/placeholder_collector.dart:449: new Selector.callConstructor(receiver.source, // class name On 2012/09/24 05:52:38, kasperl wrote: > Maybe add two local variables (className and constructorName) to avoid having to > add these end-of-line comments? > > 4 space indent. This code does not belong in the dart_backend to begin with. I don't think we should modify this code or try to get it to work. The dart backend should not do resolution, or attempt to implement features that we have not yet implemented in the front-end. If this code breaks, so be it. I do not want to waste time on two resolvers. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/elements/elements.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/elements/elements.dart:1594: static SourceString constructConstructorNameFromOneName(SourceString name) { I'm uncomfortable about adding this method. The method above, "constructConstructorName", should be sufficient (but is a hack). https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/universe/universe.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:132: Selector._noLibraryCheck( This is getting really confusing now. First, I'd like to avoid using privacy as it doesn't work for testing. Second, I don't see why we would ever want to not check that a library is provided if the name is private. So it seems to me that this change should just be reverted. Am I missing something? https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:199: "${constructorName.slowToString()}"), I think this is problematic. The constructor name may be private, but the compound name does not preserve this property. https://chromiumcodereview.appspot.com/10947024/diff/17002/tests/compiler/dar... File tests/compiler/dart2js_extra/4740_int_library.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/tests/compiler/dar... tests/compiler/dart2js_extra/4740_int_library.dart:11: interface FooInterface default Foo { Interfaces and default clauses are going away in M1. How much of this change is needed to support deprecated features?
Thank you for the feedback! Let me try again. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/universe/universe.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:132: Selector._noLibraryCheck( This implementation doesn't rely on name prefixed with '_' to determine privacy. Instead it uses presence of a library for that: library !== null -> isPrivate. That's why there is this constructor, no assert below and method isPrivate explicitly spelled out. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:191: Selector.callConstructor(SourceString className, Let me give it a try! https://chromiumcodereview.appspot.com/10947024/diff/17002/tests/compiler/dar... File tests/compiler/dart2js_extra/4740_int_library.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/tests/compiler/dar... tests/compiler/dart2js_extra/4740_int_library.dart:11: interface FooInterface default Foo { Good point. Initial 4740 bug didn't concern interfaces. I added interfaces later for better coverage. Should I take them out?
Please, take a look again when you have a chance. I removed "string-heavy" logic from Selector, adding only normalized constructor name property to it. That is 'name' property is either class or constructor name and in case of constructor name is used to determine privacy level. To address point regarding ensuring that correct constructor is called (callConstructor vs callDefaultConstructor) I reshuffled resolver's resolveSuperOrThis.../resolveImplicit... logic. Also, full dot-separated constructor names are generated on the fly only when error message is produced as they are used only for that. Thanks! https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/dart_backend/placeholder_collector.dart:449: new Selector.callConstructor(receiver.source, // class name Okay, I removed this file from the changelist. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/elements/elements.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/elements/elements.dart:1594: static SourceString constructConstructorNameFromOneName(SourceString name) { Okay, got rid of the method and storing normalized name in Selector now. Dotted names are needed for diagnostic only, it seems. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/resolver.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/resolver.dart:112: constructor.getLibrary())); Having helper function in the resolver (ResolverVisitor I assume, right?) would only work when you create selector from ResolverVisitor or when ResolveVisitor is available. But about half of the constructors selectors are created without ResolverVistor at hand - inside ResolverTask, for example. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... File lib/compiler/implementation/universe/universe.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:132: Selector._noLibraryCheck( I reverted this in most recent implementation. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:191: Selector.callConstructor(SourceString className, Okay, so name is just a constructor name, but now I store normalized name in a separate property. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:195: (constructorName === const SourceString('')) I'm not certain on how to ensure that callConstructor is never called with empty constructor name. At this point, every time I need to create selector I'm checking constructor name to decide which constructor to call. Not sure how to enforce it going forward, though. I have reshuffled logic in resolveSuperOrThisForSend/resolveImplicitSuperConstructorSend/resolveSuperOrThis to use either callConstructor or callDefaultConstructor explicitly. Not sure whether that was what you had in mind. https://chromiumcodereview.appspot.com/10947024/diff/17002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:199: "${constructorName.slowToString()}"), All this is gone now.
Sorry that I haven't made any (real) progress on reviewing this, Alexander -- I've been a bit under the weather and I've had a few too many other things to take care off too. I haven't forgotten about this, so I'll get through it soon and send you my comments. https://chromiumcodereview.appspot.com/10947024/diff/22001/lib/compiler/imple... File lib/compiler/implementation/resolver.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/22001/lib/compiler/imple... lib/compiler/implementation/resolver.dart:89: SourceString getConstructorName(Send node) => node.selector.asIdentifier().source; Line a bit too long. https://chromiumcodereview.appspot.com/10947024/diff/22001/lib/compiler/imple... lib/compiler/implementation/resolver.dart:96: ? classNameString 4 space indent. https://chromiumcodereview.appspot.com/10947024/diff/22001/lib/compiler/imple... lib/compiler/implementation/resolver.dart:114: constructor.getLibrary()); I'm still a bit worried about computing the library from different elements. As a next CL someone should probably try to add a helper to the resolver that creates these selectors using the library the resolver is current working in.
We will soon get rid of the "default class" feature. This means that we don't
have to support this:
interface Foo default Bar {
Foo.fisk();
}
class Bar {
Foo.fisk();
}
Or even:
class Bar {
libraryPrefix.Foo.fisk();
}
Also, it is not legal to have code like this:
class Foo {
Foo.named();
void named() {}
}
So we should be able to record constructors based on their name.
According to Lasse, lrn, we only have two interfaces left, List and String.
Lasse has a CL to turn these into abstract classes, but one VM test fails.
However, I think it would make sense if Alexander built on top of Lasse's work
and removed all the cruft for dealing with oddly named factories.
Alexander, we can do a hang out next week to discuss the details. Let me know if
you're available?
We have slightly more than two interfaces left (String and List requires VM changes that I haven't been able to do) and some other interfaces have const constructors which need redirecting factory constructors (which we/I just haven't implemented yet). Getting rid of interfaces is my next priority for library refurbishing.
FYI: My not-quite-complete patch for String/List is https://codereview.chromium.org/10942025
Yes, Peter, this makes perfect sense to me. I will start looking at Lasse's changelist with understanding that that is work in progress. Thank you, Kasper, for the comments. Hope you feel better soon!
Peter, Karl, Johnni, we talked yesterday about getting rid of normalizedName from Selector. This seems difficult to do since Selector is used to communicate _both_ className and constructorName pair from various places in resolver to ClassElement.lookupConstructor function in elements.dart. So we store constructor name in Selector.name, and normalized 'className$constructorName' in Selector.normalizedName. Again, this normalizedName is used by ClassElement.lookupConstructor in localLookup(normalizedName) call. This functionality seems to be orthogonal to deprecating interface/default classes. Could you, please, take another look and let me know what I'm missing. In latest patch I have addressed Kasper's comments regarding using resolver.enclosingElement more. I think there is only two places left where constructor Selector is built without using resolver.enclosingElement, where there is no resolver available. Thanks!
https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... File lib/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... lib/compiler/implementation/elements/elements.dart:1409: Element result = localLookup(selector.normalizedConstructorName); Can't you construct the normalized constructor name here from the class name in [this] ClassElement and the name in the selector? Alternatively, you should implement your own variant of localLookup that runs through local constructors (guaranteed to be on the right class) and see if the name in the selector matches? https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:99: ? classNameString 4 space indent of ? and :. https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:2786: ? new Selector.callDefaultConstructor( 4 space indent of ? and :. https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... File lib/compiler/implementation/universe/universe.dart (right): https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... lib/compiler/implementation/universe/universe.dart:126: final SourceString normalizedConstructorName; It would indeed be great to get rid of this and use an empty name for default constructors and the name of the constructor for the others. I'm okay with trying that out in another CL.
https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (left): https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:217: name = new SourceString(constructor.name.slowToString().replaceFirst( It is code like this I would like to get rid of. https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:239: constructor.defaultImplementation = defaultClass.lookupConstructor( Currently, lookupConstructor expects something like "MyClass$name". This is only to support this use case: interface List<E> default ListImplementation<E> { List.from(Iterable<E> other); } class ListImplementation<E> { factory List.from(Iterable<E> other) { ... } } Notice that the full factory name is not ListImplementation.from, but List.from. Soon we will be in a situation where we don't need that anymore. So I would like to avoid making changes to Selector to handle things that should be gone in a few weeks. Let me suggest the following: When storing the factory "List.from" in "ListImplementation" element, we currently use the name "List$from". Let's instead use the name "from" (it is an error to have two constructors, factories, or methods with the same name anyways). When looking up a constructor, we currently lookup "List$from". Instead, let's lookup "from".
Peter, please, find quick reply to your suggestion below. Thanks! https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:239: constructor.defaultImplementation = defaultClass.lookupConstructor( Peter, I see, but you can have multiple factories in one class, which have same constructor name, but target different classes. Storing just constructor name will cause conflicts. dart/lib/html/doc/nodoc-src/_TypedArrayFactoryProvider.dart has multiple fromList, fromBuffer constructors for different abstract classes: class _TypedArrayFactoryProvider { factory Float32Array(int length) => null; factory Float32Array.fromList(List<num> list) => null; factory Float32Array.fromBuffer( ArrayBuffer buffer, [int byteOffset = 0, int length]) => null; factory Float64Array(int length) => null; factory Float64Array.fromList(List<num> list) => null; factory Float64Array.fromBuffer( ArrayBuffer buffer, [int byteOffset = 0, int length]) => null; }
https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10947024/diff/35002/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:239: constructor.defaultImplementation = defaultClass.lookupConstructor( On 2012/10/08 15:03:35, aam wrote: > Peter, > > I see, but you can have multiple factories in one class, which have same > constructor name, but target different classes. Storing just constructor name > will cause conflicts. > dart/lib/html/doc/nodoc-src/_TypedArrayFactoryProvider.dart has multiple > fromList, fromBuffer constructors for different abstract classes: > > class _TypedArrayFactoryProvider { > > factory Float32Array(int length) => null; > factory Float32Array.fromList(List<num> list) => null; > factory Float32Array.fromBuffer( > ArrayBuffer buffer, [int byteOffset = 0, int length]) => null; > > factory Float64Array(int length) => null; > factory Float64Array.fromList(List<num> list) => null; > factory Float64Array.fromBuffer( > ArrayBuffer buffer, [int byteOffset = 0, int length]) => null; > } I'll send an email to Anton and see if he has time to do something about this.
Thank you for all your feedback! normalizedConstructorName is gone. Please, take another look. https://chromiumcodereview.appspot.com/10947024/diff/35002/lib/compiler/imple... File lib/compiler/implementation/elements/elements.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/35002/lib/compiler/imple... lib/compiler/implementation/elements/elements.dart:1409: Element result = localLookup(selector.normalizedConstructorName); Thanks for the tips. I gave it a try. https://chromiumcodereview.appspot.com/10947024/diff/35002/lib/compiler/imple... File lib/compiler/implementation/resolver.dart (left): https://chromiumcodereview.appspot.com/10947024/diff/35002/lib/compiler/imple... lib/compiler/implementation/resolver.dart:217: name = new SourceString(constructor.name.slowToString().replaceFirst( This code is gone, but is replaced with another string manipulation logic, that deals with named factory constructors implementing interfaces. https://chromiumcodereview.appspot.com/10947024/diff/35002/lib/compiler/imple... File lib/compiler/implementation/resolver.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/35002/lib/compiler/imple... lib/compiler/implementation/resolver.dart:99: ? classNameString On 2012/10/08 08:06:41, kasperl wrote: > 4 space indent of ? and :. Done. https://chromiumcodereview.appspot.com/10947024/diff/35002/lib/compiler/imple... lib/compiler/implementation/resolver.dart:239: constructor.defaultImplementation = defaultClass.lookupConstructor( Thanks, Peter. Further issues down the path of using just constructor name to identify factory methods and named constructors: 1) (from tests/language/factory_implementation_test.dart) === interface X default B { X(int x, int y); } class B { final int x; final int y; B(this.x, this.y); // #1 factory X(int a, int b) { return new XImpl(a,b); } // #2 } === Both constructors on lines #1 and #2 have same name - blank '', which causes name conflict and compile-time error. 2) currently constructors, methods and properties don't share same namespace, so following code is acceptable and works. It causes name conflict and compile-time error once you add "start" or "direct" to lookup tables to identify PipeServerGame.start() or _Proxy.direct(). (from tests/standalone/io/stream_pipe_test.dart, for example) === class PipeServerGame { ... PipeServerGame.start() { start(); } void start() { ... } ... } === (from runtime/bin/http_impl.dart) === class _Proxy { const _Proxy.direct() : host = null, port = null, direct = true; ... final bool direct; } === https://chromiumcodereview.appspot.com/10947024/diff/35002/lib/compiler/imple... lib/compiler/implementation/resolver.dart:2786: ? new Selector.callDefaultConstructor( On 2012/10/08 08:06:41, kasperl wrote: > 4 space indent of ? and :. Done. https://chromiumcodereview.appspot.com/10947024/diff/35002/lib/compiler/imple... File lib/compiler/implementation/universe/universe.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/35002/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:126: final SourceString normalizedConstructorName; On 2012/10/08 08:06:41, kasperl wrote: > It would indeed be great to get rid of this and use an empty name for default > constructors and the name of the constructor for the others. I'm okay with > trying that out in another CL. Done.
LGTM. https://codereview.chromium.org/10947024/diff/46001/lib/compiler/implementati... File lib/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/10947024/diff/46001/lib/compiler/implementati... lib/compiler/implementation/elements/elements.dart:1426: (constructorName.slowToString() != className.slowToString()))) { Shouldn't this be indented with an extra space? https://codereview.chromium.org/10947024/diff/46001/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10947024/diff/46001/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:315: num ndxDollar = constructorNameString.indexOf('\$'); I would avoid the abbreviation and call this 'int dollarIndex' or 'int indexOfDollar'. https://codereview.chromium.org/10947024/diff/46001/lib/compiler/implementati... File lib/compiler/implementation/universe/universe.dart (right): https://codereview.chromium.org/10947024/diff/46001/lib/compiler/implementati... lib/compiler/implementation/universe/universe.dart:198: Selector.callDefaultConstructor(SourceString name, What's the name in this case? Shouldn't it always be empty or am I missing something? Is it the class name in this case and does it have to be?
Thank you, Kasper, for the review! Please take another look when you have a chance. https://chromiumcodereview.appspot.com/10947024/diff/46001/lib/compiler/imple... File lib/compiler/implementation/elements/elements.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/46001/lib/compiler/imple... lib/compiler/implementation/elements/elements.dart:1426: (constructorName.slowToString() != className.slowToString()))) { On 2012/10/09 13:56:02, kasperl wrote: > Shouldn't this be indented with an extra space? Done. https://chromiumcodereview.appspot.com/10947024/diff/46001/lib/compiler/imple... File lib/compiler/implementation/resolver.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/46001/lib/compiler/imple... lib/compiler/implementation/resolver.dart:315: num ndxDollar = constructorNameString.indexOf('\$'); On 2012/10/09 13:56:02, kasperl wrote: > I would avoid the abbreviation and call this 'int dollarIndex' or 'int > indexOfDollar'. Done. https://chromiumcodereview.appspot.com/10947024/diff/46001/lib/compiler/imple... File lib/compiler/implementation/universe/universe.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/46001/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:198: Selector.callDefaultConstructor(SourceString name, Good point, thanks. I also have been using callDefaultConstructor in situation where callConstructor should have been used.
Still LGTM! Thanks for taking the time to work through all our comments, Alexander. https://chromiumcodereview.appspot.com/10947024/diff/38010/lib/compiler/imple... File lib/compiler/implementation/universe/universe.dart (right): https://chromiumcodereview.appspot.com/10947024/diff/38010/lib/compiler/imple... lib/compiler/implementation/universe/universe.dart:197: Selector.callDefaultConstructor(LibraryElement library) Very nice!
LGTM! All nits below. Perhaps you should land this and address any follow-up questions in a new CL. http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... File lib/compiler/implementation/elements/elements.dart (right): http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:1458: Element lookupConstructor(Selector selector, [Element noMatch(Element)]) { Perhaps in a future CL, it would be great to make noMatch a required argument. My suspicion is that most callers of this method would benefit from using the noMatch method. http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:1459: // TODO(karlklose): have a map from class names to a map of constructors I think this comment will soon be obsolete. http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:1477: [Element noMatch(Element)]) { I don't understand why noMatch is optional here. http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:1478: // TODO(karlklose): have a map from class names to a map of constructors Ditto. http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (left): http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:298: // TODO(ahe): Don't use string replacement here. Please keep this todo. I still think it is problematic that we're doing string manipulation here. Hopefully, we can clean this up in a few weeks. http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:90: bool isNamedConstructor(Send node) => node.receiver !== null; Add newline between methods. http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:91: SourceString getConstructorName(Send node) => When the function shorthand doesn't fit on one line, my preference is to use: { return expr; } http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:94: String createConstructorFullName(SourceString className, As far as I can tell, this is only used for diagnostics. That's great. I wonder if it should be renamed to something like: "constructorNameForDiagnostics"? http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:338: [selector.name, defaultClass.name]); I think we're using MyClass$foo here. Could you add a TODO and say this should use createConstructorFullName from above? http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:717: lookupTarget = lookupTarget.supertype.element; Just return? http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:2872: constructorName === const SourceString('') Only use == on SourceString. http://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:2877: resolver.enclosingElement.getLibrary()); I think I'm seeing this pattern a few times. Would it make sense to have a method for this? http://codereview.chromium.org/10947024/diff/38010/tests/co19/co19-dart2js.st... File tests/co19/co19-dart2js.status (right): http://codereview.chromium.org/10947024/diff/38010/tests/co19/co19-dart2js.st... tests/co19/co19-dart2js.status:256: Language/07_Classes/6_Constructors/1_Generative_Constructors_A16_t07: Fail # Redirecting constructors can not use initializing formals. This bug was previously masked - compilation failed, but for different, wrong reason. Is this a co19 bug?
Thank you, Peter, for the comments. I have incorporated changes you suggested. Please, take a look at the answers to your questions below. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... File lib/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/elements/elements.dart:1458: Element lookupConstructor(Selector selector, [Element noMatch(Element)]) { Added as TODO. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/elements/elements.dart:1459: // TODO(karlklose): have a map from class names to a map of constructors Removed the comment. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/elements/elements.dart:1477: [Element noMatch(Element)]) { Some users of lookupFactoryConstructor just check return result for null to determine whether there was a match. Would you recommend always use noMatch function instead? https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/elements/elements.dart:1478: // TODO(karlklose): have a map from class names to a map of constructors Removed the comment. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (left): https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:298: // TODO(ahe): Don't use string replacement here. On 2012/10/11 04:55:40, ahe wrote: > Please keep this todo. I still think it is problematic that we're doing string > manipulation here. Hopefully, we can clean this up in a few weeks. Done. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:90: bool isNamedConstructor(Send node) => node.receiver !== null; On 2012/10/11 04:55:40, ahe wrote: > Add newline between methods. Done. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:91: SourceString getConstructorName(Send node) => On 2012/10/11 04:55:40, ahe wrote: > When the function shorthand doesn't fit on one line, my preference is to use: > { > return expr; > } Done. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:94: String createConstructorFullName(SourceString className, On 2012/10/11 04:55:40, ahe wrote: > As far as I can tell, this is only used for diagnostics. That's great. I wonder > if it should be renamed to something like: "constructorNameForDiagnostics"? Done. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:338: [selector.name, defaultClass.name]); On 2012/10/11 04:55:40, ahe wrote: > I think we're using MyClass$foo here. Could you add a TODO and say this should > use createConstructorFullName from above? Done. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:717: lookupTarget = lookupTarget.supertype.element; On 2012/10/11 04:55:40, ahe wrote: > Just return? Done. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:2872: constructorName === const SourceString('') On 2012/10/11 04:55:40, ahe wrote: > Only use == on SourceString. Done. https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:2877: resolver.enclosingElement.getLibrary()); I don't think this code is used anywhere besides this place. But I moved it into createConstructorSelector method anyway. https://codereview.chromium.org/10947024/diff/38010/tests/co19/co19-dart2js.s... File tests/co19/co19-dart2js.status (right): https://codereview.chromium.org/10947024/diff/38010/tests/co19/co19-dart2js.s... tests/co19/co19-dart2js.status:256: Language/07_Classes/6_Constructors/1_Generative_Constructors_A16_t07: Fail # Redirecting constructors can not use initializing formals. This bug was previously masked - compilation failed, but for different, wrong reason. No, it is not. It is dart2js bug as far as I can tell.
LGTM! https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... File lib/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/10947024/diff/38010/lib/compiler/implementati... lib/compiler/implementation/elements/elements.dart:1477: [Element noMatch(Element)]) { On 2012/10/11 06:14:00, aam wrote: > Some users of lookupFactoryConstructor just check return result for null to > determine whether there was a match. > Would you recommend always use noMatch function instead? I would probably prefer to not have the nomatch function at all. But that depends on the performance impact. In general, I think we have a tendency to overuse optional arguments in the compiler. When there are less than a handful of callers of a method, I don't think it helps making stuff optional. |
