Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10907028:
Standalone concrete type inference (not integrated in the compiler). (Closed)
|
Created:
8 years, 3 months ago by polux Modified:
8 years, 2 months ago CC:
reviews_dartlang.org, ahe Visibility:
Public. |
DescriptionStandalone concrete type inference (not integrated in the compiler).
Committed: https://code.google.com/p/dart/source/detail?r=13215
Patch Set 1 #Patch Set 2 : Sync with last changes in CL 10905039 #Patch Set 3 : Address Karl's comment in CL 10829376. #
Total comments: 75
Patch Set 4 : Address Karl's comments #Patch Set 5 : sync to head and address Karl's comments #Patch Set 6 : sync to head #Patch Set 7 : fix tests #
Messages
Total messages: 5 (0 generated)
The function "AnalysisResult analyze(String code)" of the test file is a bit more complex than needed: eventually the result of type inference will be available from the compiler, which will take care of running it.
LGTM. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... File lib/compiler/implementation/types/concrete_types_inferrer.dart (right): http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:13: * A non-abstract type like [: int :], [: Point :], the null base type or the Isn't a non-abstract type a singleton concrete type? Perhaps that would fit better with the terminology in the other comments. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:16: interface BaseType extends Hashable { Please make this an abstract class, interface is going away. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:28: bool operator ==(BaseType other) { 'operator ==' -> 'operator equals' (also in multiple places below). http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:80: class ConcreteTypeFactory { You do not need this class. Please move the factories in this class to ConcreteType. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:123: if (this === other) return true; This check is not necessary by the new semantics of ==. (Also, the operator is called equals now). http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:182: ConcreteTypeCartesianProductIterator(baseTypeOfThis, this.baseTypeOfThis http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:236: BaseTypes(Compiler compiler) : A compiler is not necessary, a Types instance should be enough here. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:256: ConcreteType lookupTypeOfThis() => (typeOfThis === null) Please use curly braces if the body does not fit on one line. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:273: other.environment.forEach((elem, type) { 'elem' -> 'element' http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:285: if (this === other) return true; see above. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:384: // --- read-only state --- I think this is clear without the comment. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:399: // --- read-write state --- ditto. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:407: /** One line comment? (Also the next five comments.) http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:415: final Map<Element, ConcreteType> inferredFieldTypes; Is there a reason that this is not a Map<FieldElement, ConcreteType>? The same question goes for the signature of methods accessing this field. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:438: final Map<Element, ConcreteType> inferredParameterTypes; Map<VariabelElement, ConcreteType>? http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:442: ConcreteTypesInferrer(Compiler compiler) I would not call the constructor a utility method :-) http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:560: ConcreteType getConcreteTypeOfParameter(Element parameter) => Please use curly braces. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:568: * class [function] is a method of or null if [function] is static. Rephrase second sentence to something like '[receiverType] must be null or [function] must be a member of the class of [receiverType]'? http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:585: for (final tuple in product) { Please type the variable tuple. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:598: final result = new Map<Element, ConcreteType>(); I would prefer all these local variables to be typed. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:651: new ConcreteType.singleton(const NullBaseType()); You may need a marker here to indicate that it was a provided argument, because the method's implementation can use ?p to distinguish a provided argument with value null from one that was not provided. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:657: ConcreteTypesEnvironment environment) { Please align the parameters. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:658: Map<ConcreteTypesEnvironment, ConcreteType> template = cache[function]; You could add a comment somewhere, that the term 'template' is a reference to the CPA paper. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:683: FunctionExpression tree = compiler.parser.parse(element); I think you should call element.parseNode(compiler). http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:748: if (template[item.environment] != concreteType) { How about 'if (template[item.environment] == concreteType) continue' here ... http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:751: if (methodCallers != null) { ... and 'if (methodCallers == null) continue' here? http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:764: // debug(); Remove debug code. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:838: this.environment) : super(elements); Please align parameters. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:906: //analyze(node.function); Remove. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:1028: ConcreteType type = new ConcreteType.empty(); For which construct is this? http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:1180: if (element !== null) { // node is a local variable or a field of this I would prefer the comments to be on their own lines. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:1200: result = result.union(inferrer.getSendReturnType(getter, Store the result if getSendReturnType in a local variable to make this more readable. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:1210: Element classElem = member.enclosingElement; 'classElem'-> 'classElement'. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:1217: Element getterOrField = (baseReceiverType as ClassBaseType).element Assign to local variable with type ClassBaseType.
Peter: as discussed I'm adding you as a reviewer in case you find the time to review my changes before Oct 1st. Since Karl already LGTM'd it I don't expect very big changes and so I will create new CLs that build upon this CL in the following weeks regardless of the status of the review. Of course I will port any further change to this CL to the new CLs. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... File lib/compiler/implementation/types/concrete_types_inferrer.dart (right): http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:13: * A non-abstract type like [: int :], [: Point :], the null base type or the It is true that a BaseType is a singleton concrete type but I think you've read my comment as: A non-abstract type like ((int, point) OR (the null base type) OR (the unknown base type)). While I meant: (A non-abstract type like (int, point)) OR (the null base type) OR (the unknown base type)). So I'm rephrasing. On 2012/09/11 14:41:35, karlklose wrote: > Isn't a non-abstract type a singleton concrete type? Perhaps that would fit > better with the terminology in the other comments. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:16: interface BaseType extends Hashable { On 2012/09/11 14:41:35, karlklose wrote: > Please make this an abstract class, interface is going away. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:28: bool operator ==(BaseType other) { I think this now the right thing to do (http://www.dartlang.org/articles/m1-language-changes/#syntax-for-defining-ope...) On 2012/09/11 14:41:35, karlklose wrote: > 'operator ==' -> 'operator equals' (also in multiple places below). http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:80: class ConcreteTypeFactory { On 2012/09/11 14:41:35, karlklose wrote: > You do not need this class. Please move the factories in this class to > ConcreteType. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:123: if (this === other) return true; On 2012/09/11 14:41:35, karlklose wrote: > This check is not necessary by the new semantics of ==. (Also, the operator is > called equals now). Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:182: ConcreteTypeCartesianProductIterator(baseTypeOfThis, On 2012/09/11 14:41:35, karlklose wrote: > this.baseTypeOfThis Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:236: BaseTypes(Compiler compiler) : I didn't find how to get ClassElements from a Types instance (except through its compiler field). Are you referring to the one in typechecker.dart? Assuming you meant another class in typechecker.dart, say TypeCheckerVisitor, which has fields of type DartType. I wouldn't mind reusing these fields but I still need to inject them in a subtype of BaseType if that's what you wanted to avoid. On 2012/09/11 14:41:35, karlklose wrote: > A compiler is not necessary, a Types instance should be enough here. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:256: ConcreteType lookupTypeOfThis() => (typeOfThis === null) On 2012/09/11 14:41:35, karlklose wrote: > Please use curly braces if the body does not fit on one line. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:273: other.environment.forEach((elem, type) { On 2012/09/11 14:41:35, karlklose wrote: > 'elem' -> 'element' Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:285: if (this === other) return true; On 2012/09/11 14:41:35, karlklose wrote: > see above. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:384: // --- read-only state --- On 2012/09/11 14:41:35, karlklose wrote: > I think this is clear without the comment. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:399: // --- read-write state --- On 2012/09/11 14:41:35, karlklose wrote: > ditto. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:407: /** On 2012/09/11 14:41:35, karlklose wrote: > One line comment? (Also the next five comments.) Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:415: final Map<Element, ConcreteType> inferredFieldTypes; Thered doesn't seem to be such a thing: it looks like a FieldElement is an Element with kind == FIELD but thats all. There's AbstractFieldElement but FieldParameterElement.fieldElement is of type VariableElement, not AbstractFieldElement, and their only supertype is Element. This heas lead me to think that FieldParameterElement.fieldElement is maybe not a field element but according to the "testSetters" test it seems like it is. On 2012/09/11 14:41:35, karlklose wrote: > Is there a reason that this is not a Map<FieldElement, ConcreteType>? The same > question goes for the signature of methods accessing this field. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:438: final Map<Element, ConcreteType> inferredParameterTypes; On 2012/09/11 14:41:35, karlklose wrote: > Map<VariabelElement, ConcreteType>? Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:442: ConcreteTypesInferrer(Compiler compiler) On 2012/09/11 14:41:35, karlklose wrote: > I would not call the constructor a utility method :-) Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:560: ConcreteType getConcreteTypeOfParameter(Element parameter) => On 2012/09/11 14:41:35, karlklose wrote: > Please use curly braces. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:568: * class [function] is a method of or null if [function] is static. I wanted to make clear it must be null only if it is not a static method so I rephrased a bit differently. Does it sound better? On 2012/09/11 14:41:35, karlklose wrote: > Rephrase second sentence to something like '[receiverType] must be null or > [function] must be a member of the class of [receiverType]'? http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:585: for (final tuple in product) { On 2012/09/11 14:41:35, karlklose wrote: > Please type the variable tuple. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:598: final result = new Map<Element, ConcreteType>(); I am now convinced the editor should have an "insert inferred types" refactoring :) On 2012/09/11 14:41:35, karlklose wrote: > I would prefer all these local variables to be typed. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:651: new ConcreteType.singleton(const NullBaseType()); Postponing to a later CL if you don't mind. On 2012/09/11 14:41:35, karlklose wrote: > You may need a marker here to indicate that it was a provided argument, because > the method's implementation can use ?p to distinguish a provided argument with > value null from one that was not provided. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:657: ConcreteTypesEnvironment environment) { It doesn't fit in 80 characters so according to the style guide it looks I did the right thing. Do you prefer this style? On 2012/09/11 14:41:35, karlklose wrote: > Please align the parameters. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:658: Map<ConcreteTypesEnvironment, ConcreteType> template = cache[function]; On 2012/09/11 14:41:35, karlklose wrote: > You could add a comment somewhere, that the term 'template' is a reference to > the CPA paper. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:683: FunctionExpression tree = compiler.parser.parse(element); On 2012/09/11 14:41:35, karlklose wrote: > I think you should call element.parseNode(compiler). Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:748: if (template[item.environment] != concreteType) { On 2012/09/11 14:41:35, karlklose wrote: > How about 'if (template[item.environment] == concreteType) continue' here ... Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:751: if (methodCallers != null) { On 2012/09/11 14:41:35, karlklose wrote: > ... and 'if (methodCallers == null) continue' here? Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:764: // debug(); On 2012/09/11 14:41:35, karlklose wrote: > Remove debug code. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:838: this.environment) : super(elements); On 2012/09/11 14:41:35, karlklose wrote: > Please align parameters. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:906: //analyze(node.function); On 2012/09/11 14:41:35, karlklose wrote: > Remove. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:1028: ConcreteType type = new ConcreteType.empty(); For sequences of statements. Added a comment here and on analyze. On 2012/09/11 14:41:35, karlklose wrote: > For which construct is this? http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:1180: if (element !== null) { // node is a local variable or a field of this On 2012/09/11 14:41:35, karlklose wrote: > I would prefer the comments to be on their own lines. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:1200: result = result.union(inferrer.getSendReturnType(getter, On 2012/09/11 14:41:35, karlklose wrote: > Store the result if getSendReturnType in a local variable to make this more > readable. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:1210: Element classElem = member.enclosingElement; On 2012/09/11 14:41:35, karlklose wrote: > 'classElem'-> 'classElement'. Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:1217: Element getterOrField = (baseReceiverType as ClassBaseType).element On 2012/09/11 14:41:35, karlklose wrote: > Assign to local variable with type ClassBaseType. Done.
http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... File lib/compiler/implementation/types/concrete_types_inferrer.dart (right): http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:28: bool operator ==(BaseType other) { Yes, you are right. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:236: BaseTypes(Compiler compiler) : Never mind, I forgot that we moved the basic types from Types to the places they are used. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:651: new ConcreteType.singleton(const NullBaseType()); No problem, maybe add a TODO? http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:657: ConcreteTypesEnvironment environment) { I prefer the new version, because it is harder to miss the first parameter, but feel free to follow the style guide here. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:657: ConcreteTypesEnvironment environment) { I prefer the new version, because I think it is harder to overlook the first parameter, but feel free to follow the style guide here.
http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... File lib/compiler/implementation/types/concrete_types_inferrer.dart (right): http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:651: new ConcreteType.singleton(const NullBaseType()); On 2012/09/26 11:48:52, karlklose wrote: > No problem, maybe add a TODO? Done. http://codereview.chromium.org/10907028/diff/6001/lib/compiler/implementation... lib/compiler/implementation/types/concrete_types_inferrer.dart:657: ConcreteTypesEnvironment environment) { On 2012/09/26 11:48:52, karlklose wrote: > I prefer the new version, because I think it is harder to overlook the first > parameter, but feel free to follow the style guide here. I'm fine with this version too! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
