Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename does not work directly after ExtractMethod (with unknown parameter types) #10914

Closed
robinsedlaczek opened this issue Apr 27, 2016 · 4 comments · Fixed by #75752
Closed

Comments

@robinsedlaczek
Copy link
Contributor

Version Used:

Visual Studio 2015 Update 2

Steps to Reproduce:

  1. Create a method like this:
public void Foo()
{
    var foo = MissingMethod();

    if (foo)
    {
        // FooBar...
    }
}
  1. Select the complete if-statement, press CTRL+. and extract it into a new method. Since MissingMethod() is realy a missing method, the type for foo cannot be determined. That's why the type of the parameter in the new method will be object.
  2. Try to rename the new method (with F2) at the caller side.

Actual Behavior:

Error message appears stating that the element cannot be renamed.

Expected Behavior:

I would expect that the method can be renamed because:

  1. I created the method right before.
  2. There is only one method with that name in the scope.
  3. The number of parameters passed to the method fits the signature.
  4. When "ExtractMethod" assumes object for foo while the type of foo cannot be determined, why can't Rename?

Otherwise the error message should be adapted a bit with some more infos why Rename is not possible (e.g. "You cannot rename this element because the type of some parameters cannot be determined.").

Impact:

That Rename does not work if it is used at the caller side and types of parameters cannot be determined makes sense since there could be overloads of this method. But that is not the case in this particular situation when the method was created with ExtractMethod right before. And maybe it is a minor problem because there are several other ways to rename the method. But it can confuse people if the code is more complex like shown in the following screenshot:

renamemethoddoesnotwork

In line 46, the InteractiveAssemblyLoader class cannot be found because of a missing import. That is the only problem that is reported to the user. So renaming the method ProcessScriptResult in line 56 seems to be no problem. But it is. Reason: due to the missing import, the type of varibale assemblyLoader cannot be determined. Then this variable is passed to method CSharpScript.Create() in line 48. That leads to the problem that the return type of the Create method can not be evaluated (compiler is not able to select the correct overload). So type of variable script is unknown. So method RunAsync() in line 54 cannot be resolved and variable state has no type as well. Finally Rename on method ProcessScriptResult() in line 56 does not work.

It was a bit confusing for me because of the distance from line 56 to the real problem.

@Pilchie
Copy link
Member

Pilchie commented Apr 27, 2016

Similar to #4904 and #3584. Basically - the thing being renamed has to match without errors, but I'm not sure if I like that.

@Pilchie Pilchie added the Bug label Apr 27, 2016
@Pilchie Pilchie added this to the Unknown milestone Apr 27, 2016
@robinsedlaczek
Copy link
Contributor Author

Same to me. Especially in the case I described it was confusing because there is no error (on the first view). Knowing that a variable has no type requires moving the mouse over it to have a look at the tooltip. And the question mark for a missing type does not really imply an error (IMO). Next, you have to know that renaming requires match without errors. Two things that are basically not present for everybody.

In the case of methods, when I want to change the name, why should it depend on the parameters used at the callers side? Intentionally, I want to change the name, not the signature. In this moment, parameters given at the caller side are not interesting for me. Otherwise, I would like to change signature or to create a new method (that could possibly be an overload).

Next, we know that the syntax node is a method call for which we can find a declaration (or not). So I guess we could rename this method. I agree, Rename should not be possible if the syntax node is not recognized as a method call or if there is a syntax error around. Assumptions will be the consequence.

So what are the reasons that it must match without errors?

@Pilchie
Copy link
Member

Pilchie commented Apr 27, 2016

when I want to change the name, why should it depend on the parameters used at the callers side

Because if the method is overloaded, we have to know which overload to rename.

@robinsedlaczek
Copy link
Contributor Author

But if there is no overload, it can be renamed.

What about an option, and let the user choose? There is already a visual hint when the new name equals the name of another element. So if there are overloads in case of method rename, the same UI approach could be taken. And if I know what I do, I want to rename all overloads or not.

On the other Hand, I cannot guess how dangerous it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants