Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1462133005:
Downwards inference. This adds support to the resolver for downwards (Closed)
|
Created:
5 years, 1 month ago by Leaf Modified:
5 years ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionDownwards inference. This adds support to the resolver for downwards
type inference in strong mode. Downwards inference is removed from
the DDC checker in favor of the strong mode analyzer version.
BUG=
R=brianwilkerson@google.com, jmesserly@google.com
Committed: https://github.com/dart-lang/sdk/commit/64ab186b167d402e90db4e9740c4a408d42f42ca
Patch Set 1 #Patch Set 2 : Minor fixes #
Total comments: 102
Patch Set 3 : Address comments #Patch Set 4 : Address comments 2 #
Total comments: 6
Messages
Total messages: 18 (4 generated)
leafp@google.com changed reviewers: + brianwilkerson@google.com, jmesserly@google.com, paulberry@google.com
Ok, here's a cut at downwards inference for you guys to look at. I've gone ahead and removed the assertion on pruned typedefs. I'll change as necessary depending on the resolution of that discussion. All tests pass, and the DDC tests pass using this version of the analyzer.
scheglov@google.com changed reviewers: + scheglov@google.com
https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... File pkg/analyzer/test/utils.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:37: class AstFinder { Could you please move the methods of AstFinder into a separate library and make them functions? It seems that they have pretty unique names and may be easy to imported without (or with) a prefix.
LGTM once the infinite loop problem is fixed https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5759: /** Maintains and manages contextual type information used for Not asking for a change, but I'm not a fan of the abbreviated block style. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5794: /** Get the return type of the current enclusing function, if any. "enclusing" --> "enclosing" https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5800: * of t1 of the form K<S0, ..., Sm> where t2 = K<S0', ..., Sm'> What is 't2'? https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5811: if (_returnStack.isNotEmpty) { Add "assert(_returnStack.isNotEmpty);" before the if for safety during testing. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5827: if (info == null) return; Stylistic nit (for here and elsewhere): we always use blocks for control structures and always include type annotations. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5881: bool match(InterfaceType t1) { This method will result in an infinite loop if the type hierarchy is not well formed. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5900: for (final parent in t1.mixins) { Interesting that we do mixins after interfaces. I would have expected the opposite. Perhaps a comment explaining why would help. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10976: super.visitArgumentList(node); Missing return https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11022: return null; Or "return super.visitAwaitExpression(node);" https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11065: // TODO(leafp): Handle this case I won't remember what this means outside the context of this CL; more detail would be good. Perhaps: Use InferenceContext to propagate types downward. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11991: super.visitYieldStatement(node); Missing return https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... File pkg/analyzer/test/utils.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:37: class AstFinder { I'm not a fan of top-level functions; I'd actually prefer leaving it the way you have it. So I guess it's your call. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:211: /** Given assertions for the argument and return types, produce an Format the file?
Definitely LGTM! Lots of little suggestions. Feel free to take em or leave em, especially the style nits. The only "substantive" questions I had were around inferFormalParametersList (does it do anything for ctors/methods?) and if there are options other than an expando property for tracking the context type. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5759: /** Maintains and manages contextual type information used for On 2015/11/21 16:12:22, Brian Wilkerson wrote: > Not asking for a change, but I'm not a fan of the abbreviated block style. Yeah same here. I've been trying to follow: /** * The doc comment. */ ... for consistency. At some point, I guess we should run the auto conversion tool to get /// ? That would match the Dart style guide: https://www.dartlang.org/effective-dart/documentation/#do-use--doc-comments-t... https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5762: class InferenceContext { "Context" as a class suffix often just means "here's a bag of global variables" ... it's not all that descriptive. I seem to recall Vijay at some point steering me away from a "context" class in DDC. That said, it's definitely hard to name these things. I feel like every compiler eventually ends up with some "Context" types :). And here, we are using "contextual type information" so it's more sensible name than usual. But a lot of the state is the same bag of stuff we track frequently (Type provider/system/rules & error listener...) One possible solution is to make this object take a bit more of an active role, then it can be named after what it does. But that'd require moving methods to it. I haven't gotten far enough in the CL yet to know if that would make sense. Anyway this is probably an FYI comment for now, but something to noodle on. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5765: static const String _propertyName = might be worth giving this a more descriptive name, like "_contextTypeProperty". https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5782: final TypeRules _rules; Question: are we still on track to remove this object? I feel like it should be folded into StrongTypeSystemImpl. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5789: InferenceContext( BTW, if either the type or constructor can be made private, that'd be ideal. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5791: : this._typeProvider = typeProvider, nit: "this." is not necessary here https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5799: /** If t1 = I<dynamic, ..., dynamic>, then look for a supertype BTW: you probably want to put backticks around generic types, otherwise dartdoc might not render them well. I know by default many markdowns won't do the right thing. In other words: If `t1 = I<dynamic, ..., dynamic>`, then look for a supertype ... BTW, my CL might have the same problem, I'll double check... https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5800: * of t1 of the form K<S0, ..., Sm> where t2 = K<S0', ..., Sm'> On 2015/11/21 16:12:22, Brian Wilkerson wrote: > What is 't2'? looks like it's the second argument to matchTypes https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5900: for (final parent in t1.mixins) { On 2015/11/21 16:12:22, Brian Wilkerson wrote: > Interesting that we do mixins after interfaces. I would have expected the > opposite. Perhaps a comment explaining why would help. Yeah. I would normally think of mixins as coming before superclass as well. Mixins create a new superclass. Given class C extends S with M0, Mn {} what you really have is something like: class S { ... S methods ... } class M0' extends S { ... M0 methods ... } class Mn' extends M0' { ... Mn methods ... } class C extends Mn' { ... C methods ... } ... note also that the order is reversed (the last mixin instantiation, here called Mn', is actually our immediate superclass, followed by M0, followed by S) https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5923: static void annotateNode(AstNode node, DartType type) { (note: the following may be obsolete in light of my comment below) maybe this should be called "setContextType" or "setType" if we keep "Context" in the name? That way it would read like: InferenceContext.setType(node, type); I would also change "get" to something that corresponds. Either "getAnnotation" or "getType" or "typeOf" ... something along those lines... https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5924: node?.setProperty(_propertyName, type); hmmmm. Thinking on this more (having seen the use cases) ... could we get by with just a Map<AstNode, DartType>, stored inside our InferenceContext object? That would be a lot better than a permanent property on the AST node. The property here is essentially a field on AstNode... Ideally, if it's just temporary state, we can throw it away (let it be GC'd) once we're done with Resolver. Otherwise, if we need to keep the state around forever, maybe it should be an actual field on AstNode... https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5925: return; nit: return not needed here https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5935: // If t1 = I<dynamic, ..., dynamic>, then look for a supertype Is this supposed to be here? Consider folding it into the doc comment? (sometimes "sort members" will lose positions of non-doc comments, so I wanted to ask) https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10598: InferenceContext inferenceContext = null; hmmm, I guess this is for consistency, but `= null` is not required https://www.dartlang.org/effective-dart/usage/#dont-explicitly-initialize-var... https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10958: int index = 0; trivial: I'd probably just write this as a C-style for loop if iterating over two lists: for (int i = 0; i < normalCount; i++) { InferenceContext.annotateNode(positional[i], normalParameterTypes[i]); } you wouldn't need "required" either. Same thing below. (/me misses Enumerable.zip) https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10960: InferenceContext.annotateNode(argument, normalParameterTypes[index++]); you might be able to avoid some of the above logic using argument.correspondingParameterElement. (aside: correspondingParameterIndex might be more useful. I ran into this as well and ended up doing .indexOf(...) to find the FunctionType's parameter) https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10970: namedParameterTypes.containsKey(argument.name.label.name)) { not that it matters, but I'd be tempted to rewrite this to only do the map lookup once: if (argument is NamedExpression) { DartType type = namedParameterTypes[argument.name.label.name]; if (type != null) { ... } } https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11238: InferenceContext.annotateNode( might be worth a helper for this? InferenceContext.setTypeFromNode(thenExpression, node); https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11280: if (type is FunctionType) { I think this would always be the case for a constructor https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11281: _inferFormalParameterList(node.parameters, type); was just chatting about this--I'm not sure this could ever kick in for ConstructorDecl or MethodDecls, since the FunctionType is derived from the formal parameter types. I *think* they should exactly match. If not I would be curious when it kicks in. (it would work for local functions though) https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11300: FieldElement fieldElement = enclosingClass.getField(node.fieldName.name); Does `fieldName.staticElement` give you the field element? That would save a look up. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11777: if (node.methodName.staticElement is ExecutableElement) { suggestion, if you save this into a variable you can get type propagation: Element method = node.methodName.staticElement; if (method is ExecutableElement) { InferenceContext.annotateNode(node.argumentList, method.type); } https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11789: super.visitNamedExpression(node); not sure if Brian got tired of pointing it out ... but, there's a few more missing returns here & below. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11980: InterfaceType wrapperD = (_enclosingFunction.isSynchronous) nit: parens not needed https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:100: Object inferConstructorName(ConstructorName node, InterfaceType type) { oh funny, I read this the first time as "infer constructor name" ... Like you just write "new Foo" and it figures out which named constructor to call ;) I wonder if we could call it "inferConstructor" or "inferConstructorType"? That said, if there's a consistent infer$AstNodeName() pattern we want to follow, I'm okay with that. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:118: (p.type == null || p.type.isDynamic) && aside: this might be worth a TODO. Ideally we can handle implicit types consistently, rather than having to handle "null" and DynamicTypeImpl https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:460: // TODO(leafp) handle async et al for now should we skip if isGenerator or isAsync? Or is that implied by the below checks https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/gener... File pkg/analyzer/test/generated/resolver_test.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/gener... pkg/analyzer/test/generated/resolver_test.dart:12619: A<int, String> a0 = new A(3, "hello"); formatting is a little off in this string literal. not a big deal tho https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/gener... pkg/analyzer/test/generated/resolver_test.dart:12666: void test8() { // Currently we only allow variable constraints. Test that we reject. long line here https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... File pkg/analyzer/test/utils.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:27: /** The type of a function which given an [S0] and an S1, builds an assertion over [T]s. long line (& a few below) unfortunately dartfmt won't fix comments :( https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:180: class TypeAssertions { note: it'd be slightly nicer to make these be Matchers, which have better self-documenting error messages https://www.dartdocs.org/documentation/matcher/0.12.0%2B1/matcher/Matcher-cla...
Oh, and also the question about inheritance order w.r.t. mixins
Thanks for the comments! I've replied to those which may warrant further discussion: in particular, I'd be interest in hearing thoughts about using a map instead of using the property lists on nodes. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5782: final TypeRules _rules; On 2015/11/23 22:11:57, John Messerly wrote: > Question: are we still on track to remove this object? I feel like it should be > folded into StrongTypeSystemImpl. Yes. It's here only to produce the info nodes that we put into the error stream, since I didn't want to muck around with changing that right now. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5789: InferenceContext( On 2015/11/23 22:11:58, John Messerly wrote: > BTW, if either the type or constructor can be made private, that'd be ideal. The type has to be public since it's accessed by the static type analyzer (though I guess I could instead provide accessors on the resolver itself that forward to the context). What's the convention for private constructors in Dart? https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5827: if (info == null) return; On 2015/11/21 16:12:22, Brian Wilkerson wrote: > Stylistic nit (for here and elsewhere): we always use blocks for control > structures and always include type annotations. Sorry - pulled over from DDC code and I missed the style conversion. Will fix. :) https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5900: for (final parent in t1.mixins) { On 2015/11/23 22:11:58, John Messerly wrote: > On 2015/11/21 16:12:22, Brian Wilkerson wrote: > > Interesting that we do mixins after interfaces. I would have expected the > > opposite. Perhaps a comment explaining why would help. > > Yeah. I would normally think of mixins as coming before superclass as well. > Mixins create a new superclass. Given > > class C extends S with M0, Mn {} > > what you really have is something like: > > class S { ... S methods ... } > class M0' extends S { ... M0 methods ... } > class Mn' extends M0' { ... Mn methods ... } > class C extends Mn' { ... C methods ... } > > ... note also that the order is reversed (the last mixin instantiation, here > called Mn', is actually our immediate superclass, followed by M0, followed by S) I need to sit down and work through the details of this algorithm at some point. This code is still only an approximation of what it will should probably eventually be. The only way the order should matter is if there are multiple solutions. If there are multiple solutions, there's no guarantee that any particular order will give the best solution - it just depends on how the code is written. So eventually I have to decide whether just to fix an order and take the first, or find all solutions and pick the "best" (according to some heuristic). If looking at the mixins first makes sense to you guys, I'm happy working with that. I'll try to make up a couple of tests where the order matters. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5924: node?.setProperty(_propertyName, type); On 2015/11/23 22:11:57, John Messerly wrote: > hmmmm. Thinking on this more (having seen the use cases) ... could we get by > with just a Map<AstNode, DartType>, stored inside our InferenceContext object? > That would be a lot better than a permanent property on the AST node. The > property here is essentially a field on AstNode... Ideally, if it's just > temporary state, we can throw it away (let it be GC'd) once we're done with > Resolver. Otherwise, if we need to keep the state around forever, maybe it > should be an actual field on AstNode... Yeah, I think this would be fine. I initially didn't like the map approach, but I think I've warmed to it. I don't think the GC issue is significant, since any types annotated on the AST likely also appear in the element model, and the element model is more persistent than the AST, but nonetheless, it's not ideal. Anyone else have thoughts on this? https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10958: int index = 0; On 2015/11/23 22:11:57, John Messerly wrote: > trivial: I'd probably just write this as a C-style for loop if iterating over > two lists: > > for (int i = 0; i < normalCount; i++) { > InferenceContext.annotateNode(positional[i], normalParameterTypes[i]); > } > > you wouldn't need "required" either. > > Same thing below. > > (/me misses Enumerable.zip) The for loop version above will throw an out of bounds exception if the code is ill-typed and normalCount is larger than the number of positional arguments. I could take the min of the lengths...not sure if that would be better...I'll look at it. This code went through a few evolutions so I might be missing a simpler formulation, but I was trying to be very careful about overruns in ill-typed code. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11300: FieldElement fieldElement = enclosingClass.getField(node.fieldName.name); On 2015/11/23 22:11:57, John Messerly wrote: > Does `fieldName.staticElement` give you the field element? That would save a > look up. Not set until the elementResolver runs below. I could try re-ordering the call to the elementResolver, but in general I'm trying to avoid doing that whenever possible since often the order matters. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11991: super.visitYieldStatement(node); On 2015/11/21 16:12:22, Brian Wilkerson wrote: > Missing return Just a meta-comment: this is one of those analyzer hints that I'd love to make use of, but I can't actually make use of analyzer hints when working on the analyzer because of the deluge of hints about deprecated APIs. Is there any way to suppress hints, or separate them by kind in the IDE, or somehow to address this? Seems like this would be a nice win for users who want to use things like @deprecated. As it is though, I always end up running the analyzer with --no-hints, which is a shame. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:118: (p.type == null || p.type.isDynamic) && On 2015/11/23 22:11:58, John Messerly wrote: > aside: this might be worth a TODO. Ideally we can handle implicit types > consistently, rather than having to handle "null" and DynamicTypeImpl Not sure what the TODO would be to do? The extra checks are to allow for the possibility that we had an implicit type, but have already inferred something for it (unless we inferred dynamic, in which case we can try to improve it). This may not ever happen if I get rid of the calls to this from MethodDeclaration and ConstructorDeclaration though - if so I could turn this into an assertion. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... File pkg/analyzer/test/utils.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:37: class AstFinder { On 2015/11/21 16:12:22, Brian Wilkerson wrote: > I'm not a fan of top-level functions; I'd actually prefer leaving it the way you > have it. So I guess it's your call. Hmm, I was really annoyed that moving these from local instance methods to static methods in a separate class made all the lines really long, so I kind of like Konstantin's suggestion. Brian, how would you feel about them being in a library that gets imported with a shorter prefix (e.g. finder.getClass, etc)?
https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5789: InferenceContext( On 2015/11/24 19:32:11, Leaf wrote: > On 2015/11/23 22:11:58, John Messerly wrote: > > BTW, if either the type or constructor can be made private, that'd be ideal. > > The type has to be public since it's accessed by the static type analyzer > (though I guess I could instead provide accessors on the resolver itself that > forward to the context). > > What's the convention for private constructors in Dart? InferenceContext._(...) https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10958: int index = 0; On 2015/11/24 19:32:12, Leaf wrote: > On 2015/11/23 22:11:57, John Messerly wrote: > > trivial: I'd probably just write this as a C-style for loop if iterating over > > two lists: > > > > for (int i = 0; i < normalCount; i++) { > > InferenceContext.annotateNode(positional[i], normalParameterTypes[i]); > > } > > > > you wouldn't need "required" either. > > > > Same thing below. > > > > (/me misses Enumerable.zip) > > The for loop version above will throw an out of bounds exception if the code is > ill-typed and normalCount is larger than the number of positional arguments. I > could take the min of the lengths...not sure if that would be better...I'll look > at it. This code went through a few evolutions so I might be missing a simpler > formulation, but I was trying to be very careful about overruns in ill-typed > code. Ah, in that case, probably best to leave it as is! https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11300: FieldElement fieldElement = enclosingClass.getField(node.fieldName.name); On 2015/11/24 19:32:12, Leaf wrote: > On 2015/11/23 22:11:57, John Messerly wrote: > > Does `fieldName.staticElement` give you the field element? That would save a > > look up. > > Not set until the elementResolver runs below. I could try re-ordering the call > to the elementResolver, but in general I'm trying to avoid doing that whenever > possible since often the order matters. ah, interesting. In this case I think re-ordering would probably be okay, since this line essentially inlines visitConstructorFieldInitializer (all it does is look up the field element & save it). https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:118: (p.type == null || p.type.isDynamic) && On 2015/11/24 19:32:12, Leaf wrote: > On 2015/11/23 22:11:58, John Messerly wrote: > > aside: this might be worth a TODO. Ideally we can handle implicit types > > consistently, rather than having to handle "null" and DynamicTypeImpl > > Not sure what the TODO would be to do? The extra checks are to allow for the > possibility that we had an implicit type, but have already inferred something > for it (unless we inferred dynamic, in which case we can try to improve it). > This may not ever happen if I get rid of the calls to this from > MethodDeclaration and ConstructorDeclaration though - if so I could turn this > into an assertion. Ah, makes sense. Maybe worth a comment. I was confused because it looked redundant to have `p.hasImplicitType && (p.type == null || p.type.isDynamic)`. I guess what it's really asking is "did we have an implicit type? And did we fail so far to improve it?"
https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5924: node?.setProperty(_propertyName, type); I don't have a problem with either approach. The only consideration I can think of is the typical one for side-car data structures: you typically need to pass the side-car along with the original data structure, which can lead to longer arguments lists. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11991: super.visitYieldStatement(node); > Is there any way to suppress hints, or separate them by kind in the IDE, or somehow to address this? That depends on the IDE. I don't know of any way to do that in IntelliJ, but that might be planned work. I know there has also been some discussion about being able to disable hints in the .analysis_options file, but I don't think that work is completed. > ... the deluge of hints about deprecated APIs Those should go away soon as part of our breaking change bundle. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... File pkg/analyzer/test/utils.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:37: class AstFinder { > Hmm, I was really annoyed that moving these from local instance methods to > static methods in a separate class made all the lines really long, so I kind of > like Konstantin's suggestion. Brian, how would you feel about them being in a > library that gets imported with a shorter prefix (e.g. finder.getClass, etc)? It isn't my favorite solution, but I'm not going to object too loudly :-) It seems like doing so is essentially the same as making them static methods on an invisible class, except that every importer gets to choose the name of the containing class. It works OK if implementors choose a consistent and meaningful name, but if I see utils.getClass(...) in one file and finder.getClass(...) in another, it isn't obvious to me that it's the same function. But I'd be surprised to find code like import '.../utils.dart' as AstFinder; Of course, none of this would be a problem if we had a sensible (100 character) line limit :-)
PTAL, thanks! https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5759: /** Maintains and manages contextual type information used for On 2015/11/23 22:11:57, John Messerly wrote: > On 2015/11/21 16:12:22, Brian Wilkerson wrote: > > Not asking for a change, but I'm not a fan of the abbreviated block style. > > Yeah same here. I've been trying to follow: > > /** > * The doc comment. > */ > > ... for consistency. > > At some point, I guess we should run the auto conversion tool to get /// ? > That would match the Dart style guide: > https://www.dartlang.org/effective-dart/documentation/#do-use--doc-comments-t... Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5762: class InferenceContext { On 2015/11/23 22:11:57, John Messerly wrote: > "Context" as a class suffix often just means "here's a bag of global variables" > ... it's not all that descriptive. I seem to recall Vijay at some point steering > me away from a "context" class in DDC. > > That said, it's definitely hard to name these things. I feel like every compiler > eventually ends up with some "Context" types :). And here, we are using > "contextual type information" so it's more sensible name than usual. But a lot > of the state is the same bag of stuff we track frequently (Type > provider/system/rules & error listener...) > > One possible solution is to make this object take a bit more of an active role, > then it can be named after what it does. But that'd require moving methods to > it. I haven't gotten far enough in the CL yet to know if that would make sense. > > Anyway this is probably an FYI comment for now, but something to noodle on. Acknowledged. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5765: static const String _propertyName = On 2015/11/23 22:11:57, John Messerly wrote: > might be worth giving this a more descriptive name, like "_contextTypeProperty". Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5789: InferenceContext( On 2015/11/24 20:10:57, John Messerly wrote: > On 2015/11/24 19:32:11, Leaf wrote: > > On 2015/11/23 22:11:58, John Messerly wrote: > > > BTW, if either the type or constructor can be made private, that'd be ideal. > > > > The type has to be public since it's accessed by the static type analyzer > > (though I guess I could instead provide accessors on the resolver itself that > > forward to the context). > > > > What's the convention for private constructors in Dart? > > InferenceContext._(...) Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5791: : this._typeProvider = typeProvider, On 2015/11/23 22:11:57, John Messerly wrote: > nit: "this." is not necessary here Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5794: /** Get the return type of the current enclusing function, if any. On 2015/11/21 16:12:22, Brian Wilkerson wrote: > "enclusing" --> "enclosing" Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5799: /** If t1 = I<dynamic, ..., dynamic>, then look for a supertype On 2015/11/23 22:11:57, John Messerly wrote: > BTW: you probably want to put backticks around generic types, otherwise dartdoc > might not render them well. I know by default many markdowns won't do the right > thing. In other words: > > If `t1 = I<dynamic, ..., dynamic>`, then look for a supertype ... > > > BTW, my CL might have the same problem, I'll double check... Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5800: * of t1 of the form K<S0, ..., Sm> where t2 = K<S0', ..., Sm'> On 2015/11/23 22:11:57, John Messerly wrote: > On 2015/11/21 16:12:22, Brian Wilkerson wrote: > > What is 't2'? > > looks like it's the second argument to matchTypes Added a line to the comments to make this clearer. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5811: if (_returnStack.isNotEmpty) { On 2015/11/21 16:12:22, Brian Wilkerson wrote: > Add "assert(_returnStack.isNotEmpty);" before the if for safety during testing. Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5827: if (info == null) return; On 2015/11/24 19:32:12, Leaf wrote: > On 2015/11/21 16:12:22, Brian Wilkerson wrote: > > Stylistic nit (for here and elsewhere): we always use blocks for control > > structures and always include type annotations. > > Sorry - pulled over from DDC code and I missed the style conversion. Will fix. > :) Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5881: bool match(InterfaceType t1) { On 2015/11/21 16:12:22, Brian Wilkerson wrote: > This method will result in an infinite loop if the type hierarchy is not well > formed. Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5923: static void annotateNode(AstNode node, DartType type) { On 2015/11/23 22:11:58, John Messerly wrote: > (note: the following may be obsolete in light of my comment below) > > maybe this should be called "setContextType" or "setType" if we keep "Context" > in the name? That way it would read like: > > InferenceContext.setType(node, type); > > I would also change "get" to something that corresponds. Either "getAnnotation" > or "getType" or "typeOf" ... something along those lines... Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5924: node?.setProperty(_propertyName, type); On 2015/11/24 21:17:57, Brian Wilkerson wrote: > I don't have a problem with either approach. > > The only consideration I can think of is the typical one for side-car data > structures: you typically need to pass the side-car along with the original data > structure, which can lead to longer arguments lists. I'm leaving it as it is for now, but added a todo to reconsider. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5925: return; On 2015/11/23 22:11:57, John Messerly wrote: > nit: return not needed here The visitor methods seem to always explicitly return null below - is the principle that implicit returns are ok for void functions but not for Object ones (even if the return value is never used)? https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5935: // If t1 = I<dynamic, ..., dynamic>, then look for a supertype On 2015/11/23 22:11:57, John Messerly wrote: > Is this supposed to be here? Consider folding it into the doc comment? > (sometimes "sort members" will lose positions of non-doc comments, so I wanted > to ask) Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10598: InferenceContext inferenceContext = null; On 2015/11/23 22:11:58, John Messerly wrote: > hmmm, I guess this is for consistency, but `= null` is not required > https://www.dartlang.org/effective-dart/usage/#dont-explicitly-initialize-var... Acknowledged. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10960: InferenceContext.annotateNode(argument, normalParameterTypes[index++]); On 2015/11/23 22:11:58, John Messerly wrote: > you might be able to avoid some of the above logic using > argument.correspondingParameterElement. > > (aside: correspondingParameterIndex might be more useful. I ran into this as > well and ended up doing .indexOf(...) to find the FunctionType's parameter) Maybe? I'd have to think through the various implications and ordering - not sure all the right resolution and inference has happened at this point to set up the parameter elements correctly. Added a TODO. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10970: namedParameterTypes.containsKey(argument.name.label.name)) { On 2015/11/23 22:11:57, John Messerly wrote: > not that it matters, but I'd be tempted to rewrite this to only do the map > lookup once: > > if (argument is NamedExpression) { > DartType type = namedParameterTypes[argument.name.label.name]; > if (type != null) { ... } > } Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:10976: super.visitArgumentList(node); On 2015/11/21 16:12:22, Brian Wilkerson wrote: > Missing return Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11022: return null; On 2015/11/21 16:12:22, Brian Wilkerson wrote: > Or "return super.visitAwaitExpression(node);" Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11065: // TODO(leafp): Handle this case On 2015/11/21 16:12:22, Brian Wilkerson wrote: > I won't remember what this means outside the context of this CL; more detail > would be good. Perhaps: Use InferenceContext to propagate types downward. Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11238: InferenceContext.annotateNode( On 2015/11/23 22:11:57, John Messerly wrote: > might be worth a helper for this? > > InferenceContext.setTypeFromNode(thenExpression, node); Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11280: if (type is FunctionType) { On 2015/11/23 22:11:58, John Messerly wrote: > I think this would always be the case for a constructor Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11281: _inferFormalParameterList(node.parameters, type); On 2015/11/23 22:11:57, John Messerly wrote: > was just chatting about this--I'm not sure this could ever kick in for > ConstructorDecl or MethodDecls, since the FunctionType is derived from the > formal parameter types. I *think* they should exactly match. If not I would be > curious when it kicks in. > > (it would work for local functions though) Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11300: FieldElement fieldElement = enclosingClass.getField(node.fieldName.name); On 2015/11/24 20:10:57, John Messerly wrote: > On 2015/11/24 19:32:12, Leaf wrote: > > On 2015/11/23 22:11:57, John Messerly wrote: > > > Does `fieldName.staticElement` give you the field element? That would save a > > > look up. > > > > Not set until the elementResolver runs below. I could try re-ordering the call > > to the elementResolver, but in general I'm trying to avoid doing that whenever > > possible since often the order matters. > > ah, interesting. In this case I think re-ordering would probably be okay, since > this line essentially inlines visitConstructorFieldInitializer (all it does is > look up the field element & save it). Yes, it's fine here and now, but I feel like every non-standard use of these visitors is a tax on future maintenance. Mostly the element resolver can assume that sub-expressions have been resolved - this wouldn't be the case here if I move it. It happens not to matter now.... dunno. I'm inclined to leave it as is despite the redundancy, but could be convinced otherwise. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11777: if (node.methodName.staticElement is ExecutableElement) { On 2015/11/23 22:11:57, John Messerly wrote: > suggestion, if you save this into a variable you can get type propagation: > > Element method = node.methodName.staticElement; > if (method is ExecutableElement) { > InferenceContext.annotateNode(node.argumentList, method.type); > } Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11789: super.visitNamedExpression(node); On 2015/11/23 22:11:57, John Messerly wrote: > not sure if Brian got tired of pointing it out ... but, there's a few more > missing returns here & below. Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11980: InterfaceType wrapperD = (_enclosingFunction.isSynchronous) On 2015/11/23 22:11:57, John Messerly wrote: > nit: parens not needed Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:11991: super.visitYieldStatement(node); On 2015/11/24 21:17:57, Brian Wilkerson wrote: > > Is there any way to suppress hints, or separate them by kind in the IDE, or > somehow to address this? > > That depends on the IDE. I don't know of any way to do that in IntelliJ, but > that might be planned work. I know there has also been some discussion about > being able to disable hints in the .analysis_options file, but I don't think > that work is completed. > > > ... the deluge of hints about deprecated APIs > > Those should go away soon as part of our breaking change bundle. Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:118: (p.type == null || p.type.isDynamic) && On 2015/11/24 20:10:58, John Messerly wrote: > On 2015/11/24 19:32:12, Leaf wrote: > > On 2015/11/23 22:11:58, John Messerly wrote: > > > aside: this might be worth a TODO. Ideally we can handle implicit types > > > consistently, rather than having to handle "null" and DynamicTypeImpl > > > > Not sure what the TODO would be to do? The extra checks are to allow for the > > possibility that we had an implicit type, but have already inferred something > > for it (unless we inferred dynamic, in which case we can try to improve it). > > This may not ever happen if I get rid of the calls to this from > > MethodDeclaration and ConstructorDeclaration though - if so I could turn this > > into an assertion. > > Ah, makes sense. Maybe worth a comment. I was confused because it looked > redundant to have `p.hasImplicitType && (p.type == null || p.type.isDynamic)`. > I guess what it's really asking is "did we have an implicit type? And did we > fail so far to improve it?" Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:460: // TODO(leafp) handle async et al On 2015/11/23 22:11:58, John Messerly wrote: > for now should we skip if isGenerator or isAsync? Or is that implied by the > below checks Actually, nothing to do here for those cases, removed the TODO. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/gener... File pkg/analyzer/test/generated/resolver_test.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/gener... pkg/analyzer/test/generated/resolver_test.dart:12619: A<int, String> a0 = new A(3, "hello"); On 2015/11/23 22:11:58, John Messerly wrote: > formatting is a little off in this string literal. not a big deal tho Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/gener... pkg/analyzer/test/generated/resolver_test.dart:12666: void test8() { // Currently we only allow variable constraints. Test that we reject. On 2015/11/23 22:11:58, John Messerly wrote: > long line here Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... File pkg/analyzer/test/utils.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:27: /** The type of a function which given an [S0] and an S1, builds an assertion over [T]s. On 2015/11/23 22:11:58, John Messerly wrote: > long line (& a few below) > > unfortunately dartfmt won't fix comments :( Done. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:37: class AstFinder { On 2015/11/24 21:17:57, Brian Wilkerson wrote: > > Hmm, I was really annoyed that moving these from local instance methods to > > static methods in a separate class made all the lines really long, so I kind > of > > like Konstantin's suggestion. Brian, how would you feel about them being in a > > library that gets imported with a shorter prefix (e.g. finder.getClass, etc)? > > It isn't my favorite solution, but I'm not going to object too loudly :-) > > It seems like doing so is essentially the same as making them static methods on > an invisible class, except that every importer gets to choose the name of the > containing class. It works OK if implementors choose a consistent and meaningful > name, but if I see > > utils.getClass(...) > > in one file and > > finder.getClass(...) > > in another, it isn't obvious to me that it's the same function. But I'd be > surprised to find code like > > import '.../utils.dart' as AstFinder; > > Of course, none of this would be a problem if we had a sensible (100 character) > line limit :-) Ok, leaving as is for now. https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:180: class TypeAssertions { On 2015/11/23 22:11:58, John Messerly wrote: > note: it'd be slightly nicer to make these be Matchers, which have better > self-documenting error messages > https://www.dartdocs.org/documentation/matcher/0.12.0%2B1/matcher/Matcher-cla... Added TODO. Nice to know https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/test/utils... pkg/analyzer/test/utils.dart:211: /** Given assertions for the argument and return types, produce an On 2015/11/21 16:12:22, Brian Wilkerson wrote: > Format the file? Filed https://github.com/dart-lang/dart_style/issues/472 . :)
LGTM https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5759: /** Maintains and manages contextual type information used for > At some point, I guess we should run the auto conversion tool to get /// ? Personally, I find the "///" style hard to read, so I'd prefer that we didn't. (I'm not the only one on the team.) https://codereview.chromium.org/1462133005/diff/60001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1462133005/diff/60001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:469: computedType = functionType.returnType; "functionType.returnType" --> "returnType"? https://codereview.chromium.org/1462133005/diff/60001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong/checker.dart (right): https://codereview.chromium.org/1462133005/diff/60001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/checker.dart:470: } else if (node.staticType != null && node.staticType is InterfaceType) { You can remove "node.staticType != null && " (it's implied by the is check). https://codereview.chromium.org/1462133005/diff/60001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/checker.dart:490: } else if (node.staticType != null && node.staticType is InterfaceType) { ditto
other than Brian's comments, LGTM++
rnystrom@google.com changed reviewers: + rnystrom@google.com
https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5759: /** Maintains and manages contextual type information used for On 2015/12/01 22:31:09, Brian Wilkerson wrote: > > At some point, I guess we should run the auto conversion tool to get /// ? > > Personally, I find the "///" style hard to read, so I'd prefer that we didn't. > (I'm not the only one on the team.) I'll point out two things: 1. The style guide requires "///" and has for quite some time. (https://www.dartlang.org/effective-dart/documentation/#do-use--doc-comments-t...) 2. The style here is not even valid JavaDoc style according to Google's Java style guide. It should be one line or three+, but never two. But, of course, your code is your code.
thanks! https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1462133005/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:5759: /** Maintains and manages contextual type information used for On 2015/12/01 23:33:27, Bob Nystrom wrote: > On 2015/12/01 22:31:09, Brian Wilkerson wrote: > > > At some point, I guess we should run the auto conversion tool to get /// ? > > > > Personally, I find the "///" style hard to read, so I'd prefer that we didn't. > > (I'm not the only one on the team.) > > I'll point out two things: > > 1. The style guide requires "///" and has for quite some time. > (https://www.dartlang.org/effective-dart/documentation/#do-use--doc-comments-t...) > > 2. The style here is not even valid JavaDoc style according to Google's Java > style guide. It should be one line or three+, but never two. > > But, of course, your code is your code. Per point 2: that's my fault (caught in the review), and is resolved in a later patch set. I almost made a feature request to dart_style asking for the formatter to fix these for me... :) https://codereview.chromium.org/1462133005/diff/60001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1462133005/diff/60001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:469: computedType = functionType.returnType; On 2015/12/01 22:31:09, Brian Wilkerson wrote: > "functionType.returnType" --> "returnType"? Done. https://codereview.chromium.org/1462133005/diff/60001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong/checker.dart (right): https://codereview.chromium.org/1462133005/diff/60001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/checker.dart:470: } else if (node.staticType != null && node.staticType is InterfaceType) { On 2015/12/01 22:31:09, Brian Wilkerson wrote: > You can remove "node.staticType != null && " (it's implied by the is check). Done. https://codereview.chromium.org/1462133005/diff/60001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/checker.dart:490: } else if (node.staticType != null && node.staticType is InterfaceType) { On 2015/12/01 22:31:09, Brian Wilkerson wrote: > ditto Done.
Description was changed from ========== Downwards inference. This adds support to the resolver for downwards type inference in strong mode. Downwards inference is removed from the DDC checker in favor of the strong mode analyzer version. BUG= ========== to ========== Downwards inference. This adds support to the resolver for downwards type inference in strong mode. Downwards inference is removed from the DDC checker in favor of the strong mode analyzer version. BUG= R=brianwilkerson@google.com, jmesserly@google.com Committed: https://github.com/dart-lang/sdk/commit/64ab186b167d402e90db4e9740c4a408d42f42ca ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 64ab186b167d402e90db4e9740c4a408d42f42ca (presubmit successful). |
