Skip to content

Add back ability to reproduce default parameter values#356

Merged
jonorossi merged 5 commits intocastleproject:masterfrom
stakx:default-value-replication
May 15, 2018
Merged

Add back ability to reproduce default parameter values#356
jonorossi merged 5 commits intocastleproject:masterfrom
stakx:default-value-replication

Conversation

@stakx
Copy link
Member

@stakx stakx commented May 6, 2018

@jonorossi - This is in response to #291 (comment) and low-priority. If you've changed your mind since January and would rather not merge this, that's no problem.

Reproduction of default parameter values has caused a lot of trouble in the past and was eventually (in #149) disabled completely, because ParameterBuilder.SetConstant was deemed too buggy. At that time, it wasn't clearly understood what bugs exactly caused the problems.

I've spent some time discovering and documenting the relevant bugs that are present in current versions of the CLR, CoreCLR, and on Mono. This commits adds back the ability to reproduce default parameter values, but this time with more precise error detection.

This means that on recent runtimes, default parameter values will stand a good chance of being copied much more faithfully.

⚠️ I've tested this only on .NET 3.5, .NET 4.7.1, .NET Core 1.1, .NET Core 2.0, a private (dev) build of .NET Core 2.1, and Mono 5.10.1.47. It is possible that on older runtime versions (esp. of Mono and .NET 4-4.7.0) , this may cause bugs if merged. Testing for older versions isn't always easy: For instance, I can't test locally for e.g. .NET 4.0 since 4.x versions are in-place updates and I have 4.7.1.

This is work in progress and shouldn't be considered for merging until the following have been ticked off:

  • Systematically add unit tests that exercise default value reproduction for any and all possible types.
  • Remove or rewrite existing unit tests that are covered by the above, but were based on a less precise understanding of the framework bugs.
  • Add a user documentation page explaining why default values aren't always reproduced correctly (see it rendered in my fork)

/cc #35 + #36, #45, #87 + #91, #141 + #149, #291

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants