Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1477953002:
Fix propagated argument element bug. This fixes #25040. (Closed)
|
Created:
5 years 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. |
DescriptionFix propagated argument element bug. This fixes #25040.
BUG=
R=brianwilkerson@google.com
Committed: https://github.com/dart-lang/sdk/commit/77bd51c2dc4b9c5d3d761f90fab6713198b1b827
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add failing test for static/propagated elements #Patch Set 3 : Alternate fix #Patch Set 4 : Alternate fix 2 #
Messages
Total messages: 18 (2 generated)
leafp@google.com changed reviewers: + brianwilkerson@google.com, jmesserly@google.com
Invocations of prefixed identifiers have propagated parameter elements, whereas invocations of non-prefixed identifiers don't (see the MethodInvocation code in element_resolver.dart for the case where target is null). This just fixes up all of them to have the correct parameter element as the propagated parameter element.
https://codereview.chromium.org/1477953002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1477953002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1727: correspondingPropagatedParams; I don't understand why we would need to do this. The property correspondingPropagatedParameters is only suppose to be set if the invocation (math.max in this case) was resolved to a different method using propagated type information than it was when using static type information. But that's not the case here. Where does the test fail if we don't do this?
https://codereview.chromium.org/1477953002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1477953002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1727: correspondingPropagatedParams; On 2015/11/25 21:39:40, Brian Wilkerson wrote: > I don't understand why we would need to do this. The property > correspondingPropagatedParameters is only suppose to be set if the invocation > (math.max in this case) was resolved to a different method using propagated type > information than it was when using static type information. But that's not the > case here. Where does the test fail if we don't do this? yeah, I was wondering the same thing. We're essentially trying to update the static types everywhere (including function type & parameters & return type). So far I don't think "strong mode" has had to deal with propagated type. But, thinking through it... I think I see what's going on. We're now treating generic methods as more "real" than we did before. However: we still don't check for generic methods type arguments when we look up methods. So upwards inference is the only way that type arguments can get filled in. Therefore, there's nothing to ever fill them in for the propagated types, and in particular, the propagated parameters. So ... I think this fix makes sense ... but, shouldn't we do it for the function & return type as well? Thoughts?
https://codereview.chromium.org/1477953002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1477953002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1727: correspondingPropagatedParams; On 2015/11/25 22:06:59, John Messerly wrote: > On 2015/11/25 21:39:40, Brian Wilkerson wrote: > > I don't understand why we would need to do this. The property > > correspondingPropagatedParameters is only suppose to be set if the invocation > > (math.max in this case) was resolved to a different method using propagated > type > > information than it was when using static type information. But that's not the > > case here. Where does the test fail if we don't do this? > > yeah, I was wondering the same thing. We're essentially trying to update the > static types everywhere (including function type & parameters & return type). So > far I don't think "strong mode" has had to deal with propagated type. > > But, thinking through it... I think I see what's going on. We're now treating > generic methods as more "real" than we did before. However: we still don't check > for generic methods type arguments when we look up methods. So upwards inference > is the only way that type arguments can get filled in. Therefore, there's > nothing to ever fill them in for the propagated types, and in particular, the > propagated parameters. > > So ... I think this fix makes sense ... but, shouldn't we do it for the function > & return type as well? Thoughts? BTW, "makes sense" in the temporary sense... I don't think we'll need this once we have a way of explicitly filling in type arguments (or implicitly filling in dynamic)... that's the TODO on line 1712.
On 2015/11/25 21:39:40, Brian Wilkerson wrote: > https://codereview.chromium.org/1477953002/diff/1/pkg/analyzer/lib/src/genera... > File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): > > https://codereview.chromium.org/1477953002/diff/1/pkg/analyzer/lib/src/genera... > pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1727: > correspondingPropagatedParams; > I don't understand why we would need to do this. The property > correspondingPropagatedParameters is only suppose to be set if the invocation > (math.max in this case) was resolved to a different method using propagated type > information than it was when using static type information. But that's not the > case here. Where does the test fail if we don't do this? I was surprised that methods invocations were getting both propagated and static elements, but this seems to happen in this case, and I don't think it's strong mode specific. If you look at visitMethodInvocation in element_resolver.dart, math.max will fall into the last case of the first if-then-else, corresponding to the case that the target is non-null. In that branch, both the propagated elements and the static elements will be set if there is both a propagated and static type, which there seems to be. I can look into why both of these are set.
I just added a (failing) test that shows the issue. With a regular library (no strong mode, no generic methods), there is both a propagated and static element for method names in a method invocation with prefixed access. Unless I'm missing something, this will result in both the correspondingStaticParameters and the correspondingPropagatedParameters being set in element_resolver.dart. I'll keep looking into why both of those are set. An alternative short term fix would be to just not set the correspondingPropagatedParameters in that method if there is a staticElement.
https://codereview.chromium.org/1477953002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right): https://codereview.chromium.org/1477953002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1727: correspondingPropagatedParams; That still doesn't make sense to me, but perhaps I'm being dense. Prior to strong mode, the propagated parameters property would have been used in a case like the following: var s = 'string'; s.codeUnitAt(3); The static type of 's' would be 'dynamic', so we would have been unable to resolve 'codeUnitAt', which means that the static parameters would have been unknown. But the propagated type of 's' would have been String, allowing us to resolve 'codeUnitAt' to the method element for String.codeUnitAt, which would have allowed us to set the propagated parameters to the parameter elements for the method String.codeUnitAt. (The only time the lists are different are when there are optional parameters, and the order can only be different when there are named parameters.) The invocation of a top-level or static method can never have propagated parameters because there's only one way to resolve it. (Or, equivalently, there is no target, so resolution cannot be based on a propagated type.) The presence or absence of type arguments does not impact that. > We're now treating generic methods as more "real" than we did before. However: we still > don't check for generic methods type arguments when we look up methods. We will never use the type arguments when looking up a generic method because we don't need to. Generic methods still define a single element in the element model, just like non-generic methods, and lookup is done by name. > So upwards inference is the only way that type arguments can get filled in. > Therefore, there's nothing to ever fill them in for the propagated types ... Ok... > ... and in particular, the propagated parameters. I'm not sure what you mean by "filling in" type arguments in a parameter element. We will definitely need to be able to compare the actual type of the argument against the substituted type of the parameter, but the parameter *element* will always have the method's type parameter as it's type.
> I just added a (failing) test that shows the issue. The test looks valid to me. We should not have a static and propagated element for a top-level function. We only keep propagated information where it is different than the static information. Sounds like the presence of a propagated type is the real bug, not the absence of propagated parameters.
I think we got far afield here. There may or may not be an issue related to generic methods and strong mode, but right now I think the first issue has nothing directly to do with them, but rather is this: On 2015/11/25 22:46:37, Brian Wilkerson wrote: > The invocation of a top-level or static method can never have propagated > parameters because there's only one way to resolve it. (Or, equivalently, there This is not the case right now (correctly, or incorrectly). Somewhere around line 628 in element_resolver.dart there are two successive calls to _resolveInvokedElementWithTarget. In the test case which I just added (which has nothing to do with strong mode, nor to do with generics), target will be the library name, the type argument (staticType and propagatedType respectively) will be null, methodName will be max, and isConditional will be false. Both calls will therefore return the same element, which then gets used immediately below that to fill in the propagatedParameters. If this is not supposed to happen, then I can fix it there by checking to see if we have a staticElement, or if the staticElement is equal to the propagateElement, and that will fix this immediate issue as well (I believe).
> > The invocation of a top-level or static method can never have propagated > > parameters because there's only one way to resolve it. > > This is not the case right now (correctly, or incorrectly). > > If this is not supposed to happen, then I can fix it there by checking to see if > we have a staticElement, or if the staticElement is equal to the > propagateElement, and that will fix this immediate issue as well (I believe). Yes, that should not happen. We might be able to fix it even earlier, and hence more efficiently, by noticing that the target is a prefix and not even doing the lookup using the propagated type.
Ok, patch set 3 has the simple "check if they're equal after the fact" fix. On 2015/11/25 23:12:10, Brian Wilkerson wrote: > We might be able to fix it even earlier, and hence more efficiently, by noticing > that the target is a prefix and not even doing the lookup using the propagated > type. So would this just be something like checking if the target is a SimpleIdentifier whose element is a PrefixElement? As long as it is never the case that such a simple identifier could have an interface type, then it should be equivalent. I'll give it a shot.
LGTM
Hmm... there's another path just above the _resolveInvokedElementWithTarget path in which getTypeReference returns non-null, which also results in the static and propagated elements being equal. I'm worried about cutting off above that. It seems like that line should not be setting the propagatedElement either? On 2015/11/25 23:21:45, Leaf wrote: > Ok, patch set 3 has the simple "check if they're equal after the fact" fix. > > On 2015/11/25 23:12:10, Brian Wilkerson wrote: > > We might be able to fix it even earlier, and hence more efficiently, by > noticing > > that the target is a prefix and not even doing the lookup using the propagated > > type. > > So would this just be something like checking if the target is a > SimpleIdentifier whose element is a PrefixElement? As long as it is never the > case that such a simple identifier could have an interface type, then it should > be equivalent. I'll give it a shot.
Patch set 4 has an alternative early cut off version. Thoughts, preferences between the two?
The most recent version looks best to me. The sooner we can detect that type propagation isn't buying us anything the less work we'll do. LGTM
Description was changed from ========== Fix propagated argument element bug. This fixes #25040. BUG= ========== to ========== Fix propagated argument element bug. This fixes #25040. BUG= R=brianwilkerson@google.com Committed: https://github.com/dart-lang/sdk/commit/77bd51c2dc4b9c5d3d761f90fab6713198b1b827 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 77bd51c2dc4b9c5d3d761f90fab6713198b1b827 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
