Add back ability to reproduce default parameter values #356

stakx · 2018-05-06T11:44:16Z

@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

stakx

Found another bug corner case that needs to be taken into account.

stakx · 2018-05-08T15:52:17Z

src/Castle.Core/DynamicProxy/Generators/Emitters/MethodEmitter.cs

+			{
+				defaultValue = from.DefaultValue;
+			}
+			catch (FormatException) when (from.ParameterType == typeof(DateTime))


.DefaultValue may also throw a FormatException for (closed generic) enum parameter types; see https://github.com/dotnet/corefx/issues/29570. Another catch dedicated for this issue might be necessary here.

stakx · 2018-05-08T15:53:21Z

docs/dynamicproxy-default-parameter-values.md

+
+   You can only find out the correct default value by double-checking the default value in the original method of the proxied type.
+
+   For .NET Core, the underlying cause of this problem has been documented in [dotnet/coreclr#17893](https://github.com/dotnet/coreclr/issues/17893).


Add another bullet point for the (closed generic) enum case mentioned in the other review comment.

jonorossi · 2018-05-13T17:02:39Z

@stakx amazing work here, especially with the runtime teams! It appears you've covered all bases, but I'll have another look tomorrow at a sensible hour.

stakx · 2018-05-13T17:12:01Z

src/Castle.Core.Tests/ExcludeOnClrAttribute.cs

+
+		public void ApplyToTest(Test test)
+		{
+			if (test.RunState != RunState.NotRunnable && test.RunState != RunState.Skipped && !IsRunningOnMono())


This is actually a bit hacky. Just because we're not running on Mono doesn't mean that we're running on the CLR. This is good enough for the current build setup, but not ideal.

Also, now that the CoreCLR has merged fixes for all identified limitations and defects regarding default parameter value reproduction, it'd be nice to be able to distinguish between the CLR and CoreCLR with two attributes, [ExcludeOnClr] and [ExcludeOnCoreClr]. I didn't want to inflate testing infrastructure too much for now.

Oh yer, I saw that and thought this needs to be better, will give it some thought tomorrow.

Just discovered RuntimeInformation.FrameworkDescription, which appears to work on .NET (*), .NET Core, and Mono. How about reimplementing a single [ExcludeOnRuntime(<runtime id>)] based on that? In a first iteration, it would perhaps be version-agnostic, that could still be added later on.

Apart from that, BenchmarkDotNet might be another good place to find runtime / OS detection logic.

And finally, there's always the option of dropping all runtime / OS detection magic and having the required parameters passed in from the build script (but that seems tricky for a couple reasons).

*) RuntimeInformation isn't around on .NET 4.6.1 but we could either upgrade the test projects to .NET 4.7.1, or find installed .NET Framework versions using a registry query.

I agree that it would be preferred to do at runtime rather than pass a parameter into the runner so we don't run into problems with using different test runners.

BenchmarkDotNet treats "CLR" as just the full/desktop CLR, and not also the CoreCLR. A single attribute looks like a good option as we might want to enable some on the CoreCLR once things get fixed.

Regarding the attribute name though, should it be "runtime" or "framework"? For example, the runtime is CoreCLR but heaps of the code we depend on is in CoreFX, so in general the framework is .NET Core? If we go with "runtime" we could probably do something like this, maybe even drop "runtime" from the attribute name as you'd always have the enum:

[ExcludeOnRuntime(Runtime.Mono)] [ExcludeOnRuntime(Runtime.Clr)] [ExcludeOnRuntime(Runtime.CoreClr)] [ExcludeOnRuntime(Runtime.Clr | Runtime.CoreClr)] [ExcludeOnRuntime(Runtime.ClrAndCoreClr)] // I think I prefer the bitwise one

With respect to determining which runtime/framework the tests are running on I don't mind requiring .NET 4.7.1 (supports Windows 7 SP1) for the unit tests, that is what BenchmarkDotNet is using for their shipping code anyway, and falling back to conditional compilation.

Regarding the attribute name though, should it be "runtime" or "framework"?

Of the two, I'm starting to tend more towards "framework", but I'm still undecided. I'll happily defer to your wise decision.

Perhaps, it doesn't matter too much, since any particular framework usually comes with its own runtime, so the two are tied together. I don't know of any two CLI runtimes that share a common framework library. Framework is the more general term of the two, so probably less restrictive if we wanted to enhance that [Exclude...] attribute later on.

There is a difference however, when it comes to versioning. While the CLR only has three major versions (1, 2, and 4), the .NET Framework has several more. Perhaps because of this, dotnet/corefx mixes up both terms into "target framework [moniker]": see e.g. its [SkipOnTargetFramework] attribute and the TargetFrameworkMoniker flags enum.

("Platform" would be another possibility. It's somewhat vague but that could actually be good for an attribute that we might want to expand with additional constraints later on.)

[ExcludeOnRuntime(Runtime.ClrAndCoreClr)] // I think I prefer the bitwise one

Agreed, a flags enum would be nicer.

Let's go with framework, the framework is the distribution of a runtime and BCL. For example, UWP's runtime is CoreCLR but is a different framework.

Which option do you like for the attribute name? [Framework] would work more like NUnit's [Platform]?

[ExcludeOnFramework(Framework.Mono)] [ExcludeOn(Framework.Mono)] [Framework(Exclude = Framework.Mono)] [Framework(Framework.Mono)] // could be Mono only [Framework(~Framework.Mono)] // this should be doable, but is it clear?

I'd go with the first one iff the Framework parameter is positional and not an optional, named one. Otherwise, I'd go with your second suggestion, which would allow for more future extensibility (e.g. [ExcludeOn(OS = OS.Unix, Framework = Framework.NetCore, ...)]

(The third suggestion suffers from Exclude being a named parameter and therefore IIRC optional. What effect would [Framework()] have? The last two suggestions suffer from unclear intent: Does a [Framework] attribute imply inclusion or exclusion?)

Good points. I'm also heavily leaning on the first one now too, very explicit and we don't really have (or want) unit tests that can't be run everywhere, obviously except for framework defects.

Would you now support multiple declarations on the same member (like the example below), or just merge text into one?

[ExcludeOnFramework(Framework.NetCore, "Blah blah see dotnet/coreclr/issues/1")] [ExcludeOnFramework(Framework.NetFramework, "Other see msconnect/2")] public void MyTest() { ... }

Edit: I wasn't clear, let's go with the first one [ExcludeOnFramework(Framework.NetCore)]

OK, the first form it shall be then!

Yes, there can be several of those attributes, which is good for being able to give different reasons for different frameworks.

Conversely, I'd still make the Framework enum a flags one so you can state the same reason once for several frameworks if they're all affected by the same problem.

jonorossi · 2018-05-14T12:57:36Z

docs/dynamicproxy.md

@@ -18,6 +18,7 @@ If you're new to DynamicProxy you can read a [quick introduction](dynamicproxy-i
 * [Use proxy generation hooks and interceptor selectors for fine grained control](dynamicproxy-fine-grained-control.md)
 * [SRP applies to interceptors](dynamicproxy-srp-applies-to-interceptors.md)
 * [Behavior of by-reference parameters during interception](dynamicproxy-by-ref-parameters.md)
+* [Some default parameter values may not be reproduced correctly in proxy types](dynamicproxy-default-parameter-values.md)


It might be better if the docs page title was something shorter like "Optional parameter value limitations".

jonorossi · 2018-05-14T13:01:35Z

docs/dynamicproxy-default-parameter-values.md

+   The underlying causes have been documented in [mono/mono#8504](https://github.com/mono/mono/issues/8504) and [mono/mono#8597](https://github.com/mono/mono/issues/8597).
+
+
+## When your code runs on .NET or .NET Core


".NET" should be ".NET Framework" (there are a few mentions below), it is important to identity the product especially when you then refer to ".NET Core" in the same sentence.

jonorossi · 2018-05-14T13:03:52Z

src/Castle.Core.Tests/DynamicProxy.Tests/ParameterDefaultValuesTestCase.cs

+			// parameter. Typically this matches the declared default value, but there are exceptions!
+			//
+			// This method list is intended to be exhaustive, i.e. it should cover *all* possible
+			// kinds of default values. If you find a case that is missing, please add it. (Some lines


I'd pull the sentence in parenthesis beginning "Some lines" into its own paragraph so it is more obvious.

Reproduction of default parameter values has caused a lot of trouble in the past and was eventually 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 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.

Replace the existing [ExcludeOnMono] attribute with the more generic [ExcludeOnFramework], which will also permit exclusion of tests on the .NET Framework and .NET Core.

All default value tests are assembled in a single fixture: * Four existing tests are no longer necessary because they are now covered by the new test cases. * Two tests contain faulty logic. Even if default value reproduction went completely berserk, they'd still succeed. Begone! * Two remaining tests are turned into test cases in the new fixture.

stakx · 2018-05-14T21:49:03Z

I cleaned up, rearranged, and squashed all these commits a bit.

The new [ExcludeOnFramework] attribute appears to be doing its job, judging from the build logs. I also added a few tests for it, just in case.

jonorossi · 2018-05-15T11:06:04Z

All good, merging.

stakx force-pushed the default-value-replication branch 8 times, most recently from 33ce560 to 20fbe77 Compare May 7, 2018 20:56

stakx changed the title ~~Work in progress: Add back ability to reproduce default parameter values~~ Add back ability to reproduce default parameter values May 7, 2018

stakx force-pushed the default-value-replication branch from 20fbe77 to eceb7dd Compare May 7, 2018 21:31

stakx commented May 8, 2018

View reviewed changes

stakx force-pushed the default-value-replication branch 4 times, most recently from ad4490a to f88111a Compare May 12, 2018 23:42

stakx commented May 13, 2018

View reviewed changes

jonorossi reviewed May 14, 2018

View reviewed changes

stakx force-pushed the default-value-replication branch 3 times, most recently from a83905a to bcf0a1c Compare May 14, 2018 18:50

stakx force-pushed the default-value-replication branch from bcf0a1c to aa97fb9 Compare May 14, 2018 20:37

stakx added 4 commits May 14, 2018 23:45

Introduce [ExcludeOnFramework] attribute

814917b

Replace the existing [ExcludeOnMono] attribute with the more generic [ExcludeOnFramework], which will also permit exclusion of tests on the .NET Framework and .NET Core.

Document default parameter value reproduction issues

f4a0343

Update the changelog

15a1fed

stakx force-pushed the default-value-replication branch from aa97fb9 to 15a1fed Compare May 14, 2018 21:45

jonorossi approved these changes May 15, 2018

View reviewed changes

jonorossi added this to the vNext milestone May 15, 2018

jonorossi merged commit acbf476 into castleproject:master May 15, 2018

stakx deleted the default-value-replication branch May 15, 2018 11:26

jonorossi mentioned this pull request Jun 12, 2018

ArgumentException in MethodEmitter #371

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add back ability to reproduce default parameter values #356

Add back ability to reproduce default parameter values #356

stakx commented May 6, 2018 •

edited

Loading

stakx left a comment

stakx May 8, 2018

stakx May 9, 2018

stakx May 8, 2018

stakx May 9, 2018

jonorossi commented May 13, 2018

stakx May 13, 2018 •

edited

Loading

jonorossi May 13, 2018

stakx May 14, 2018 •

edited

Loading

jonorossi May 14, 2018

stakx May 14, 2018 •

edited

Loading

jonorossi May 14, 2018

stakx May 14, 2018

jonorossi May 14, 2018 •

edited

Loading

stakx May 14, 2018

jonorossi May 14, 2018

jonorossi May 14, 2018

jonorossi May 14, 2018

stakx commented May 14, 2018 •

edited

Loading

jonorossi commented May 15, 2018


		You can only find out the correct default value by double-checking the default value in the original method of the proxied type.

		For .NET Core, the underlying cause of this problem has been documented in [dotnet/coreclr#17893](https://github.com/dotnet/coreclr/issues/17893).

		The underlying causes have been documented in [mono/mono#8504](https://github.com/mono/mono/issues/8504) and [mono/mono#8597](https://github.com/mono/mono/issues/8597).


		## When your code runs on .NET or .NET Core

Add back ability to reproduce default parameter values #356

Add back ability to reproduce default parameter values #356

Conversation

stakx commented May 6, 2018 • edited Loading

stakx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonorossi commented May 13, 2018

stakx May 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonorossi May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx commented May 14, 2018 • edited Loading

jonorossi commented May 15, 2018

stakx commented May 6, 2018 •

edited

Loading

stakx May 13, 2018 •

edited

Loading

stakx May 14, 2018 •

edited

Loading

stakx May 14, 2018 •

edited

Loading

jonorossi May 14, 2018 •

edited

Loading

stakx commented May 14, 2018 •

edited

Loading