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

Lightbulb suggests renaming a type to "var" #2605

Closed
ghost opened this issue May 8, 2015 · 10 comments · Fixed by #2766
Closed

Lightbulb suggests renaming a type to "var" #2605

ghost opened this issue May 8, 2015 · 10 comments · Fixed by #2766

Comments

@ghost
Copy link

ghost commented May 8, 2015

Screen shot says it all:

rename type to var

@pharring
Copy link
Contributor

pharring commented May 9, 2015

@lgolding Is this with VS 2015 RC? If not, what version of VS and Roslyn are you using?

@ghost
Copy link
Author

ghost commented May 9, 2015

Yes, 2015 RC. 22822.1 IIRC, although I don't have it in front of me here on the bus :)

Sorry, I included that information when I originally filed the bug in TFS, but forgot to mention it when I re-filed it in GitHub.

Larry

Sent from my Windows Phone


From: Paul Harringtonmailto:[email protected]
Sent: ‎5/‎8/‎2015 5:35 PM
To: dotnet/roslynmailto:[email protected]
Cc: Larry Goldingmailto:[email protected]
Subject: Re: [roslyn] Lightbulb suggests renaming a type to "var" (#2605)

@lgoldinghttps://github.com/lgolding Is this with VS 2015 RC? If not, what version of VS and Roslyn are you using?


Reply to this email directly or view it on GitHubhttps://github.com//issues/2605#issuecomment-100399749.

@dpoeschl
Copy link
Contributor

@lgolding Did you manually replace the type name with "var" by typing "var" (or pasting it or making some other edit), or did you run an automated fix that reduced the type name to "var" automatically?

Some background: When a reference is tracked for rename, we usually show the dotted border around the identifier. But, once the identifier binds to something again (whether a different symbol or, in this case, the same symbol), the dotted border goes away because you're probably done with your edit. But, we continue to allow the rename to be executed just in case the user really wants to rename the original underlying symbol to a name already used by another symbol (probably introducing an ambiguity at the current reference, but that's their decision), or they really want to rename the underlying symbol to "var" in this case, since it would be a legal rename.

There's a few things we could do here:

  1. Don't offer RenameTracking fixes when the new name binds to the same symbol as the old name (this wouldn't help if the var in your example were previously some base type or interface of ResultCollection, but it would fix cases where the var is the same type as the one replaced).
  2. Special case "var" in RenameTracking and disallow renames to it.
  3. Disallow RenameTracking after any type of "programmatic edit" -- If you replaced the "ResultCollection" with "var" by invoking a code fix that converted it to "var", then we would have to detect that the edit was caused by a programmatic edit rather than a user edit and reset the Rename Tracking state. This has been discussed before but it quickly gets tricky and error prone, so we haven't implemented it.
  4. Do nothing. We've removed the dotted outline in this case to indicate that Rename Tracking doesn't think you need to do anything, and renaming to "var" would indeed be valid.

For now, I'd vote for either 4 (do nothing because it would be a legal rename) or 2 (special case "var" in Rename Tracking).

@ghost
Copy link
Author

ghost commented May 11, 2015

Hi David,

I manually edited the type name to var.

This bug report is a false positive. I did not realize that “var” is a legal type namehttps://msdn.microsoft.com/en-us/library/bb384061(v=vs.140).aspx: “If a type named var is in scope, then the var keyword will resolve to that type name and will not be treated as part of an implicitly typed local variable declaration.”

That said, I would still vote for #2 (“special case var”) because I think it's extremely unlikely that somebody who makes that particular edit really intended to rename their type to “var”, so almost always the user would see a lightbulb that does not help them.

Thanks,
Larry

@Pilchie
Copy link
Member

Pilchie commented May 11, 2015

I'm also in favor of just special casing var (and dynamic).

@Pilchie Pilchie added this to the 1.0 (stable) milestone May 11, 2015
dpoeschl pushed a commit to dpoeschl/roslyn that referenced this issue May 14, 2015
Fixes dotnet#2605 "Lightbulb suggests renaming a type to 'var'"

This change disables using Rename Tracking to rename to/from "var" and "dynamic" in C#.
@ghost
Copy link
Author

ghost commented May 15, 2015

Thanks David!

Sent from my Windows Phone


From: David Poeschlmailto:[email protected]
Sent: ‎5/‎14/‎2015 7:39 PM
To: dotnet/roslynmailto:[email protected]
Cc: Larry Goldingmailto:[email protected]
Subject: Re: [roslyn] Lightbulb suggests renaming a type to "var" (#2605)

Closed #2605#2605 via #2766#2766.


Reply to this email directly or view it on GitHubhttps://github.com//issues/2605#event-305446828.

@dpoeschl
Copy link
Contributor

@lgolding No problem, thanks for reporting it! Can you take a look and verify that it works as expected in this case now and add the Verified label if so? I can find someone else to take a look if needed -- just assign the issue back to me if that's the case.

@dpoeschl dpoeschl removed their assignment May 18, 2015
@dpoeschl
Copy link
Contributor

Actually, I can't assign it to you for some reason, @lgolding. Do you know why? /cc: @srivatsn

@ghost ghost assigned dpoeschl May 18, 2015
@ghost
Copy link
Author

ghost commented May 18, 2015

@dpoeschl Taking you up on your offer to find someone else to verify this, thanks.

@dpoeschl
Copy link
Contributor

Simple enough bug, verified myself.

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.

4 participants