Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Reflection: Fix FormatException when querying ParameterInfo.DefaultValue of optional DateTime parameter#17877

Merged
1 commit merged into
dotnet:masterfrom
stakx:issue-corefx-26164
May 7, 2018
Merged

Reflection: Fix FormatException when querying ParameterInfo.DefaultValue of optional DateTime parameter#17877
1 commit merged into
dotnet:masterfrom
stakx:issue-corefx-26164

Conversation

@stakx

@stakx stakx commented May 3, 2018

Copy link
Copy Markdown

Summary:

Fixes a reflection bug (as discussed in https://github.com/dotnet/corefx/issues/26164) which causes a FormatException when calling ParameterInfo.DefaultValue for an optional DateTime parameter having an encoded default value of null (which is how the Roslyn C# compiler encodes default(DateTime)).

Example of the bug being fixed:

The following C# program currently throws an exception:

class Program
{
    public void Method(System.DateTime arg = default(System.DateTime)) { }

    static void Main()
    {
        var argDefaultValue = typeof(Program).GetMethod("Method").GetParameters()[0].DefaultValue;
        System.Diagnostics.Debug.Assert(object.Equals(null, argDefaultValue));
    }
}
Unhandled Exception: System.FormatException: Encountered an invalid type for a default value.
   at System.Reflection.MdConstant.GetValue(MetadataImport scope, Int32 token, RuntimeTypeHandle fieldTypeHandle, Boolean raw)
   at System.Reflection.RuntimeParameterInfo.GetDefaultValueInternal(Boolean raw)
   at System.Reflection.RuntimeParameterInfo.GetDefaultValue(Boolean raw)
   at Program.Main()

This PR makes the above program succeed.

Some additional notes:

  • Reflection reporting a default value null when it's default(DateTime) in source code may seem surprising, but is in line with reflection's behavior for how C# encodes other default(<any value type>).

  • I'm including a test project tests/src/reflection/OptionalParameters/DefaultValue.csproj. tests/src/reflection/OptionalParameters/DefaultValue.ilproj, however it doesn't currently get built automatically (see Refine test discovery and partition during test build with new target #17331). I've run it manually to verify this bugfix.

@stakx stakx force-pushed the issue-corefx-26164 branch from 30becff to a781fe5 Compare May 3, 2018 21:23
@ghost

ghost commented May 4, 2018

Copy link
Copy Markdown

I'm including a test project tests/src/reflection/OptionalParameters/DefaultValue.ilproj

could it be a csproj, with the example you posted above? my understanding is that usually things are tested with ilproj, which otherwise can't be test with c# grammar?

@stakx

stakx commented May 4, 2018

Copy link
Copy Markdown
Author

@kasper3: A .csproj would work just as well, but it would make the test dependent on an implementation detail of the Roslyn compilers (i.e. that they encode default(TValueType) default parameter values as a null constant).

I'm happy to change the tests to a .csproj.

@stakx

stakx commented May 5, 2018

Copy link
Copy Markdown
Author

@atsushikan - Could you please review this?

(I am also submitting additional tests for this to CoreFX. Shall I remove the test project here completely?)

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As with the other one, we shouldn't include the test in coreclr, just corefx. Otherwise, LGTM

Querying the default value of an optional `DateTime` parameter using
`ParameterInfo.DefaultValue` can throw a `FormatException` if that
default value is a null constant in metadata, which is how the C#
compiler encodes a default value of `default(DateTime)`.

This commit fixes that error by adding the proper handling for null
metadata constants with `DateTime` parameters.
@stakx stakx force-pushed the issue-corefx-26164 branch from 08758ca to a063e1a Compare May 7, 2018 16:10
@stakx

stakx commented May 7, 2018

Copy link
Copy Markdown
Author

@atsushikan - removed all test code & rebased.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants