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
18.71k stars 3.98k forks source link

Pressing Tab key causes unwanted code changes #68873

Closed levicki closed 12 months ago

levicki commented 1 year ago

Please consider this code:

image

Pressing the Tab key while the cursor is positioned immediately after the Class property results in IDE making this change:

image

I have removed all the code snippets for C#:

image

I have chosen to never include snippets:

image

But the IDE refuses to allow me to insert a tab character after the word Class, and instead performs this nonsensical change resulting in broken code.

Furthermore, copying Tab character from elsewhere in IDE and pasting it after the word Class results in this change:

image

That's right, the code is reformatted (which I didn't ask for) and Tab character is not inserted (which I did ask for).

If at this point I hit Ctrl+Z, the alignment will be restored and the Tab character will appear to the right of the word Class.

Maybe I am missing some additional IDE setting which can disable this madness, but even so this is a seriously f*cked up default behavior and it cannot but remind me of Charles Petzold saying back in 2005 how Ctrl+Z is his most used key in Visual Studio and Word -- I am literally spending more time fighting the IDE than coding because of issues like this.

I am observing this problem in Visual Studio 2022 version 17.6.4.

Please fix.

CyrusNajmabadi commented 1 year ago

@levicki We'd def take a PR here to address this issue if you're interested in contributing!

levicki commented 1 year ago

@levicki We'd def take a PR here to address this issue if you're interested in contributing!

@CyrusNajmabadi

If you don't believe the issue is serious enough to assign a dev to fix it for next point release, then I am afraid I can't help. I have a paid full time job and this constant fighting with VS IDE is already negatively affecting my productivity. Even taking the time to report this was the time I was supposed to be spending writing the code for living, and meeting my deadlines at work.

Learning my way around the Roslyn codebase in order to be able to understand where the problem is and how to fix it is the time I unfortunately cannot spare at the moment nor in any forseeable future — the best I can do is report issues like this, help you reproduce, and eventually confirm the fix once it is available.

Furthermore, even if it weren't for the lack of time and domain-specific knowledge, the company I work for is already contributing to your team indirectly through various licencing fees (VS, E3, Azure, etc) they are paying to Microsoft on a regular basis.

Finally, open-sourcing just a part of a commercial product is a great way of privatizing profits and socializing lossesdevelopment costs where the benefit of open-sourcing is heavily biased towards the company cashing in on the commercial product, not towards open-source community — I have a great dislike for such practice which is another incentive not to contribute. If that means this issue will never be fixed, so be it.

CyrusNajmabadi commented 1 year ago

where the benefit of open-sourcing is heavily biased towards the company

It seems like it's a benefit to both sides. Users are no longer at the mercy of if an issue in the closed source side ever gets to being fixed. So if the issue isn't able to make it off the backlog based on the rest of the work the team is doing, teh community always has an option of working on things if the issue is important to them. It's a strictly better state of affairs than if the product was closed source, and i think the rest of the ecosystem, community, and contributors would prefer we stay open source :)

Learning my way around the Roslyn codebase in order to be able to understand where the problem is and how to fix it is the time I unfortunately cannot spare at the moment nor in any forseeable futur

Understood. If you ever change your mind, let us know and we'd be happy to work with you on this. Thanks!

levicki commented 1 year ago

... the community always has an option of working on things if the issue is important to them

You seem to be (intentionally?) conflating community and paying customers.

I am sure there are a lot of people using free Visual Studio Community Edition, but there are still many of us using Professional or Enterpise subscriptions.

While the former may be happy with the ability to fix things they consider important themselves, the latter are rightfully expecting that your team will fix the reported issues instead of relying on community contributions let alone suggesting they should roll up their sleeves and contribute a fix themselves.

This "option" also allows your team to work on issues important for internal consumption, and / or on implementing "pet features" both of which only a minority of external users might benefit from, while leaving the "boring backlog" to the community to sort out which can lead to issues reported by paying customers being neglected.

Just my 0.02.

