Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1352763004:
Add additional strong mode inference. (Closed)
|
Created:
5 years, 3 months ago by Leaf Modified:
5 years, 3 months ago CC:
reviews_dartlang.org, vsm, Jennifer Messerly Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd additional strong mode inference. This adds in all the remaining inference cases from the strong mode RestrictedStaticTypeAnalyzer.
BUG=
R=brianwilkerson@google.com
Committed: https://github.com/dart-lang/sdk/commit/4fe661995d664aa388400d92ee81703f99d13a68
Patch Set 1 #Patch Set 2 : Add tests #
Total comments: 9
Patch Set 3 : Address comments 1 #Patch Set 4 : Address comments 2 #
Total comments: 2
Patch Set 5 : Fix inline JS #
Total comments: 4
Messages
Total messages: 19 (4 generated)
leafp@google.com changed reviewers: + brianwilkerson@google.com, paulberry@google.com
This has all of the remaining inference except the downwards inference. Some of this is probably temporary (e.g. the whitelisted ad-hoc generic methods) and some of this should perhaps be integrated better (e.g. the JSInline code), but I'd like to try to land this to unblock Vijay if it's in good enough shape for now.
jmesserly@google.com changed reviewers: + jmesserly@google.com
Nice! just one comment I noticed while skimming this https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:15136: // corresponding analyzer bug is fixed: I wonder if it's worth moving this fix into the analyzer generally, instead of putting it here? That would fix the bug below. Kill two birds with one stone :) https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1724: // TODO(vsm): If the inferred type is not a subtype, should we use a GLB instead? long line
https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:15133: // TODO(vsm): The static type of a conditional should be the LUB of the Are you sure this is necessary? I thought I fixed it in https://codereview.chromium.org/1179343004/, and there are tests validating the fix in pkg/analyzer/test/generated/type_system_test.dart. (The only reason I didn't close bug 22854 is because I didn't know about it; I discovered the bug independently).
https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:13444: Map<String, DartType> get objectMemberTypes; I'm not thrilled with this getter for three reasons. First, it feels wrong for a type provider to provide information about the members of a class, even though the information is a type related to that member. Second, the map mixes two kinds of mappings: from method names to the method's function type and from getter names to the return type of the getter. Third, there's already a way to accomplish the same thing. How bad would it be to replace code like: typeProvider.objectMemberTypes[memberName] with code like: typeProvider.objectType.element.getMethod(memberName) or typeProvider.objectType.element.getGetter(memberName) As far as I can see, we always know whether we're looking for a method or a getter; in fact, we have to in order to know how to interpret the returned value.
https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:15133: // TODO(vsm): The static type of a conditional should be the LUB of the On 2015/09/22 18:23:13, Paul Berry wrote: > Are you sure this is necessary? I thought I fixed it in > https://codereview.chromium.org/1179343004/, and there are tests validating the > fix in pkg/analyzer/test/generated/type_system_test.dart. > > (The only reason I didn't close bug 22854 is because I didn't know about it; I > discovered the bug independently). Right you are, thanks! Code removed, tests still pass. https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1724: // TODO(vsm): If the inferred type is not a subtype, should we use a GLB instead? On 2015/09/22 18:17:21, John Messerly wrote: > long line Done.
https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:13444: Map<String, DartType> get objectMemberTypes; On 2015/09/22 18:36:53, Brian Wilkerson wrote: > I'm not thrilled with this getter for three reasons. First, it feels wrong for a > type provider to provide information about the members of a class, even though > the information is a type related to that member. Second, the map mixes two > kinds of mappings: from method names to the method's function type and from > getter names to the return type of the getter. Third, there's already a way to > accomplish the same thing. > > How bad would it be to replace code like: > > typeProvider.objectMemberTypes[memberName] > > with code like: > > typeProvider.objectType.element.getMethod(memberName) > > or > > typeProvider.objectType.element.getGetter(memberName) > > As far as I can see, we always know whether we're looking for a method or a > getter; in fact, we have to in order to know how to interpret the returned > value. +1 ... I recall trying this refactoring myself at one point, though must've not finished.
https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:13444: Map<String, DartType> get objectMemberTypes; On 2015/09/22 18:45:52, John Messerly wrote: > On 2015/09/22 18:36:53, Brian Wilkerson wrote: > > I'm not thrilled with this getter for three reasons. First, it feels wrong for > a > > type provider to provide information about the members of a class, even though > > the information is a type related to that member. Second, the map mixes two > > kinds of mappings: from method names to the method's function type and from > > getter names to the return type of the getter. Third, there's already a way to > > accomplish the same thing. > > > > How bad would it be to replace code like: > > > > typeProvider.objectMemberTypes[memberName] > > > > with code like: > > > > typeProvider.objectType.element.getMethod(memberName) > > > > or > > > > typeProvider.objectType.element.getGetter(memberName) > > > > As far as I can see, we always know whether we're looking for a method or a > > getter; in fact, we have to in order to know how to interpret the returned > > value. > > +1 ... I recall trying this refactoring myself at one point, though must've not > finished. I can take a look at changing this. I don't think that code alone works because I think it allows for static members and properties, which we don't want, yes? So the result is going to look more like PropertyAccessorElement element = typeProvider.objectType.element.getGetter(name); DartType t = (element.isStatic) ? element.type : null; Not too bad, but a bit repetitive. (There are no static members on the object class that I know of, but some could be added. I think we'd also have to worry about library private members if we didn't check for static, since the implementation class could have library private static members, and we could be in the same library).
https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/1352763004/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:13444: Map<String, DartType> get objectMemberTypes; Another option would be to add a 'getInstanceGetter' to ClassElement, or extend the method to take an optional set of flags: getGetter(String name, {bool canBeStatic: true, bool canBeInstance: true})
Ok, objectMemberTypes eliminated, PTAL.
vsm@google.com changed reviewers: + vsm@google.com
https://codereview.chromium.org/1352763004/diff/60001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1352763004/diff/60001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1746: _recordPropagatedType(node, returnType); I think this needs to be _recordStaticType also - not picking this up.
https://codereview.chromium.org/1352763004/diff/60001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1352763004/diff/60001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1746: _recordPropagatedType(node, returnType); On 2015/09/22 20:29:02, vsm wrote: > I think this needs to be _recordStaticType also - not picking this up. Good catch, thanks. Fixed now.
LGTM
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 4fe661995d664aa388400d92ee81703f99d13a68 (presubmit successful).
Message was sent while issue was closed.
scheglov@google.com changed reviewers: + scheglov@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1352763004/diff/80001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1352763004/diff/80001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1875: * an ad-hoc list of pseudo-generic methids. "methods" https://codereview.chromium.org/1352763004/diff/80001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1901: tx == _typeProvider.doubleType) { Maybe "tx == ty && (tx == _typeProvider.intType || tx == _typeProvider.doubleType)" instead? There is a test "max(1, 2.0) => num". But it seems that "max(1.0, 2)" will give "double".
Message was sent while issue was closed.
https://codereview.chromium.org/1352763004/diff/80001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1352763004/diff/80001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1875: * an ad-hoc list of pseudo-generic methids. On 2015/09/22 21:21:32, scheglov wrote: > "methods" Done. https://codereview.chromium.org/1352763004/diff/80001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1901: tx == _typeProvider.doubleType) { On 2015/09/22 21:21:32, scheglov wrote: > Maybe "tx == ty && (tx == _typeProvider.intType || tx == > _typeProvider.doubleType)" instead? > > There is a test "max(1, 2.0) => num". > But it seems that "max(1.0, 2)" will give "double". Great catch, thanks! Will fix in followup. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
