Fix reading indent style and indent size from VS settings #65382

tmat · 2022-11-11T23:32:50Z

Turns out ISettingsManager stores indent size as Int64 which wasn't converted to Int32.
We were also missing storage location for Indent Style.

Fixes #65375 (https://developercommunity.visualstudio.com/t/Quick-actions-use-wrong-indent/10197217)

Fixes AB#1674009
Fixes AB#1675408

Fixes #65360

Fixes regression introduced by #62684

tmat · 2022-11-11T23:36:29Z

@CyrusNajmabadi @jasonmalinowski @arkalyanms

jasonmalinowski

Thanks for the explanation in the PR description!

Youssef1313 · 2022-11-12T11:17:41Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/FormattingOptions2.cs

+        public static PerLanguageOption2<IndentStyle> SmartIndent { get; } =
+            new(FeatureName, FormattingOptionGroups.IndentationAndSpacing, nameof(SmartIndent), defaultValue: IndentationOptions.DefaultIndentStyle,
+                new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Indent Style"));


I don't understand how roaming storage location is useful here?
This doesn't seem to be exposed in Tools → Options, and it's not in AutomationObject.

So its value will always remain DefaultIndentStyle and it is never fetched/persisted through import/export.

Anything I'm missing? I'm curious to know the user-visible effect of this change if you noticed any.

The option is read from global options here: https://sourceroslyn.io/#Microsoft.CodeAnalysis.LanguageServer.Protocol/Features/Options/IndentationOptionsStorage.cs,21

Global options end up calling VisualStudioSettingsOptionPersister to read the option from VS settings, but that only happens if it has the storage.

The option is owned by the VS platform, not Roslyn. Roslyn does not set it nor has any UI that exposes it. It just needs to read it.

Thanks @tmat @arkalyanms for clarifying.

Youssef1313

Does this PR fix #65360 ? If so, can you modify the description to include "Fixes #65360" so the issue gets auto closed?

Youssef1313 · 2022-11-12T11:24:03Z

src/VisualStudio/Core/Def/Options/VisualStudioSettingsOptionPersister.cs

+                if (optionKey.Option.Type == typeof(int) &&
+                    _settingManager.TryGetValue(storageKey, out int intValue) == GetValueResult.Success)
+                {
+                    return intValue;
+                }


Should we instead update the following line to match the type stored in _settingsManager?

roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/FormattingOptions2.cs

Line 55 in f814f00

EditorConfigStorageLocation.ForInt32Option("indent_size"),

Edit: This change might not be necessary with the suggestion in #65382 (review)

I don't think so. The fact that the option is stored as Int64 in VS setting is a VS-specific quirk. Quite odd, since there is no reason for it to be 64 bit.

@tmat Then what about the more generalized approach suggested in #65382 (review)?

This will ensure that no such issues exist whatever the type of option is.

Will follow up on that later on: #65394

Youssef1313

Also I think we could adjust the following:

roslyn/src/VisualStudio/Core/Def/Options/VisualStudioSettingsOptionPersister.cs

Lines 205 to 210 in f814f00

    
           else if (value != null && optionKey.Option.Type != value.GetType()) 
        
           { 
        
               // We got something back different than we expected, so fail to deserialize 
        
               value = null; 
        
               return false; 
        
           }

We can do it like this:

try
{
    value = Convert.ChangeType(value, optionKey.Option.Type);
}
catch
{
    value = null; 
    return false; 
}

This basically says if we got different types, try to convert it. Can we convert? Then everything is good. Failed to convert? Nothing we can do to read the option.

arkalyanms · 2022-11-12T17:52:49Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/FormattingOptions2.cs

+        public static PerLanguageOption2<IndentStyle> SmartIndent { get; } =
+            new(FeatureName, FormattingOptionGroups.IndentationAndSpacing, nameof(SmartIndent), defaultValue: IndentationOptions.DefaultIndentStyle,
+                new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Indent Style"));


arkalyanms · 2022-11-12T17:56:09Z

@tmat Lets park it here until we get soft approval from QB. Please fill in the ask mode template at https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1675408

arkalyanms · 2022-11-16T00:21:48Z

Merging based on QB response over email. This should be taken when 17.4.2 opens up.

cc: @jasonmalinowski

arkalyanms · 2022-11-16T00:24:22Z

@Cosifne should we consider backporting the integration test fixes to 17.5 and enabling the check since we are taking servicing fixes? Might be worthwhile discussing in team meeting.

sharwell · 2022-11-16T14:47:13Z

@tmat do we know how this worked prior to 17.4?

Youssef1313 · 2022-11-16T15:03:10Z

@sharwell Maybe because of the deletion of https://github.com/dotnet/roslyn/blob/da6f98ea75b91293a16ed2f75ac0d7bae767c4fd/src/VisualStudio/Core/Def/Options/LanguageSettingsPersister.cs in 51cc7c9

I'm not sure though how Visual Studio is interacting with these options, so @tmat can say for sure.

In the deleted class, I also see we were initializing settings for TS, F#, and Xaml. I don't know how these are supposed to work today with the current implementation.

tmat · 2022-11-16T17:22:33Z

Yup, we used a legacy VS interface to read the options.

Youssef1313 · 2022-11-16T17:25:47Z

@tmat What about TS, F#, and Xaml? How do they work now? Asking specifically about these line which don't exist now:

roslyn/src/VisualStudio/Core/Def/Options/LanguageSettingsPersister.cs

Lines 64 to 66 in da6f98e

    
           InitializeSettingsForLanguage(InternalLanguageNames.TypeScript, new Guid("4a0dddb5-7a95-4fbf-97cc-616d07737a77")); 
        
           InitializeSettingsForLanguage("F#", new Guid("BC6DD5A5-D4D6-4dab-A00D-A51242DBAF1B")); 
        
           InitializeSettingsForLanguage("Xaml", new Guid("CD53C9A1-6BC2-412B-BE36-CC715ED8DD41"));

tmat · 2022-11-16T17:52:20Z

When they call Roslyn to perform an operation that needs these options the operation reads them from global options, which works the same way as C#/VB.

Fix reading indent style and indent size from VS settings

394d527

tmat requested a review from a team as a code owner November 11, 2022 23:32

dotnet-issue-labeler bot added the Area-IDE label Nov 11, 2022

tmat changed the base branch from main to release/dev17.4 November 11, 2022 23:36

CyrusNajmabadi approved these changes Nov 11, 2022

View reviewed changes

jasonmalinowski approved these changes Nov 11, 2022

View reviewed changes

Youssef1313 reviewed Nov 12, 2022

View reviewed changes

arkalyanms approved these changes Nov 12, 2022

View reviewed changes

arkalyanms added the OnDeckForServicing label Nov 12, 2022

This was referenced Nov 12, 2022

Cover VisualStudioSettingsOptionPersister with tests #65394

Open

Fix reading indent style and indent size from VS settings #65420

Merged

Visual Studio 2022 formatting issue #65359

Closed

arkalyanms merged commit 5309168 into dotnet:release/dev17.4 Nov 16, 2022

tmat deleted the IndentFix branch November 16, 2022 17:22

arkalyanms removed the OnDeckForServicing label Nov 21, 2022

allisonchou mentioned this pull request Nov 22, 2022

[Automated] PRs inserted in VS build main-33121.346 #65554

Closed

tmat added this to the 17.4 milestone Nov 22, 2022

jasonmalinowski mentioned this pull request Nov 28, 2022

Incorrect formatting behaviour when using SyntaxFactory.Block #63013

Open

Axemagus mentioned this pull request Nov 30, 2022

Codemaid Cleanup causing indentation changes codecadwallader/codemaid#958

Closed

Youssef1313 mentioned this pull request Dec 20, 2022

Visual Basic: "Object initialization can be simplified (IDE0017)" wrong indent with custom tab/indent size #66071

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix reading indent style and indent size from VS settings #65382

Fix reading indent style and indent size from VS settings #65382

tmat commented Nov 11, 2022 •

edited

Loading

tmat commented Nov 11, 2022

jasonmalinowski left a comment

Youssef1313 Nov 12, 2022 •

edited

Loading

tmat Nov 12, 2022 •

edited

Loading

arkalyanms Nov 12, 2022

Youssef1313 Nov 12, 2022

Youssef1313 left a comment

Youssef1313 Nov 12, 2022 •

edited

Loading

tmat Nov 12, 2022 •

edited

Loading

Youssef1313 Nov 12, 2022

tmat Nov 12, 2022

Youssef1313 left a comment •

edited

Loading

arkalyanms Nov 12, 2022

arkalyanms commented Nov 12, 2022

arkalyanms commented Nov 16, 2022 •

edited

Loading

arkalyanms commented Nov 16, 2022

sharwell commented Nov 16, 2022

Youssef1313 commented Nov 16, 2022

tmat commented Nov 16, 2022

Youssef1313 commented Nov 16, 2022 •

edited

Loading

tmat commented Nov 16, 2022

	else if (value != null && optionKey.Option.Type != value.GetType())
	{
	// We got something back different than we expected, so fail to deserialize
	value = null;
	return false;
	}

Fix reading indent style and indent size from VS settings #65382

Fix reading indent style and indent size from VS settings #65382

Conversation

tmat commented Nov 11, 2022 • edited Loading

tmat commented Nov 11, 2022

jasonmalinowski left a comment

Choose a reason for hiding this comment

Youssef1313 Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

tmat Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

arkalyanms Nov 12, 2022

Choose a reason for hiding this comment

Youssef1313 Nov 12, 2022

Choose a reason for hiding this comment

Youssef1313 left a comment

Choose a reason for hiding this comment

Youssef1313 Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

tmat Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

Youssef1313 Nov 12, 2022

Choose a reason for hiding this comment

tmat Nov 12, 2022

Choose a reason for hiding this comment

Youssef1313 left a comment • edited Loading

Choose a reason for hiding this comment

arkalyanms Nov 12, 2022

Choose a reason for hiding this comment

arkalyanms commented Nov 12, 2022

arkalyanms commented Nov 16, 2022 • edited Loading

arkalyanms commented Nov 16, 2022

sharwell commented Nov 16, 2022

Youssef1313 commented Nov 16, 2022

tmat commented Nov 16, 2022

Youssef1313 commented Nov 16, 2022 • edited Loading

tmat commented Nov 16, 2022

tmat commented Nov 11, 2022 •

edited

Loading

Youssef1313 Nov 12, 2022 •

edited

Loading

tmat Nov 12, 2022 •

edited

Loading

Youssef1313 Nov 12, 2022 •

edited

Loading

tmat Nov 12, 2022 •

edited

Loading

Youssef1313 left a comment •

edited

Loading

arkalyanms commented Nov 16, 2022 •

edited

Loading

Youssef1313 commented Nov 16, 2022 •

edited

Loading