You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
@bwilkerson I'm investigating why the Inline Method refactor doesn't work correctly when the method has type args (No argument for the parameter "a".). It seems to fail because code in the refactor doesn't match up the arguments with the parameters correctly.
The code compares arg.staticParameterElement == parameter (see highlighted line below), but when there are type parameters these do not match (arg.staticParameterElement is a ParameterMember and not the ParameterElement).
ParameterMember has a .declaration which looks similar (ParameterElementImpl (T a)), but it's not the same instance. The one in parameter is fully populated, but the one in the ParameterMember is not (it's synthetic and looks a bit like a placeholder.. it has -1 for nameOffset for example).
I'm not sure whether a) my code is bad and something is not resolving correctly, b) this is a bug and that parameter declaration should be non-synthetic and match, or c) this is expected and the refactor should match the parameters in a different way (for example comparing the names?).
Yes, we have issues around the way generic method invocation resolution is implemented. IIRC we get the generic FunctionType, eventually instantiate it, and use ParameterElements of this new instantiated FunctionType to bind arguments. This causes a few issues, and a few exceptions.
Maybe we should do what we did for RecordType, and FunctionType should not pretend that it has ParameterElements, it should have positional and named formal parameter... types?
This will not fix this immediate issue though.
We probably should instantiate corresponding ExecutableElement, with its ParameterElements, and use these for resolution. Relatedly, calling these MethodMember and FunctionMember might be not the best. FunctionElement is not a member of a ClassElement. And, for this issue, it does not even have MapSubstitution substitution, only bool isLegacy.
I don't understand this well enough to know what the scale of that work is - is it significant/breaking? (and/or something I can do without significant hand-holding?).
Out of curiosity, I did try changing it to just compare based on name, and it passes the tests:
Activity
DanTup commentedon Sep 22, 2022
@bwilkerson I'm investigating why the Inline Method refactor doesn't work correctly when the method has type args (
No argument for the parameter "a".). It seems to fail because code in the refactor doesn't match up the arguments with the parameters correctly.The code compares
arg.staticParameterElement == parameter(see highlighted line below), but when there are type parameters these do not match (arg.staticParameterElementis aParameterMemberand not theParameterElement).ParameterMemberhas a.declarationwhich looks similar (ParameterElementImpl (T a)), but it's not the same instance. The one inparameteris fully populated, but the one in theParameterMemberis not (it's synthetic and looks a bit like a placeholder.. it has -1 fornameOffsetfor example).I'm not sure whether a) my code is bad and something is not resolving correctly, b) this is a bug and that parameter declaration should be non-synthetic and match, or c) this is expected and the refactor should match the parameters in a different way (for example comparing the names?).
Some tests and comments are here if it helps:
https://dart-review.googlesource.com/c/sdk/+/260580/
bwilkerson commentedon Sep 22, 2022
@scheglov
scheglov commentedon Sep 22, 2022
Yes, we have issues around the way generic method invocation resolution is implemented. IIRC we get the generic
FunctionType, eventually instantiate it, and useParameterElements of this new instantiatedFunctionTypeto bind arguments. This causes a few issues, and a few exceptions.Maybe we should do what we did for
RecordType, andFunctionTypeshould not pretend that it hasParameterElements, it should have positional and named formal parameter... types?This will not fix this immediate issue though.
We probably should instantiate corresponding
ExecutableElement, with itsParameterElements, and use these for resolution. Relatedly, calling theseMethodMemberandFunctionMembermight be not the best.FunctionElementis not a member of aClassElement. And, for this issue, it does not even haveMapSubstitution substitution, onlybool isLegacy.DanTup commentedon Sep 23, 2022
I don't understand this well enough to know what the scale of that work is - is it significant/breaking? (and/or something I can do without significant hand-holding?).
Out of curiosity, I did try changing it to just compare based on name, and it passes the tests:
https://dart-review.googlesource.com/c/sdk/+/260580/2/pkg/analysis_server/lib/src/services/refactoring/legacy/inline_method.dart
I can't think of any reasons why this wouldn't be sound, but I'm not certain there aren't any :-)
scheglov commentedon Sep 26, 2022
Yes, this looks as a relatively big piece of work.
Your solution looks good, I left a couple of comments.
DanTup commentedon Sep 28, 2022
Fixed by dart-lang/sdk@813aa69.