Skip to content

(#12430) Improve detection of mutable value types #12495

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.

Sign up for GitHub

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

Merged
rjmholt merged 3 commits into from
Dec 10, 2020
Merged

(#12430) Improve detection of mutable value types #12495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec25fd6
:wrench: Improve detection of mutable value types
vexx32 May 1, 2020
4460672
:recycle: Address Rob's feedback
vexx32 Aug 8, 2020
e235d87
:bug: Resolve issue in history input handling
vexx32 Aug 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 11 additions & 88 deletions src/System.Management.Automation/engine/hostifaces/History.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1436,6 +1436,7 @@ void ProcessRecord()
{
break;
}

// Read CommandLine property
string commandLine = GetPropertyValue(mshObject, "CommandLine") as string;
if (commandLine == null)
Expand All @@ -1445,109 +1446,32 @@ void ProcessRecord()

// Read ExecutionStatus property
object pipelineState = GetPropertyValue(mshObject, "ExecutionStatus");
if (pipelineState == null)
{
break;
}

PipelineState executionStatus;
if (pipelineState is PipelineState)
{
executionStatus = (PipelineState)pipelineState;
}
else if (pipelineState is PSObject)
{
PSObject serializedPipelineState = pipelineState as PSObject;
object baseObject = serializedPipelineState.BaseObject;
if (baseObject is not int)
{
break;
}

executionStatus = (PipelineState)baseObject;
if (executionStatus < PipelineState.NotStarted || executionStatus > PipelineState.Failed)
{
break;
}
}
else if (pipelineState is string)
{
try
{
executionStatus = (PipelineState)Enum.Parse(typeof(PipelineState), (string)pipelineState);
}
catch (ArgumentException)
{
break;
}
}
else
if (pipelineState == null || !LanguagePrimitives.TryConvertTo<PipelineState>(pipelineState, out PipelineState executionStatus))
{
break;
}

// Read StartExecutionTime property
DateTime startExecutionTime;
object temp = GetPropertyValue(mshObject, "StartExecutionTime");
if (temp == null)
{
break;
}
else if (temp is DateTime)
{
startExecutionTime = (DateTime)temp;
}
else if (temp is string)
{
try
{
startExecutionTime = DateTime.Parse((string)temp, System.Globalization.CultureInfo.CurrentCulture);
}
catch (FormatException)
{
break;
}
}
else
if (temp == null || !LanguagePrimitives.TryConvertTo<DateTime>(temp, out DateTime startExecutionTime))
{
break;
}

// Read EndExecutionTime property
DateTime endExecutionTime;
temp = GetPropertyValue(mshObject, "EndExecutionTime");
if (temp == null)
{
break;
}
else if (temp is DateTime)
{
endExecutionTime = (DateTime)temp;
}
else if (temp is string)
{
try
{
endExecutionTime = DateTime.Parse((string)temp, System.Globalization.CultureInfo.CurrentCulture);
}
catch (FormatException)
{
break;
}
}
else
if (temp == null || !LanguagePrimitives.TryConvertTo<DateTime>(temp, out DateTime endExecutionTime))
{
break;
}

return new HistoryInfo
(
0,
commandLine,
executionStatus,
startExecutionTime,
endExecutionTime
);
return new HistoryInfo(
pipelineId: 0,
commandLine,
executionStatus,
startExecutionTime,
endExecutionTime
);
} while (false);

// If we are here, an error has occured.
Expand Down Expand Up @@ -1988,4 +1912,3 @@ private void ClearHistoryEntries(long id, int count, string cmdline, SwitchParam
#endregion Private
}
}

37 changes: 20 additions & 17 deletions src/System.Management.Automation/engine/runtime/Binding/Binders.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2030,36 +2030,39 @@ private static object NullRule(CallSite site, object obj)

internal static bool IsValueTypeMutable(Type type)
{
if (type.IsPrimitive || type.IsEnum)
// First, check for enums/primitives and compiler-defined attributes.
if (type.IsPrimitive
|| type.IsEnum
|| type.IsDefined(typeof(System.Runtime.CompilerServices.IsReadOnlyAttribute), inherit: false))
{
return false;
}

// If there are any fields, the type is mutable.
if (type.GetFields(BindingFlags.Public | BindingFlags.Instance).Length > 0)
// If the builtin attribute is not present, check for a custom attribute from by the compiler. If the
// library targets netstandard2.0, the compiler can't be sure the attribute will be provided by the runtime,
// and defines its own attribute of the same name during compilation. To account for this, we must check the
// type by name, not by reference.
foreach (object attribute in type.GetCustomAttributes(inherit: false))
{
return true;
if (attribute.GetType().FullName.Equals(
"System.Runtime.CompilerServices.IsReadOnlyAttribute",
StringComparison.Ordinal))
Comment on lines +2047 to +2049
Copy link
Collaborator

@rjmholt rjmholt Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for needing to do a type name comparison rather than a simple type comparison here?

Copy link
Collaborator

@SeeminglyScience SeeminglyScience Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for needing to do a type name comparison rather than a simple type comparison here?

@rjmholt if you target netstandard2.0 and use readonly structs then Roslyn embeds the attribute definition in the assembly, so type identity will only be reliable for assemblies targeting netcoreapp3.0+ (approximately, don't remember when that was introduced).

Copy link
Collaborator

@rjmholt rjmholt Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok got it -- might be worth a comment so that someone doesn't "optimise" it

Copy link
Collaborator Author

@vexx32 vexx32 Aug 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already added comments for this just above (2040-2043) - should I move those closer to this line/reword them a bit?

Copy link
Collaborator

@rjmholt rjmholt Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I would just add something like "so we must check for the type by name rather than by reference" after the bit about the compiler

{
return false;
}
}

// If there are any properties with setters, the type is mutable.
var properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);
for (int index = 0; index < properties.Length; index++)
// Fallback: check all fields (public + private) to verify whether they're all readonly.
// If any field is not readonly, the value type is potentially mutable.
foreach (var field in type.GetFields(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic))
{
var property = properties[index];
if (property.CanWrite)
if (!field.IsInitOnly)
{
return true;
}
}

// If there are any methods other than the property getters, the type might
// be mutable, so assume the type is mutable.
var methods = type.GetMethods(BindingFlags.Public | BindingFlags.Instance);
if (methods.Length != properties.Length)
{
return true;
}

// If all fields are init-only (read-only), then the value type is immutable.
return false;
}

Expand Down