-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dependency on Type.IsSerializable from OptionSet serialization #54990
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,57 +215,28 @@ public void Serialize(ObjectWriter writer, CancellationToken cancellationToken) | |
Debug.Assert(ShouldSerialize(optionKey)); | ||
|
||
if (!_serializableOptions.Contains(optionKey.Option)) | ||
{ | ||
continue; | ||
} | ||
|
||
var kind = OptionValueKind.Null; | ||
object? valueToWrite = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to specially handle 'null', the default case below handles it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, no need for 'valueToWrite'. we always just write out the literal value. |
||
if (value != null) | ||
OptionValueKind kind; | ||
switch (value) | ||
{ | ||
switch (value) | ||
{ | ||
case ICodeStyleOption codeStyleOption: | ||
if (optionKey.Option.Type.GenericTypeArguments.Length != 1) | ||
{ | ||
continue; | ||
} | ||
|
||
kind = OptionValueKind.CodeStyleOption; | ||
valueToWrite = codeStyleOption; | ||
break; | ||
|
||
case NamingStylePreferences stylePreferences: | ||
kind = OptionValueKind.NamingStylePreferences; | ||
valueToWrite = stylePreferences; | ||
break; | ||
|
||
case string str: | ||
kind = OptionValueKind.String; | ||
valueToWrite = str; | ||
break; | ||
|
||
default: | ||
var type = value.GetType(); | ||
if (type.IsEnum) | ||
{ | ||
kind = OptionValueKind.Enum; | ||
valueToWrite = (int)value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this cast isn't necessary here due to teh line where we actually write the enum doing this already. |
||
break; | ||
} | ||
|
||
if (optionKey.Option.Type.IsSerializable) | ||
{ | ||
kind = OptionValueKind.Serializable; | ||
valueToWrite = value; | ||
break; | ||
} | ||
|
||
case ICodeStyleOption: | ||
if (optionKey.Option.Type.GenericTypeArguments.Length != 1) | ||
continue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We just ignore options without single generic type param? Shouldn't this throw or something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i am adding this to the list of things to try out in my followup PR :) but yes. seems insane. |
||
} | ||
|
||
kind = OptionValueKind.CodeStyleOption; | ||
break; | ||
|
||
case NamingStylePreferences: | ||
kind = OptionValueKind.NamingStylePreferences; | ||
break; | ||
|
||
default: | ||
kind = value != null && value.GetType().IsEnum ? OptionValueKind.Enum : OptionValueKind.Object; | ||
break; | ||
} | ||
|
||
valuesBuilder.Add(optionKey, (kind, valueToWrite)); | ||
valuesBuilder.Add(optionKey, (kind, value)); | ||
} | ||
|
||
writer.WriteInt32(valuesBuilder.Count); | ||
|
@@ -279,7 +250,7 @@ public void Serialize(ObjectWriter writer, CancellationToken cancellationToken) | |
RoslynDebug.Assert(value != null); | ||
writer.WriteInt32((int)value); | ||
} | ||
else if (kind == OptionValueKind.CodeStyleOption || kind == OptionValueKind.NamingStylePreferences) | ||
else if (kind is OptionValueKind.CodeStyleOption or OptionValueKind.NamingStylePreferences) | ||
{ | ||
RoslynDebug.Assert(value != null); | ||
((IObjectWritable)value).WriteTo(writer); | ||
|
@@ -292,9 +263,7 @@ public void Serialize(ObjectWriter writer, CancellationToken cancellationToken) | |
|
||
writer.WriteInt32(_changedOptionKeysSerializable.Count); | ||
foreach (var changedKey in _changedOptionKeysSerializable.OrderBy(OptionKeyComparer.Instance)) | ||
{ | ||
SerializeOptionKey(changedKey); | ||
} | ||
|
||
return; | ||
|
||
|
@@ -342,9 +311,7 @@ public static SerializableOptionSet Deserialize(ObjectReader reader, IOptionServ | |
for (var i = 0; i < count; i++) | ||
{ | ||
if (!TryDeserializeOptionKey(reader, lookup, out var optionKey)) | ||
{ | ||
continue; | ||
} | ||
|
||
var kind = (OptionValueKind)reader.ReadInt32(); | ||
var readValue = kind switch | ||
|
@@ -356,16 +323,13 @@ public static SerializableOptionSet Deserialize(ObjectReader reader, IOptionServ | |
}; | ||
|
||
if (!serializableOptions.Contains(optionKey.Option)) | ||
{ | ||
continue; | ||
} | ||
|
||
object? optionValue; | ||
switch (kind) | ||
{ | ||
case OptionValueKind.CodeStyleOption: | ||
var defaultValue = optionKey.Option.DefaultValue as ICodeStyleOption; | ||
if (defaultValue == null || | ||
if (optionKey.Option.DefaultValue is not ICodeStyleOption defaultValue || | ||
optionKey.Option.Type.GenericTypeArguments.Length != 1) | ||
{ | ||
continue; | ||
|
@@ -386,10 +350,6 @@ public static SerializableOptionSet Deserialize(ObjectReader reader, IOptionServ | |
optionValue = Enum.ToObject(optionKey.Option.Type, readValue); | ||
break; | ||
|
||
case OptionValueKind.Null: | ||
optionValue = null; | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for this. already handled by the object reading code. |
||
|
||
default: | ||
optionValue = readValue; | ||
break; | ||
|
@@ -403,16 +363,15 @@ public static SerializableOptionSet Deserialize(ObjectReader reader, IOptionServ | |
for (var i = 0; i < count; i++) | ||
{ | ||
if (TryDeserializeOptionKey(reader, lookup, out var optionKey)) | ||
{ | ||
changedKeysBuilder.Add(optionKey); | ||
} | ||
} | ||
|
||
var serializableOptionValues = builder.ToImmutable(); | ||
var changedOptionKeysSerializable = changedKeysBuilder.ToImmutable(); | ||
var workspaceOptionSet = new WorkspaceOptionSet(optionService); | ||
|
||
return new SerializableOptionSet(languages, workspaceOptionSet, serializableOptions, serializableOptionValues, | ||
return new SerializableOptionSet( | ||
languages, workspaceOptionSet, serializableOptions, serializableOptionValues, | ||
changedOptionKeysSerializable, changedOptionKeysNonSerializable: ImmutableHashSet<OptionKey>.Empty); | ||
|
||
static bool TryDeserializeOptionKey(ObjectReader reader, ILookup<string, IOption> lookup, out OptionKey deserializedOptionKey) | ||
|
@@ -439,11 +398,9 @@ static bool TryDeserializeOptionKey(ObjectReader reader, ILookup<string, IOption | |
|
||
private enum OptionValueKind | ||
{ | ||
Null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Null and String and Serializable were merged into a single item called 'Object' |
||
CodeStyleOption, | ||
NamingStylePreferences, | ||
Serializable, | ||
String, | ||
Object, | ||
Enum | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should view with whitespace off.