Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1306313002:
Implement instance member inference (Closed)
|
Created:
5 years, 4 months ago by Brian Wilkerson Modified:
5 years, 4 months ago CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionImplements instance member inference. Records the correct return type for synthetic function elements representing initializers.
R=leafp@google.com, paulberry@google.com
Committed: https://github.com/dart-lang/sdk/commit/0061447293bc601fce9b98920830706e3cb81ee0
Patch Set 1 #
Total comments: 24
Patch Set 2 : Address comments #
Total comments: 13
Patch Set 3 : Address more comments and fix bug #
Total comments: 9
Messages
Total messages: 15 (1 generated)
brianwilkerson@google.com changed reviewers: + leafp@google.com, paulberry@google.com
This might change the semantics slightly from the current implementation, so please look over the test cases carefully. Also, I couldn't find any tests for the class LibraryResolverWithInference. If there are some, please let me know and I'll get them ported over.
On 2015/08/21 20:58:49, Brian Wilkerson wrote: > This might change the semantics slightly from the current implementation, so > please look over the test cases carefully. Also, I couldn't find any tests for > the class LibraryResolverWithInference. If there are some, please let me know > and I'll get them ported over. Siggi's tests are here: https://github.com/dart-lang/dev_compiler/blob/master/test/checker/inferred_t... Search for "Override".
This is looking really nice, I'm pleasantly surprised with how cleanly it seems to be fitting in. Some comments and questions below. As Vijay mentioned, there are a bunch of tests that should be pulled over. I'm planning on pulling a bunch over anyway for the checker, so maybe I should just plan on doing a CL with all of the DDC tests (most of them initially set to fail)? https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:110: * element of the list, the current class will be the last. Are the references to ordering and list here out of date, or do Dart HashSets double as lists? https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:160: * `null` if the executable element is not a setter or does not have a single This doesn't actually check that the executable element is a setter does it? https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:176: FunctionType functionType = element.type; When does this happen (we have no returnType, but do have a type which is a FunctionType)? The comment on ExecutableElement.returnType suggests that returnType should only be null if the element model is not fully populated, is that what's going on here? https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:234: if (classElement.hasBeenInferred) { Can this happen? We know it was false on entry to this function and we've put the classElement in the elementsBeingInferred set, so if any of the _inferType calls reached this classElement again they should have thrown _CycleException before reaching the code which sets hasBeenInferred. Is there some other path that can set this? https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:278: if (newType.isDynamic && fieldElement.initializer != null) { I think we only want to do this if either this is not an override, or else if it is final. Consider: class A { dynamic x; } class B extends A { var x = 3; } Inferring x as having type int will cause an error later on, since the setter will not be a subtype of the setter from the supertype. If x is final, it's ok, and if x wasn't present in any of the superclasses it's ok. https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:279: newType = fieldElement.initializer.returnType; The old code postponed doing this initializer based inference until after all of the override based inference was completed. I can't really see where this would matter - the fields can't refer to each other right? Are there any places this could matter? https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:280: } Siggi's code avoids inferring from the initializer when the type is bottom, which I think is probably a good idea. Is it an invariant that returnType won't be bottom here? https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:325: if (parameter != null && !type.isAssignableTo(parameter.type)) { Once we've added a type system, we need to make this be DDC assignability. Maybe add a TODO? Also, I think this check is backwards (doesn't matter in regular dart since it's a symmetric relation there, I think). In DDC, we need the new setter type (type -> void) to be a subtype of the superclass setter type (parameter.type -> void), so we want parameter.type <: type. https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:379: FunctionType functionType = element.type; The code sequence from here down is duplicated above, and is fairly non-obvious (to me) why it needs to be done. Maybe factor out to a helper with a comment (or just comment, even just a pointer to where to look for why this needs doing)? https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/test/src/task/... File pkg/analyzer/test/src/task/strong_mode_test.dart (right): https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/test/src/task/... pkg/analyzer/test/src/task/strong_mode_test.dart:159: (fieldC as FieldElementImpl).hasImplicitType = true; Why does this need to be set manually? https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/test/src/task/... pkg/analyzer/test/src/task/strong_mode_test.dart:425: '''); Might be worth having a test or two that validates that field/method inference gets done before method body inference? That is, if class C has an extra method like: m2() => m() validate that the type of the invocation m() is int and not dynamic?
PTAL > As Vijay mentioned, there are a bunch of tests that should be > pulled over. I'm planning on pulling a bunch over anyway for > the checker, so maybe I should just plan on doing a CL with > all of the DDC tests (most of them initially set to fail)? I think that would be great. Just note that those tests are integration level tests and can't actually be run until we have the whole think working end-to-end. This CL is just a small part of the work. https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:110: * element of the list, the current class will be the last. It was out of date, and has been removed. I was going to try to minimize the loss of information by keeping the order in which we visited classes and only failing to infer in the actual cycle, but decided that we could wait until later to decide whether that was worth doing. https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:160: * `null` if the executable element is not a setter or does not have a single > This doesn't actually check that the executable element is a setter does it? No, but it does now. https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:176: FunctionType functionType = element.type; > When does this happen (we have no returnType, but do have a type which is a > FunctionType)? In the absence of bugs, it doesn't. I was just being defensive (perhaps overly so). https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:234: if (classElement.hasBeenInferred) { > Can this happen? No, this was left over from when I was going to search for cycles. Good catch! https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:278: if (newType.isDynamic && fieldElement.initializer != null) { Done https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:279: newType = fieldElement.initializer.returnType; > The old code postponed doing this initializer based inference until after all of > the override based inference was completed. I can't really see where this would > matter - the fields can't refer to each other right? Are there any places this > could matter? It's possible for the code to be invalid (and therefore have references to other instance fields). Consider class C { var f0 = 0; var f1 = f1 + 1; } I don't think that basing the inferred type of f1 on the non-inferred type of f0 will be a problem. I can't think of anything other than invalid code that could be impacted. https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:280: } > Siggi's code avoids inferring from the initializer when the type is bottom, > which I think is probably a good idea. Is it an invariant that returnType won't > be bottom here? No. I just saw that case while looking through the tests. I'll fix the code and add a unit test. https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:325: if (parameter != null && !type.isAssignableTo(parameter.type)) { Done https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:379: FunctionType functionType = element.type; Comments added. (I can't easily abstract the code to a separate method because ExecutableElementImpl and PropertyAccessorElementImpl don't have a common supertype that defines the behavior I need.) https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/test/src/task/... File pkg/analyzer/test/src/task/strong_mode_test.dart (right): https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/test/src/task/... pkg/analyzer/test/src/task/strong_mode_test.dart:159: (fieldC as FieldElementImpl).hasImplicitType = true; > Why does this need to be set manually? I don't remember why I originally needed to add those lines, but they aren't required so I've removed them. https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/test/src/task/... pkg/analyzer/test/src/task/strong_mode_test.dart:425: '''); > Might be worth having a test or two that validates that field/method inference > gets done before method body inference? That would be good, but these tests are only testing the portion that infers the overridden type. Resolving method bodies is a later phase that won't have been performed yet. But we should definitely add tests like that in the code that tests that method bodies are being resolved correctly.
> I think that would be great. Just note that those tests are integration level > tests and can't actually be run until we have the whole think working > end-to-end. This CL is just a small part of the work. They're structured as integration tests, but I think their intent is a bit more unit test-ish. I'll maybe look at pulling some of them in as unit tests. https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:342: return elements.isNotEmpty; I can't work out why this matters, nor why it should be different for _onlyGetters vs _onlyMethods. What's happening here? The comment should probably be updated as well " ... is non-empty and contains only getters".
I have some significant concerns listed below (mostly the issues about overwriting the types and possible nondeterminism in the presence of circularities). But in the interest of keeping up forward progress so we can get the new task model switched on asap, lgtm. Feel free to defer the larger issues to TODO comments and/or issues in the issue tracker if you think it makes sense to do so. https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:176: FunctionType functionType = element.type; On 2015/08/24 16:52:57, Brian Wilkerson wrote: > > When does this happen (we have no returnType, but do have a type which is a > > FunctionType)? > > In the absence of bugs, it doesn't. I was just being defensive (perhaps overly > so). FWIW, I think I would argue in this case that this is probably overly defensive, since the implementation of FunctionType.returnType simply delegates (by way of FunctionTypeImpl.baseReturnType) to Element.returnType. https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:91: class InstanceMemberInferrer { I have a broad concern about this implementation strategy: previously, all of our analysis tasks have had the property that they only set fields in the element model that were previously null, and they express all their dependencies using the task model. That had the advantage that if a subtle dependency change causes us to re-execute some tasks that we've executed previously, we can be confident that the results from the previous execution won't pollute the results of the re-execution. The algorithm in this class doesn't have that property, since it decided which types to infer based on looking for types that are "dynamic", and then it *replaces* those types with the inferred types. This means that if type inference is re-run, it won't re-infer already-inferred types. I worry about a situation arising where, for example, class A extends class B, and a change to class B requires inference to be re-run on class A, but since class A has already had inference run on it once before, it will not re-infer properly. This probably isn't a problem today because changing class B will cause class A's element model to be re-built from scratch (wiping out any inference results), but one day we might try to make things more incremental, and that could cause subtle breakages. Similar problems might arise if the type of a declaration in class A is in turn inferred from some other elements. Consider adding a TODO comment to capture some of these issues. In the future we might want the element model to keep track of a "preliminary type" distinct from the "static type" in order to avoid these problems. https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:144: for (int i = 0; i < length; i++) { Why not "for (DartType type in overriddenMethods)"? https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:149: if (returnType != type) { If I understand correctly, this means that if there are multiple overridden methods declaring different return types, we will infer "dynamic"; is that correct? If so, consider adding an explanatory comment to this effect, since a casual reader might expect us to do some sort of "greatest lower bound" computation here. https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:223: throw new _CycleException(); I'm concerned that this will lead to nondeterministic results when a circularity exists, since the order in which we visit classes will determine which classes get evaluated before the _CycleException is thrown. Maybe we should consider using the task model to handle the circularities, since it already has the capability of resuming analysis in a deterministic fashion when a circularity is found. https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:342: return elements.isNotEmpty; On 2015/08/24 18:24:48, Leaf wrote: > I can't work out why this matters, nor why it should be different for > _onlyGetters vs _onlyMethods. What's happening here? > > The comment should probably be updated as well " ... is non-empty and contains > only getters". I'm confused by this too. If the non-empty behavior is crucial here, it would be nice to have a comment explaining why; also, I think it would be worth changing the name of the method, since I believe the default assumption when seeing a method called "_onlyGetters" would be to assume that the empty list vacuously satisfies this query. Alternatively, move the non-empty check to the call site so there won't be any confusion (e.g. line 199 changes to "if (overriddenGetters.isNotEmpty && _onlyGetters(overriddenGetters))").
PTAL In the process of addressing comments I discovered a bug that I fixed and added a test for. Specifically, we were inferring a return type for a getter even when it was inconsistent with its sibling or overridden setters. https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:176: FunctionType functionType = element.type; > FWIW, I think I would argue in this case that this is probably overly defensive ... Ok. I've removed it. https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:91: class InstanceMemberInferrer { I added you comments (reworded slightly) as a comment in the class. https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:144: for (int i = 0; i < length; i++) { > Why not "for (DartType type in overriddenMethods)"? Historic reasons that no longer apply. I'm happy to change it if you'd like. https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:149: if (returnType != type) { Done https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:223: throw new _CycleException(); > I'm concerned that this will lead to nondeterministic results when a circularity > exists, since the order in which we visit classes will determine which classes > get evaluated before the _CycleException is thrown. I can't think of a case where there is any nondeterminism, can you?. If we try to infer types for members of a class C and there is a circularity in the inheritance structure, then none of the members of C will be inferred. (In other words, we only infer members for a class if all of that classes supertypes can be inferred.) If we cannot infer members for a class C then we will not infer members for any subtype of C. If the members of a supertype of C are successfully inferred in the process of inferring members for C, then it must be the case that neither that class nor any of its superclasses are involved in any circularity, and hence would have had member types inferred in exactly the same way no matter when the inference were done. https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:342: return elements.isNotEmpty; > I can't work out why this matters, nor why it should be different for > _onlyGetters vs _onlyMethods. What's happening here? When _computeReturnType is passed an empty list it will return 'dynamic', which causes a problem in _inferField because then we cannot tell whether or not to infer based on the initializer. I thought it would be better to solve the problem by not passing in an empty list, and this was a convenient place to add that check. I've moved the check to the call sites in the hope that it's more readable. We don't have that issue with methods because if we cannot infer the return type from an overridden method we revert to the default of 'dynamic' anyway. > The comment should probably be updated as well " ... is non-empty and contains > only getters". Done.
lgtm https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:229: PropertyAccessorElement setter = (accessorElement.enclosingElement as ClassElement).getSetter(accessorElement.name); Long line. Did you forget to run the formatter? https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:237: } Is it a problem that we're doing this setter-related logic only when visiting the corresponding getter? Does that mean that if there's a setter without a corresponding getter (which is allowed in Dart) it won't get its type inferred? https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:368: * Return `true` if the list of [elements] is non-empty and contains only Revert the comment change. Since you accepted the suggestion below (to move the "isNonEmpty" check to the caller), "Return `true` if the list of [elements] contains only getters." is now a correct description.
lgtm modulo the comment fix, thanks! https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:91: class InstanceMemberInferrer { On 2015/08/24 21:20:57, Brian Wilkerson wrote: > I added you comments (reworded slightly) as a comment in the class. Just as a passing comment - we've discussed keeping track of what types were inferred (and why), which might also provide a handle on addressing this. https://codereview.chromium.org/1306313002/diff/20001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:342: return elements.isNotEmpty; On 2015/08/24 21:20:58, Brian Wilkerson wrote: > > I can't work out why this matters, nor why it should be different for > > _onlyGetters vs _onlyMethods. What's happening here? > > When _computeReturnType is passed an empty list it will return 'dynamic', which > causes a problem in _inferField because then we cannot tell whether or not to > infer based on the initializer. I thought it would be better to solve the > problem by not passing in an empty list, and this was a convenient place to add > that check. I've moved the check to the call sites in the hope that it's more > readable. > > We don't have that issue with methods because if we cannot infer the return type > from an overridden method we revert to the default of 'dynamic' anyway. > > > The comment should probably be updated as well " ... is non-empty and contains > > only getters". > > Done. Ah, missed that, makes sense. I think this way of writing it is clearer though, thanks. But now the comment is wrong again... :}
Thanks for your patience with multiple passed. At this point, I'll wait until morning to commit. https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:229: PropertyAccessorElement setter = (accessorElement.enclosingElement as ClassElement).getSetter(accessorElement.name); > Long line. Did you forget to run the formatter? Yes. Done. https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:237: } > Is it a problem that we're doing this setter-related logic only when visiting > the corresponding getter? Does that mean that if there's a setter without a > corresponding getter (which is allowed in Dart) it won't get its type inferred? We only infer return types, and setters don't have a return type, so I don't think it's a problem. https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:368: * Return `true` if the list of [elements] is non-empty and contains only > Revert the comment change. Done
https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:237: } On 2015/08/24 22:11:46, Brian Wilkerson wrote: > > Is it a problem that we're doing this setter-related logic only when visiting > > the corresponding getter? Does that mean that if there's a setter without a > > corresponding getter (which is allowed in Dart) it won't get its type > inferred? > > We only infer return types, and setters don't have a return type, so I don't > think it's a problem. I'm confused because the call to _setParameterType() appears to be setting the type of a parameter for a setter. Am I misunderstanding something?
https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:237: } > I'm confused because the call to _setParameterType() appears to be setting the > type of a parameter for a setter. Am I misunderstanding something? No, you're right. And it probably is a problem. The original code doesn't infer the parameter type for a setter unless it's an implied setter. Unless I hear otherwise, I'll remove that line.
https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1306313002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong_mode.dart:237: } On 2015/08/24 22:29:59, Brian Wilkerson wrote: > > I'm confused because the call to _setParameterType() appears to be setting the > > type of a parameter for a setter. Am I misunderstanding something? > > No, you're right. And it probably is a problem. The original code doesn't infer > the parameter type for a setter unless it's an implied setter. > > Unless I hear otherwise, I'll remove that line. sgtm
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 0061447293bc601fce9b98920830706e3cb81ee0 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
