Expand persister deserialization to also take into account string and accessibility modifiers

[Therzok]

CodeStyleOption supports more than just bool and enums, so look into expanding this in the roaming profile persister.

It can be seen here:

roslyn/src/Workspaces/Core/Portable/CodeStyle/CodeStyleOption.cs

Lines 151 to 152 in d6f483e

	case nameof(String):
	return v => (T)(object)v;

roslyn/src/Workspaces/Core/Portable/CodeStyle/CodeStyleOption.cs

Lines 76 to 79 in d6f483e

	if (typeof(T) == typeof(string))
	{
	return nameof(String);
	}

roslyn/src/Workspaces/Core/Portable/CodeStyle/CodeStyleOption.cs

Lines 56 to 59 in d6f483e

	if (typeof(T) == typeof(string))
	{
	return Value;
	}

This was discovered during porting option persisting to VSMac. We use a more flexible approach, but with reflection, due to the nature of how the serialization code is modeled:
https://github.com/mono/monodevelop/blob/bdc236c2faa86ecdd88d7f36c6294b028a92477e/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.RoslynServices.Options/RoslynPreferences.Properties.cs#L109-L129

Side note 2

It also seems you're not doing anything in case the types don't match, either report that or clear the stored setting:

• https://github.com/dotnet/roslyn/blob/master/src/VisualStudio/Core/Def/Implementation/Options/RoamingVisualStudioProfileOptionPersister.cs#L176

Side note:

I think the current CodeStyleOption and NamingPreferences (de)serializers can be improved a bit, but for now, this patch works.

3 things at hand:

• If the approach is kept to whitelist specific variants of CodeStyleOption, a test should be added to ensure all possible Ts are handled (maybe just run through all the options that go in the persister

• I think the approach is not scalable, since anyone can provide their own T (i.e. enum) for CodeStyleOption<T>

• Only the persister works with FromXElement.

So, there could be some better way to model this. Can we make FromXElement just return ICodeStyleOption and use reflection to get the required T and construct the CodeStyleOption<T>? That way, every other possible type for T (i.e. custom enum) can be handled, rather than whitelisting on a case-by-case basis.

Ask Mode template not completed

Customer scenario

Some options might not get persisted even if surfaced int he UI.

Bugs this fixes

N/A

Workarounds, if any

None, really.

Risk

Someone can modify the user preference in the options UI and not see it persisted.

Performance impact

Don't think there's any

Is this a regression from a previous update?

Don't think so.

Root cause analysis

See side-note.

How was the bug found?

Porting over option persister in VSMac.

Test documentation updated?

None

[Therzok]

Possible values found via:

➜  roslyn git:(sqlite-blob) ag --nofilename --nobreak -o "CodeStyleOption<.*?>" | sort | uniq -c
  14 CodeStyleOption<AccessibilityModifiersRequired>
  44 CodeStyleOption<ExpressionBodyPreference>
  33 CodeStyleOption<ParenthesesPreference>
   1 CodeStyleOption<SomeEnumType>
  62 CodeStyleOption<T>
 212 CodeStyleOption<bool>
  14 CodeStyleOption<string>

Then looking if any defines a roaming profile persister

[Therzok]

Can I get a buddy on this?

[KirillOsenkov]

@heejaechang @jinujoseph

[heejaechang]

@dpoeschl did code style options. @jasonmalinowski did option persister.

[jasonmalinowski]

@jinujoseph Delegated M2 approval will be needed.

@Therzok If you want to give a crack at fixing this better (not special casing), I'd be for that. It's funny that we can serialize all of them in one way but not deserialize them. :-/

[Therzok]

I can. So, 2 tasks to improve this PR:

1. Log a non-fatal error when deserialization fails (and clear value? Not sure)
2. Make the deserialization code work on any type.

[jasonmalinowski]

I'd skip 1 for now (or do that independently) -- I could totally imagine right now we have some users with bad roamed profiles that we'd suddenly see a spike of non-fatal errors being reported. It might be a good thing to just have that change separate so if we have to roll that reporting back we can do so independently of your bug fix.

[jinujoseph]

Will wait for 2'nd task to be addressed.

[Neme12]

Note: I would recommend pulling latest master branch before you start working on the additional changes so that you don't have conflicts with my changes to RoamingVisualStudioProfileOptionPersister in #26165 (commit d9262c2)

[Therzok]

Rebased and modified the code.

So, there were a few choices at hand, each with its drawbacks, and this seems to be the simplest and best way to handle it, so a few comments below:

1. RoamingVisualStudioProfileOptionPersister has no generically typed information, so we are forced to have a reflection-based way to create a new object of the type of the option if we want to support all options - manual checks seem to be the only way to avoid using reflection.

2. CodeStyleOption is public API, so it's not really possible to change. ToXElement/FromXElement are also public, as opposed to NamingStylePreferences counterparts which are internal.

3. Even if there was a completely generic way to do it based on the T:new() constraint, it'll still be reflection based via activator.

4. Not sure if the gains of a compiled lambda outweigh the costs, since each option is fetched only once (some might be reused more).

5. Moving the serialization/deserialization code out of the persister into a centralized place does not fix the general case enum issue, since some still can be missed.

[jasonmalinowski]

Ah, reflection. It's icky, but hey it's small at least.

[jasonmalinowski]

@dpoeschl want to take a quick look at this as well?

[jasonmalinowski]

@jinujoseph Second task is done, so this would be ready for delegated M2 approval.

[jinujoseph]

Approved to merge for 15.8.PReview3 once tests are green

[jinujoseph]

test windows_debug_spanish_unit32_prtest

[jinujoseph]

test windows_release_vs-integration_prtest