dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.04k stars 4.03k forks source link

Variable naming suggestions should prefer long form #20927

Open sharwell opened 7 years ago

sharwell commented 7 years ago

Version Used: 15.3 Preview 4

Steps to Reproduce:

  1. Open a file with an existing method, which also has an import to System.Threading
  2. In the method, start to declare a local variable by typing CancellationToken c

Expected Behavior:

cancellationToken is the suggested name (soft selection).

Actual Behavior:

cancellation is the suggested name (soft selection).

CyrusNajmabadi commented 7 years ago

While i agree that this naming isn't great for us for this specific case, i'm not certain if we want to change the overall approach here. In many (possibly most) cases i definitely want the short form. And, indeed, in a lot of code i've seen people write "CancellationToken token", so the full form doesn't seem like something we should default on.

Now what we could consider is making it so that this properly plays with MRU, and/or considers the surrounding context when offering names. If it learned that in Roslyn we want 'cancellationToken' that would be great!

alrz commented 7 years ago

Could we default on the recently used or the recently most used forms (potentially in a per-type setting)?

sharwell commented 7 years ago

@CyrusNajmabadi For the specific case of naming suggestions, it's my long-term observation that developers will typically start typing characters at some index within the type name, and the name they want is the text from there to the end of the type name.

in a lot of code i've seen people write "CancellationToken token"

We previously offered no assistance at this particular location. The use of short names is a symptom of a failure to aid the user, not an indication of what the user would have preferred.

sharwell commented 7 years ago

:memo: Also note that the long form of these identifiers is more specific. On the basis that more descriptive identifiers tend to be more readable than less descriptive ones, I believe that moving to the behavior I described is more representative of the direction we're trying to move towards, so we should start there and make adjustments based on the feedback we get.

tannergooding commented 7 years ago

I definitely find the long form to be a better default. It is really annoying if you have several names that start with the same prefix and to always have the short form (which conflicts) offered.

It might not be what everyone wants (and an option is probably the only way to truly remedy it), but I feel generating a non-conflicting name using the "long form" is the best default.

Pilchie commented 7 years ago

@kuhlenh Let's watch for some more feedback on this once 15.3 ships and decide what (if anything) to do here.

CyrusNajmabadi commented 7 years ago

I feel like any 'default' we pick will be wrong for some subset of users or some subset of use cases. Tihs really does feel like a feature that needs to learn from how the user uses those specific names in context to best elevate the items.

We already have all the tech necessary for this. It seems like it would be trivial to make use of it to have results that end up becoming great for any user almost immediately, even if the default isn't ideal for some.

tannergooding commented 7 years ago

@CyrusNajmabadi, at the very least, if we didn't always offer the long form, I would hope that we could determine the shortest non-conflicting name and offer that (and not offer a name if another variable with the same name already exists).

CyrusNajmabadi commented 7 years ago

@tannergooding Feel free to file a bug on that :)

CyrusNajmabadi commented 2 weeks ago

Would take a targeted community fix here.