Make default value replication of optional parameters more tolerant of type-mismatched default values #375
Conversation
a8f5062 to
3eb77a7
Compare
It is possible to produce metadata constants whose type does not exactly match that of an optional parameter, but might be coercible. Add some tests for this scenario using [DefaultParameterValue].
3eb77a7 to
25a06af
Compare
| { | ||
| CopyDefaultValueConstant(from: parameter, to: parameterBuilder); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Note that I'm intentionally wrapping CopyDefaultValueConstant with a catch-all instead of rewriting it so that it never throws. Why? Because adding catch-all clauses inside CopyDefaultValueConstant (two of them, one for reading the default value, one for writing it) would make it less obvious why we're going to great lengths catching specific exceptions first. A later maintainer might say, let's just eliminate all these superfluous catch (...) clauses and just keep the catch-all. While that would be a operationally correct optimization (less catching and throwing overall), it would also remove much valuable information from the source code about what can possibly go wrong.
There was a problem hiding this comment.
Have not reviewed this PR properly yet but I am liking this approach. It formalises ones understanding as a snapshot or point in time. Things change and I am a firm believer in fail and fail hard. Don’t hide things. Like it!
| // might produce such metadata. Make a final attempt to coerce it to the required type: | ||
| try | ||
| { | ||
| var coercedDefaultValue = Convert.ChangeType(defaultValue, parameterNonNullableType); |
There was a problem hiding this comment.
Should probably explicitly specify the invariant culture here, otherwise this performs a conversion using the thread's current culture.
bdd8122 to
c4eeabd
Compare
|
Looks great. I kinda knew something would pop up that we missed adding the default value stuff back, but this is a pretty niche problem and a sensible fix. |
This should resolve #371 by making two changes:
If
MethodEmitter.CopyDefaultValueConstantfails to set the default value for a parameter, it tries a second time but with a default value that's been coerced to the parameter's type. (Because the type coercion can itself fail, it's not done right away, but only when there's a chance that it might help resolve a previous problem.)Default value replication is turned into a nice-to-have feature that allows silent failure.
Regarding (2), we've been discussing this before (IIRC, in #291), and @jonorossi's opinion was that we should not fail silently because we want to learn about potential problems in default value replication. That discussion was based on the premise that we only need to deal with metadata that's been generated by the .NET Roslyn compilers. As it turns out now, we forgot about the possibility that there are other code generators out there which might not follow the same strict rules of the .NET compilers, and do things that we cannot easily write test cases for from C#'s end.
Since ECMA-335 doesn't require metadata constants to match their associated parameters' types (they're recorded for purely informative purposes), we shouldn't throw errors when we fail to interpret such default values.