CyrusNajmabadi commented 1 year ago

the latter are rightfully expecting that your team will fix the reported issues

We fix issues we have the bandwidth to fix, depending on importance and whatever else is necessary to do. That means (same as when we were closed source) there are large swaths of things that we will not get around to. Our velocity though is much higher now that we're open source, and we're able to both do more work, make more fixes, and incorporate more community efforts now that we're open source :)

instead of relying on community contributions let alone suggesting they should roll up their sleeves and contribute a fix themselves.

We regularly do. To the order of many thousands of fixes and changes. But not every reported issue will be able to get effort to fix it. So we often work with interested parties to find other ways to get the fixes in so that the people who care about them get the results quicker than having to wait for the item to make it off the backlog :)

while leaving the "boring backlog" to the community to sort out which can lead to issues reported by paying customers being neglected.

From above, there is more work needed from all our customers (including paying ones) than there are resources available to address them. This was true when we were closed source, but our velocity is much higher now that we are open source.

If you are ever interested, def drop us a line. We are very active in communities like discord, and lots of the team engages there to work with people from the community to solve problems people are running into, and to help expedite fixes for areas people are passionate about (often into the very next preview release). DotNetEvolution/Roslyn is a great place to start. Thanks! :)

levicki commented 1 year ago

@CyrusNajmabadi Just to be perfectly clear, the problems I have with Visual Studio and other Microsoft products are not caused by Microsoft developers working on them — they are caused by bone-headed decisions made by higher-ups who chase some ridiculous metrics for product "improvement".

Sorry for the offtopic, because what I am about to share is in no way related to Roslyn, this issue, or your team, but I think you guys need to see the latest crazy example of "what were they thinking?" moment I just had in order to better understand my frustration:

image

Whose "bright" idea was to greenlight this search behavior?!?

Showing those other results == blatant ignoring of my explicitly set search filters — I didn't ask for "other results" (especially not totally wrong ones!) just for the sake of having a full page of results.

This is not Bing web search where returning just a couple of (or not returning any) results might turn people away and make them use another search engine! This is a professional product aimed at developers who by definition should know what they are doing and be exact and concise. Why are the search filters there if they aren't respected? Doesn't the person who greenlighted this know that the most cardinal sin any application can commit is to allow and then ignore user input?

You may think I am overreacting, but frankly I find this behavior condescending, insulting, and disruptive in the same manner as the IDE "correcting" what I typed, or Google / Bing showing me results that aren't relevant at all for the keywords I entered.

Every day there's more of this "user engagement at all costs" in every product and service, and it makes me not want to use them at all, let alone contribute to their development.

The real problem is the internal corporate culture in large companies which seems to be tuned to maximize shareholder profits through customer engagement which allows this engagement plague to spread, and I don't see that changing anytime soon. If it continues unabated, developers will soon be more productive using Notepad and csc from the command line, at least until those tools also get the same treatment.

Sorry for the rant, and have a good weekend.

CyrusNajmabadi commented 1 year ago

Whose "bright" idea was to greenlight this search behavior?!?

This simply sounds like a bug. Please file feedback from within VS and this can be routed and fixed.

You may think I am overreacting,

Bugs happen. Just file issues and we can take care of things. I've never once seen a process here that engages in any of the things you're speculating about to produce result like this. What really just happens is that VS itself is a massively complex beast of a platform, with code going back 30+ years, which is constantly evolving based almost entirely on customer requests. And with that amount of code, and all the combinations of components interacting, tiny issues like this can occur.

levicki commented 1 year ago

This simply sounds like a bug. Please file feedback from within VS and this can be routed and fixed.

I love your optimism, but having a separate section titled "other results based on your search" doesn't seem like a bug to me — someone had to consciously design that UI element into the product.

CyrusNajmabadi commented 1 year ago

doesn't seem like a bug to me —

It seems like a bug to me. And if be happy to track and engage with the team on this if you're willing to file the bug.

sharwell commented 12 months ago

@levicki I'm not sure why the snippet matching behavior is case-insensitive, but I wouldn't be surprised if the behavior is more than a decade old at this point. A viable workaround should be to press Space, Tab if the identifier to the left of the caret happens to be a snippet trigger.

If the new behavior only recently appeared, it probably means the new snippet experience (see the following image) is failing to account for manual removal of snippets using the Code Snippet Manager. Unchecking this feature to return to the old snippet manager would restore the behavior of earlier versions of Visual Studio. image

levicki commented 12 months ago

@sharwell Thanks for responding and providing some constructive feedback on the possible origin of the issue.

Sadly as you can see in one of the screenshots I provided in the initial report I have this new "experience" unchecked (I believe it is unchecked by default), and I can still reproduce the issue.

Settings which don't actually control what they claim to control are a recurring theme in Microsoft software it seems.

A viable workaround should be to press Space, Tab if the identifier to the left of the caret happens to be a snippet trigger.

Sorry, but that's not a viable workaround and here's why:

  1. Pressing Space, Tab would not accomplish having a Tab character after the word, it would add another layer of fighting the IDE to accomplish desired formatting. Pressing ; at the end of line would also remove the Tab even if you managed to enter it, and replace it with spaces despite everything in the IDE configured to use Tabs only (another longstanding issue I reported / commented on which was extensively discussed in this repo and went nowhere because "it's complicated").
  2. I don't have a comprehensive list of snippet triggers memorized.
sharwell commented 12 months ago

@levicki based on the history of this behavior, the exceptionally high complexity of altering the current behavior, and the fact that very few users report problems with the current behavior, I'm going to resolve this in two parts. First, my recommendation for addressing your current needs is to create a Visual Studio extension which handles spaces occurring within relevant lines of code and converts them back to literal \t characters (consistently referred to internally as "hard tabs"). This extension can be written such that pressing tab after an existing sequence of one or more spaces also absorbs the existing spaces into the new sequence of hard tab characters. This is a common recommendation for users who create customized code style policies which significantly deviate from practices seen throughout the majority of the .NET ecosystem, and allows those teams to focus on writing code instead of manually maintaining the customized styling.

Secondly, I'm going to resolve this issue as a duplicate of the old encompassing issue for the handling of tab characters in the middle of a line.

sharwell commented 12 months ago

Duplicate of #24031

levicki commented 12 months ago

@sharwell Thank you so much for completely misunderstanding the issue reported and "resolving" it in the worst possible way.

This issue is about code snippet still being inserted when Tab key is pressed even though all the snippets are deleted and snippet options are disabled, not about code formatting with Tab character.

I have no idea how anyone can see those two as a same thing.

The only conclusion I can draw from you "resolving" this issue without understanding it is that some Microsoft developers are actively hostile in their interaction with the customers and that they are so self-centered and full of themselves that they won't even stop to consider that their "perfect" product design might actually be user-hostile for some of their customers.

We have already seen that hostility and "we know what's good for you" attitude with Windows Terminal showing the notice about disabled Touch Keyboard service on every launch and Microsoft developers initially flat-out refusing to add an option to disable it after being seen once and even going as far as to mock and even insult the users who complained about it.

TL;DR — you aren't eating enough of your own dog food or you'd be more humble and willing to help resolve issues like this.

CyrusNajmabadi commented 12 months ago

@levicki The above post violates several parts of our code of conduct. Including engaging in a friendly and constructive manner and treating people with respect. Regardless of whether you agree with the thinking or decision making on something, lashing out at individuals is completely unacceptable and will not be tolerated here.

If you do disagree with things, be constructive and be professional. If you can do that, you're welcome to stay here and continue engaging with the team. If you can't do that, future interactions will be curtailed.

For more info, please see our code of conduct here: https://dotnetfoundation.org/about/policies/code-of-conduct

Please note in particular:

levicki commented 12 months ago

