dotnet
Put TArg before callbacks
Throughout roslyn, we have a lot of callback-based code that uses a TArg parameter for context, to avoid costly repeat lambda allocations. Unfortunately, they uniformly put TArg after the delegate type. This is not great for IDE usability, as the compiler can't make any type inferences on the callback parameter until the TArg argument is actually specified. This results in an awkward pattern of just typing an empty lambda, filling out the TArg parameter, and then going back to the body of the lambda to fill it out. I've previously asked that new overloads of these types of methods put TArg before the callback, to avoid this pitfall. The pushback on this has always been "let's address that uniformly across the project, rather than making this one callback different." Well, I got tired of people telling me that, so I did just that.
In order to avoid any potential for ordering differences and to make the code easier to review, I opted to leave existing callsites essentially untouched: I just explicitly specified parameter names for calls, so that the current execution orders are unchanged. To construct this PR, I wrote a small tool that went through all of our non-public, callback-taking methods (namely, generic methods with a type parameter named TArg, a parameter of that argument, and a parameter of a delegate type that is generic and takes a TArg parameter). For each call of these methods, it found the first callback parameter, and the first TArg parameter. If they were out of order (ie, callback before TArg) then it did:
- For every method invocation, it adjusted the code to explicitly name every argument from the callback to the end of the invocation. This ensures that we won't have compilation errors when we reorder parameters.
- For every method definition, it pulled the
TArgparameter to just before the first callback.
I then went through, fixed up formatting errors, and verified that nothing was getting unexpectedly reordered. To make this process easier to review, I have split this into 4 commits. The first commit is just parameter names. The second commit is just parameter reordering. Commit 3 is a few VB formatting updates that needed to happen after parameter names shifted lambda locations, and a baseline change that was asserting a method signature. Commit 4 is a fix to the IOperation generator for the new format. I would definitely suggest reviewing commit-by-commit here.
@CyrusNajmabadi @tmat I got tired of being told new apis shouldn't be in this form, so here you go.
Thoughts on making it a compiler feature?
If a method follows certain TArg lambda passing pattern (potentially marked with a new attribute) the compiler would automatically pass captured locals via TArg. The IDE would hide TArg from the signature.
Thoughts on making it a compiler feature?
If a method follows certain
TArglambda passing pattern (potentially marked with a new attribute) the compiler would automatically pass captured locals via TArg. The IDE would hide TArg from the signature.
I think I'd need to think about it more, though I also wonder how @CyrusNajmabadi would feel about it. It's a level of signature munging and magic that I don't know that I'd immediately be comfortable with; in particular, I don't know how, as a reviewer, I'd really know that the method is ok.
Perhaps the caller would be required to use static on the lambda. If the lambda has captures it would be an error without this feature. If the method supports the TArg passing static would convert the captures to the TArg.
@tmat @CyrusNajmabadi regardless of language feature proposals, I'd still like to move forward with this change. Can I please get reviews?
I would like to see a .NET Libraries / API design signoff before this merges, since it deviates from the previously established pattern. This feels like hiding a known problem affecting many users instead of trying to fix it (addressing it only in the dogfood environment while leaving everyone else unresolved). The pain caused by the current state is an accurate and desirable reflection of the behavior everyone sees inside and outside this project.
The pain caused by the current state is an accurate and desirable reflection of the behavior everyone sees inside and outside this project.
These are our apis, and entirely within our control to shape however we want.
Perhaps the caller would be required to use
staticon the lambda. If the lambda has captures it would be an error without this feature. If the method supports the TArg passingstaticwould convert the captures to the TArg.
Will there be possibly a proposal for this?
There are many APIs that support TArgs and analyzers to suggest to use it, but that still will look quite like some decompiled code.
It's a level of signature munging and magic that I don't know that I'd immediately be comfortable with
I think this could be close to caller info attributes in terms of how it looks like and work? (an optional parameter with compiler-provided value)
I don't see the need for a language proposal here. If someone wants an analyzer for this, then one can just be written.
I don't see the need for a language proposal here. If someone wants an analyzer for this, then one can just be written.
The suggestion was about a compiler feature https://github.com/dotnet/roslyn/pull/74370#issuecomment-2231848280, analyzers work fine but the final result is still hard to read as mentioned in my previous comment, this is actually a common pattern that many apis follow. Note that I'm not suggesting that this should "light up" in existing code, that's up for debate, I was just checking if there's gonna be a proposal out of the discussion here.