When renaming something to ~Attribute, Visual Studio shows an extra “Attribute” at the end

[poke]

Version Used: Visual Studio 16.1.0 Preview 2.0

🔗 Also reported as AB#1271881

Steps to Reproduce:

1. Press Ctrl + R, R to rename a member.
2. Rename the member so that its name ends with Attribute

Expected Behavior: Visual Studio should preview the new name correctly.

Actual Behavior: Visual Studio previews the new name with another “Attribute” at the end of the name. This extra “Attribute” is not kept when submitting the refactoring though.

This might be a regression from #22955 and/or #21657.

---

This behavior is best explained by the following GIF: Here, I have a property MyProp and try to rename it to MyPropAttribute. As soon as I finish typing that name, Visual Studio displays another “Attribute” at the end of the name. If I submit the rename refactoring, that extra “Attribute” does not actually appear though.

[yvan-solutions]

As I understand, this is year old bugs that go reintroduced in Visual Studio 2015. Is this fix going to be included in a VS2k15 update or is it VS2k19 only?

[tamlin-mike]

It seems to me this bug is due to an inverted comparison.

• If the new type name is not ending in Attribute, nothing at all is appended.
• If it is ending in Attribute, another instance of Attribute is appended.

That seems to be the completely opposite of what would be both logical and reasonable. Ergo, inverted condition, and should be very quick and easy t fix by someone knowing the code.

A cursory grep of the source suggest first places to look could be CSharpRenameRewriterLanguageService.cs or ExpressionSyntaxExtensions.cs.

Remaining issue, after this is fixed, could be to make sure the replaced usages of the new typename doesn't include the Attribute suffix. I.e. usage of an attribute type AttrBaseNameAttribute would be

[AttrBaseName]

not

[AttrBaseNameAttribute]

[tamlin-mike]

I might have found the bug

https://github.com/dotnet/roslyn/blob/bdcf1326865171e2581668979415e823f3d7b0ed/src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs#L605-L609

I don't know if it's as simple as negating name.EndsWith, but it looks like a promising candidate.

[RikkiGibson]

Would be happy to have a PR for this :)

[tamlin-mike]

Alternative (or additional) potential problem source

https://github.com/dotnet/roslyn/blob/e704ca635bd6de70a0250e34c4567c7a28fa9f6d/src/EditorFeatures/Core.Wpf/InlineRename/AbstractEditorInlineRenameService.SymbolRenameInfo.cs#L188-L190

It seems possible (even likely) _isRenamingAttributePrefix should be in that check.

[RikkiGibson]

@tamin-mike please let us know if you need any pointers getting started with this :) The gitter channel may be helpful here

[tamlin-mike]

Forget that previous comment by me. Should teach me to look for problem sources when almost hibernating. The following could however make sense (L95 in that same file).

- return triggerText.StartsWith(triggerText); // TODO: Always true? What was it supposed to do?
+ return nameWithoutAttribute.StartsWith(triggerText);

Even if it doesn't solve the bug, at least it seems it could make the code more correct.

As for fixing the bug, forking, signing SLA, ... Nah, let's see how many years this issue stays open. :-)

[poke]

@tamlin-mike If you don’t want to contribute, I would attempt to pick this up. The information you pointed out should give me a starting point for where to look at.

@RikkiGibson I haven’t contributed to Roslyn yet. I assume that I will have to test this change in VS; is this manual the right thing to look at for that?

[tamlin-mike]

@poke Be my guest. But before spending any time on it, please read on.

Having slept on the issue, I have started to wonder if this "help" is really needed, or even wanted, at all. I mean, if you are creating an Attribute subclass, we'd have to assume you already know what you are doing, and that the Attribute identifier-suffix-hack in the language should be used.

Additionally, this (currently anti-) feature isn't anything a customer have asked for. If it was, that customer would have flogged Microsoft for providing this implementation. :-)

Would it therefore perhaps make sense to simply remove this hack, optionally replacing it with an Analyzer that could offer to rename Xyzzy to XyzzyAttribute? That seems like a much cleaner separation of responsibility than the current code, by letting InlineRename do Rename, and nothing else.

Just a thought.

[poke]

@tamlin-mike Sorry but I cannot really follow. This is a real issue that my team encountered which made me create this issue, and I still regularly hit this issue. There are also at least two other issues about the same thing that have been closed as duplicate.

I personally don’t need this feature that the attribute suffix is kept during rename but I cannot speak for others. It is very difficult if not impossible to gauge whether any customers use the functionality or not. But I would be very reluctant to remove a feature that has been shipping for as long as this has; even if it causes a different issue.

This problem is a bug; it is one that could be avoided when the feature wasn’t there but it is also one that is fixable without changing the feature set. So I would avoid starting to discuss a feature removal which neither you nor I can decide, and which would likely have to involve a lot more people. Instead, we can just focus on fixing this obvious bug that exists in a shipped feature.

[RikkiGibson]

Yeah the "Building, Debugging and Testing" doc you linked is pretty accurate. The only thing I'd suggest to speed up your workflow is that when you debug RoslynDeployment, use the "Start Without Debugging" command so that the debug instance of VS will launch much faster. (you can just attach it once you've loaded a project and set up the scenario you want to hit a breakpoint in)

[RikkiGibson]

I do not know what this issue is in detail but it seems like the intent was to preserve the user's "style" of referencing the attribute class-- did they write [FooAttribute] or just [Foo], and during a refactoring, preserving that style.

Keep in mind that when you refactor you must (iirc):

1. complexify, i.e., fully qualify all names in the code
2. actually change the name in question
3. simplify, i.e. remove all qualifiers that are still unneeded after the refactor

I assume that something about this loop required special handling to preserve whether the user explicitly used the 'Attribute' suffix in the name or not. The fact that this is happening outside the context of refactoring an attribute seems wrong.

[tamlin-mike]

@poke Oh, don't get me wrong. I have also been bitten by this bug, just yesterday in fact. That's why I suggested removing this "feature" completely, since it really is a hack in the renaming refactoring and doesn't really belong in it.

It is very difficult if not impossible to gauge whether any customers use the functionality or not.

No it's not. It's so buggy it seemingly tries to drive anyone encountering it insane. That makes it, IMHO, almost certain it's unwanted.

That said, I appreciate your standpoint of rather simply fixing the bug, than telling Microsoft it's the feature itself that is a bug. Me, I'm not so diplomatic. I call it as I see it. :-)

@RikkiGibson From my understanding, it's rather that it tries to be helpful when you rename XAttribute, intended to allow you to only rename the X part while retaining the Attributepart. That has nothing to do with users "style", and 100% to do with trying to provide a helpful/handholding workaround for the C# language hack of auto-appending "Attribute" to Attribute type use at compile time.

The reasons I suggest simply removing this buggy code from InlineRename is:

• The obvious; It's buggy.
• It really has no business being in InlineRename. I'm a firm believer in "Do one thing, and do it well". This hack goes contrary on both accounts.
• It could probably, as I suggested, be better served by an analyzer.

But I digress.

[CyrusNajmabadi]

Guys, this convo isn't really helping.

1. The behavior in inline rename is a bug. It should be fixed. Community PRs welcome.
2. The behavior of C# wrt attribute naming is not a hack. It's been the stated design and desired behavior since C# 1.0. Users expect VS to understand this universal style and to operate properly with it.

That's it :)