Improve native `switch` on `Value` with a DiagnosticSuppressor
shuebner opened this issue · 18 comments
Hi,
this is a feature proposal. I don't know if this has been brought up before or if it is even in line with OneOf's philosophy, so feel free to close immediately.
Right now, consumers have to use OneOf's own Switch and Match method to have an exhaustive and type-safe way of acting on the underlying value. This is nice, but does not come with the native switch expression's (and statement's) syntactic sugar, like recursive pattern matching.
Using a native switch on Value right now is cumbersome because it is of type object. The compiler can thus not tell if you are matching on all the possible types. It will give you CS8509 (not exhaustive) as well as IDE0010 and IDE0072 (populate switch). The compiler warnings seem to be the only problem here.
It looks to me as if the experience could be improved by a comparatively simple DiagnosticSuppressor that checks exhaustiveness of the switch statement or expression on the basis of the guarantees that OneOf provides.
I happened to have just written such a DiagnosticSuppressor for the closed hierarchy pattern (the one with a private abstract base constructor and sealed nested subtypes). I assume it could be easily adapted to OneOf as well. Would that be something of value for consumers of OneOf? Or does such a thing already exist? Or is everyone just happy with the way things are?
@mcintyre321 any feedback on this?
I was not implying it should be part of this project, one reason being the aforementioned boxing costs.
My suggestion just meant to test the waters if a lot of people are switching on .Value for various reasons.
If yes, for those people it may make sense to have such an analyzer, so they can get correct warnings on missing matches. (the way @mcintyre321 phrased it, it sounded like my suggestion would produce errors, when in fact, it suppresses them when it is reasonably sure that the switch is exhaustive; if it is not, you will still get the compiler's warnings, because you do neither suppress them nor use a discard match to shut them up)
No, you are not :-)
Without the analyzer, switch statements and switch expressions on OneOf<T1, T2, T3>.Value get CS8509 and either IDE0010 or IDE0072, even if they match on all T1, T2 and T3. That is annoying because there is no way to get rid of the warning and still get a new warning if an actually possible case is not matched (e. g. when a new case T4 is added).
The diagnostic suppressor looks at each CS8509, IDE0010 and IDE0072 diagnostic.
If it finds that the switch statement switches on the Value property of an instance of type OneOf<T1, T2, T3>, it will look at the cases (otherwise it will not do anything and the diagnostics are not suppressed).
Then, if it is certain that T1 and T2 and T3 is matched without any restrictions (no when, no type narrowing etc.), it suppresses the diagnostic.
If it is not certain, it does not suppress the diagnostic.
You see that the compiler diagnostics are not lost.
They are only suppressed whenever the diagnostic suppressor is sure that all cases are accounted for.
This analysis can be made arbitrarily clever as long as it always errs on the safe side, i. e. when in doubt does not suppress.
Think of it as the compiler gaining awareness of OneOf, just as it is aware of other magic built-in stuff in C# like IEnumerable and Add enabling the collection initializer syntax, GetAwaiter enabling await and SelectMany enabling LINQ syntax.
I am using such a suppressor on all provably closed type hierarchies in my current project (think Either<T1, T2>) and it is working great with only medium cleverness in the suppressor's analysis.
@shuebner Would such an analyzer have to be shipped in the same package in order to work, or could this be released as a separate NuGet package for those who want "the upgraded experience"?
@mcintyre321 I do not know what you mean by "protected". The compiler behavior with the suppressor is strictly superior to the behavior without.
If it is shipped with the same package, people do not need to know about it. It will just do its work and people switching on Value will be pleasantly surprised that they get correct warnings now when new cases are added to some OneOf on whose Value they switch. (Provided they did not suppress the warnings or add a discard match already, in which case they will never get a warning independent of the proposed suppressor.)
If it is shipped as a separate package, people would have to install that to get the benefits. If they don't, they get the current behavior with no way of getting warnings when new cases are added to a OneOf on whose Value they switch.
@shuebner in case the maintainer thinks this is out of scope for OneOf, would you mind publishing it as a separate package? I would love to use it, since I'm also doing native pattern matching on the value from time to time (which is why I found this thread).
I do want to do it anyway for fun and bragging rights 😉
This proposal was mainly to find out if it is going to be used or not. A question that has not been answered yet.
I will not get around to implementing it any time soon though (think summer) because even adaption from the current working suppressor for closed type hierarchies is going to take a couple of hours that I cannot spare right now.
Hi. I would definitely want to see it in action as a seperate download before I would consider including it as a reference in OneOf.
- I still think that using
.Valueisn't a recommended approach due to boxing (I have considered removing it entirely before, hearing people are making regular use of it makes me want to do that even more!) - I'm still concerned that downstream consumers wouldn't get a compile time error, which is a Red Line
Just release new major version 😏
This thread is not dead, I just have very little time :-)
As a teaser, I finally got around to publishing a diagnostic suppressor that works well for closed type hierarchies.
It should be easily adaptable to OneOf.
https://github.com/shuebner/ClosedTypeHierarchyDiagnosticSuppressor.
It is NRT-aware and supports a variety of pattern matching including base type matching, property patterns and nested patterns.
Available on nuget:
<PackageReference Include="SvSoft.Analyzers.ClosedTypeHierarchyDiagnosticSuppression" Version="1.0.0" PrivateAssets="All" />Looking forward to having the analyzer work with OneOf @shuebner . One of the worst aspects of the library is exactly the Match method because of how non-idiomatic it is. Being able to use a proper switch would be an absolutely massive improvement.
I'd be willing to make the change to standard switch even with the loss regarding boxing on .Value to be honest. The gains in readability and familiarity will trump that loss in the vast amount of cases.
@julealgon
Thanks for the feedback.
I find the type inference for native switch to be much better, too.
@mcintyre321
I finally understand now what you meant by
I'm still concerned that downstream consumers wouldn't get a compile time error
Sorry it took so long.
You mean that when adding, removing or changing a type argument of a OneOf during development, people using native switch on Value may get a compiler warning/error depending on their settings whereas people using the OneOf.Match method will always get a compiler error.
Yes, that is true. That is a disadvantage when using Value.
However, with my suppressor, people gain the ability to escalate the exhaustiveness warnings to errors, because the false positives will be suppressed.
I myself have escalated the relevant switch exhaustiveness warnings to errors in my .editorconfig, i. e. CS8509, IDE0010 and IDE0072.
I do this because I want to be forced by the compiler to be exhaustive in all cases and use a diagnostic suppressor to avoid false positives.
Concerning boxing:
I assume the performance penalty would occur whenever Value is used on a OneOf instance, one of whose type arguments is a non-nullable value type?
That is something that could independent of this issue be reported by a simple analyzer. People could then decide how important this performance disadvantage is for them and set the severity accordingly.
Alright, I slapped together a basic diagnostic suppressor for OneOf:
<PackageReference Include="SvSoft.OneOf.Analyzers.SwitchDiagnosticSuppression" Version="0.0.1" PrivateAssets="All" />Repository at https://github.com/shuebner/OneOfDiagnosticSuppressor.
Please let me know what you think, if it's worth it, bugs you find and features you crave by opening an issue.
Thank you for this opportunity.
@shuebner would there be any way to make this work without having to specify .Value at all? That would make the library feel even closer to actual discriminated unions.
@julealgon Not on the analyzer level. If it were possible it would at a minimum be a significant change in how the compiler generates IL from switches. switch is a language construct and as long as you want to call the language "C#", a switch on an instance has defined semantics. Silently switching on some property of that instance would violate the language specification (not checked, but it would have to, right?).