@CyrusNajmabadi Closing an issue as a duplicate just because he can without bothering to understand whether it is actually a duplicate (and it isn't, because I suspect you could reproduce it even if you use spaces) is demonstrating lack of empathy and kindness on his end, not to mention failing to gracefully accept my feedback or to apologize for his mistake.

But of course, "rules for thee and not for me" so I get a thinly veiled ban threat for stating the unpleasant truth, and he gets protected from "abuse". And the issue I reported? Who cares about that anyway? CLOSED. DUPLICATE. WONTFIX. FIXITYOURSELFYOUPEASANT.

This is the last issue / feedback / comment from me on anything Microsoft. Goodbye.

sharwell commented 12 months ago

@levicki It's not an exact duplicate, but it involves very old behavior which, to our knowledge, has not changed in more than a decade. Since I wasn't able to identify any other reports of this specific issue even though it's existed in the current form for so long, it would be difficult to prioritize over other issues which have many more reports. Since it additionally involves implementations interacting with complex legacy code, it wouldn't be fair for me to suggest that the behavior has any significant likelihood of changing in a future release.

The issue where tabs are not preserved after a whitespace character is a clear duplicate of the linked issue, which is behavior that was introduced in Visual Studio 2015. Our current understanding of the issue where Tab triggers a case insensitive snippet in C# code even when that snippet was removed from the snippet manager goes back indefinitely (potentially Visual Studio 2010 or earlier). If you can identify a specific release newer than these where the behavior on one of these issues changed, please do let us know since the age of a potentially-problematic behavior is a significant contributor to the prioritization.

levicki commented 12 months ago

@sharwell Just to be clear I am only responding because you @mentioned me, I have no desire to engage further with any of you.

Yes it was my bad to bring up Tab being replaced by spaces into this.

The only reason I did that was to highlight the inability to enter a Tab character at all after a trigger keyword, because of the combined effect of a snippet trigger AND conversion of pasted Tab character caused by the (known broken) formatting.

My understanding as a principal software engineer is that if some feature is disabled in the settings (in this case snippets, including the new snippet experience), and if the actual data (code snippets) that is needed for the feature to work is deleted from disk, then no amount of pressing the Tab key be it configured to insert Tabs or spaces should trigger the feature (snippet insertion), case-insensitive trigger matching or not.

Therefore I have to disagree with your assessment and root cause analysis of this issue — for me the primary bug is that a disabled feature is not actually disabled.

That's what I reported here, and that's what I as a customer expected the team to be able and willing to fix.

To my great astonishment, what you and Cyrus are saying is making my expectation sound totally unreasonable, and the problem vastly more complex than it actually is.

If you can identify a specific release newer than these where the behavior on one of these issues changed, please do let us know...

This request isn't clear to me. Do I need to install all Visual Studio versions going back to say Visual Studio 2012 and test where disabling snippets stopped actually disabling them or what? I am sorry, but you lost me there.

sharwell commented 12 months ago

To my great astonishment, what you and Cyrus are saying is making my expectation sound totally unreasonable, and the problem vastly more complex than it actually is.

I don't think the request itself is unreasonable, but we have to evaluate it the same way we evaluate all the other issues that get filed:

  1. How many users are impacted? (Larger values increase priority)
  2. How long has the current behavior been present? (Shorter values increase priority; larger values increase risk that users are depending on the current behavior and will complain if it changes)
  3. How complex/old is the code that needs to be rewritten in order to change the behavior? (Simpler values increase priority)

For the old snippet experience, this issue hits a worst-case scenario in our prioritization logic, where all three metrics suggest against making a change. For the new snippet experience, item (3) is less problematic so we might be able to resolve #68985 in the new experience.

levicki commented 12 months ago

users are depending on the current behavior and will complain if it changes

Are you really suggesting that someone might depend on a feature disable switch not actually fully disabling said feature? That someone has disabled snippets but they are ok with just this one random snippet still working? Because I am not buying that.

I really don't know the complexity of your code (I heard back in 2012 Visual Studio had ~60 million lines of code), but here is a greatly simplified way of describing this particular issue:

if (IncludeSnippetsSetting() != NeverIncludeSnippets) { // <- this doesn't work for me, fixing it shouldn't affect anyone else
    if (TabIsPressed()) {
        if (WordIsATrigger(Word, CaseInsensitiveMatch)) { // <- changing this breaks it for users relying on case-insensitive match
            InsertMatchingSnippet(Word, CaretPosition);
        }
    }
}

My rationale:

  1. Users either want snippets enabled or disabled. I guarantee you that nobody (well at least nobody sane who we should care about) wants them enabled when they select "Never include snippets" radio button in Settings. Never should mean never.
  2. Changing case-insensitive match to case-sensitive match might actually impact some users who rely on that behavior so I think that shouldn't be touched, especially because that's not what this ticket was about.

Pretty please, reconsider actually fixing the broken setting so that snippets get actually disabled when "Never use snippets" is selected. I don't care if that happens with old experience or new experience.

I actually totally don't care about snippets experience because I think it encourages copy-paste programming, enabling anyone to add blocks of code without actually understanding what those blocks do. All I care is about having an off switch for it and I really think that shouldn't be too much to ask.

CyrusNajmabadi commented 12 months ago

@levicki I think we could def consider a fix here, if it's localized and low risk. As per one of our prior conversations, would be happy to work with you on submitting a community contribution to fast track this. Otherwise, based on a few factors this would likely remain on backlog. Thanks! :-)

levicki commented 12 months ago

@CyrusNajmabadi Better on a backlog than closed.

sharwell commented 12 months ago

Users either want snippets enabled or disabled. I guarantee you that nobody (well at least nobody sane who we should care about) wants them enabled when they select "Never include snippets" radio button in Settings. Never should mean never.

There are (at least) three ways to invoke snippets:

  1. Via the completion list
  2. Via Edit → IntelliSense → Insert Snippet...
  3. By pressing Tab after the snippet shortcut identifier

The option "Never include snippets" only affects item (1). Since this has been around in the same form for many years, it's very likely there are multiple users who have come to rely on this behavior.

sharwell commented 12 months ago

We have a unit testing option defined for snippets: https://github.com/dotnet/roslyn/blob/7c6d5b92a584f0c512b51a0c44a9f441a9443f6f/src/EditorFeatures/Core/Snippets/SnippetsOptionsStorage.cs#L11

Currently it isn't bound to any storage location, but it would be pretty easy to write a Visual Studio extension that set the value for this option using reflection. We could also potentially give it a registry storage location so it could be set using the vsregedit command line tool.

levicki commented 12 months ago

The option "Never include snippets" only affects item (1)

🤦

When we are at snippet insertion methods, hijacking the Tab key which produces legitimate input character and already has a well defined function during text editing to insert snippets is akin to some websites hijacking Alt+D shortcut used to give focus to address bar for some other gizmo on their web page.

Furthermore, not allowing the user to disable this hijacking is equally horrible design in both cases.

Finally, suggesting writing a Visual Studio extension is on about the same level of "workaround" as having to install a Tampermonkey browser extension and write this Tampermonkey script to block said Alt+D hijacking in browser instead of having a proper option for it in the Settings:

// Super simple javascript code which every browser user
// annoyed with Alt+D hijacking should know how to write
let keycodes = [68];

document.addEventListener('keydown', function(e) {
    if (e.altKey && keycodes.indexOf(e.keyCode) != -1) {
        e.cancelBubble = true;
        e.stopImmediatePropagation();
    }
    return false;
});

it would be pretty easy to write a Visual Studio extension that set the value for this option using reflection.

Maybe to someone who knows the Visual Studio internals, certainly not easy for me (not impossible, just time-consuming because I'd have to learn how to write VS extensions, and then figure out how to change this).

We could also potentially give it a registry storage location so it could be set using the vsregedit command line tool.

That sounds like the best option to me if you already know that new snippet experience (the experimental one you mentioned) is not going to have a checkbox that exposes this setting in the UI.

I still hope that your team will eventually decide to sit down and consolidate old and new "snippet experience" such that all the controls are in one place in Settings and that the users can customize the insertion key combination, and fully enable or disable the feature.

DoctorKrolic commented 12 months ago

I agree that this is not a full duplicate of https://github.com/dotnet/roslyn/issues/24031. https://github.com/dotnet/roslyn/issues/68985 wouldn't cover this issue fully as well. Consider the follwing example:

class MyClass
{
    public int Class$$ { get; set; }
}

Place caret at $$ and press Tab. No matter whether you have semantic snippet experience enabled or not, such action triggers old-fashioned class snippet: devenv_yaK8fvJIob The incorrect behaviour here is that old-fashioned snippet inline completions are case-insensitive. It should trigger for class$$, but not for Class$$. As demonstrated by the gif, the latter isn't currently true. I think this issue needs to be reopened to track the problem I described. It is probably several-lines fix, although I am not sure whether this logic belongs to roslyn since snippets are not exclusive feature of C# editor

levicki commented 12 months ago

The incorrect behaviour here is that old-fashioned snippet inline completions are case-insensitive.

No, the incorrect behavior here is that the snippet completion cannot be fully disabled using Settings.

If you want to campaign for making it case-sensitive, that's what the issue #68985 is for.

I just want the option to fully disable snippets.

sharwell commented 12 months ago

The incorrect behaviour here is that old-fashioned snippet inline completions are case-insensitive.

This is hard-coded in the central editor code that affects all languages (implementation of IVsExpansionManager.GetExpansionByShortcut), and goes back at least to Visual Studio 2015 but probably much earlier since it's one of the very old COM interfaces. I don't see a viable path forward to change this for the old snippet engine, but it might be possible for the new snippet experience.

No, the incorrect behavior here is that the snippet completion cannot be fully disabled using Settings.

This has also always been the case. The editor does not provide a central way to disable the feature, and it appears to have fallback code to load default snippets that come with the installation if a user removes all user-specific snippets.

I searched our feedback backlog and was not able to find any other user requests to add the ability to disable snippets completely.

levicki commented 11 months ago

The editor does not provide a central way to disable the feature, and it appears to have fallback code to load default snippets that come with the installation if a user removes all user-specific snippets.

I wonder what is the purpose of these OnOff and Installed properties in this SnippetsIndex.xml file?

image

They seem either unused or incorrect, because I don't have those other folders in my install at all (I didn't install office development workload).

Just to confirm, I tried changing the OnOff and Installed values for the Visual C# (i.e. the first SnippetDir) which is present, but as expected it was not respecting either the OnOff state or Installed state as long as the folder itself was present on the disk. Once I renamed the actual folder from Visual C# to ~Visual C# it stopped inserting the class snippet.

I searched our feedback backlog and was not able to find any other user requests to add the ability to disable snippets completely.

That's probably because people thought that "Never include snippets" actually means what it says on the tin, and haven't hit the issue themselves. The fact that it's hard to differentiate what part of any given IntelliSense auto-completion is driven by snippets and what comes from other sources may have also contributed.

Is there a way for you to please create the request for proper snippet disable option internally on my behalf based on this issue without me having to submit new feedback from within Visual Studio?

In the meantime I will use the workaround of renaming the built-in snippet folder and hope that in some future VS update its absence won't cause the IDE to crash, and that someone won't include those snippets in a resource DLL required for the IDE to work which I then won't be able to remove.

Another thought that just occured to me is that you might want to have case-insensitive snippet matching for user-defined snippets, but not for language keywords. Instead of making all snippets case-sensitive perhaps this should be a per-snippet setting stored in an actual snippet file with default value being case-insensitive if the setting is missing (for backward compatibility).

CyrusNajmabadi commented 11 months ago

Is there a way for you to please create the request for proper snippet disable option internally on my behalf based on this issue without me having to submit new feedback from within Visual Studio?

I'd def recommend filing the feedback. The issue will be weighted more heavily coming from an external source.