In OneOf.SourceGenerator - Generate type specific TryPick/As methods?
jamesbascle opened this issue · 7 comments
I find the .AsTn and .TryPickTn methods very useful, but potentially fraught. For example:
if(result.TryPickT5(out Success _, out var error))
If you do not use the type Success there, and instead do if(result.TryPickT5(out var _, out var error)), then in future, if someone changes the order of the OneOf's generic types, it would change the consuming logic.
It would be cool if the generator would generate something so that you could do if(result.TryPickSuccess(out var _, out var error)).
Maybe this could be controlled by a new optional GenerateOneOf constructor parameter? Thoughts? Would you accept this as a PR if implemented?
Do you mean in the manner I noted, or via some other mechanism? The problem I see is that two entirely innocent changes, one to use var, and one to add an additional component of the union, could cause business logic to become wrong, because there's no semantic binding in the method call, merely a positional artifact due to the limits of the way the union is implemented.
As I think through this more, I am remembering why I started (and stopped!) working on DiscU :D
No shade, I understand your position here and of course appreciate all your work on this lib.
I think probably the best thing to do is create an analyzer that prevents using var for the first out variable in the .TryPickTn method calls. Is that something you'd accept, or should I just get that made and published as a separate analyzer only package?
@jamesbascle there a lot of edge cases here FYI: #95
System.Text.Json tries to do something similalr, but in case of errors they allow to ovrload naming via TypeInfoPropertyName
Yeah, I understand. I Have now had a week to think about this more and I think the approach I outlined in my last post would be the best way to go, since it doesn't change the API, or require any additional code generation, it just puts guard rails onto the use of the TryPickTn methods.
I'm going to have it implemented for us internally as an analyzer, and we'll import OneOf/OneOf.Extended/OneOf.SourceGenerator through that package with the analyzer attached.
If that's something you would want to integrate into the OneOf NuGet package directly, I can see about getting it open sourced.
This has some overlaps with #93
Indeed it does. But, I think I've addressed my concerns with our team adopting the package with some analyzers to enforce good practice, so I will close this.