Roslyn Analyzer and Code Fix for repeated StringName implicit allocations #106096
Conversation
There was a problem hiding this comment.
Thanks for contributing to the .NET module!
The .NET team was discussing something like this the other day. We regret making the conversion between StringName and String implicit and think it should've been explicit instead. However, given how much it would break to change this now, we think an analyzer and code fixer would be a great alternative.
We were thinking of reporting a diagnostic for all cases of implicit conversion, and not just in loops. Checking if the implicit conversion is inside a loop doesn't seem worth the effort and likely misses complex scenarios.
We think the noise would be acceptable because in most cases there's a preferable alternative that we'd want to encourage users to use instead (such as PropertyName, MethodName, or SignalName). It would also be easy to disable the warning with .editorconfig, #pragma warning disable, or an explicit cast (which also conveys that the conversion was intentional). The code fixer would also make it easy to fix and can be applied in batch.
|
@raulsntos so just to recap, you’re saying that this should warn on every single implicit conversion from That is a trivial change, I’ll take care of it tomorrow morning! |
I mean, it doesn’t have to disable the warnings. Slap a framework attribute on the new string parameters (like |
|
I'm not familiar with Roslyn Analyzer, but #106066 still isn't worth adding the complexity. In places where performance matters, there's no reason to use strings instead of constructing StringName in advance. By the way, the warnings and auto fixes should also be provided for |
I strongly disagree with that. In fact I believe that the Godot C#'s closest competitor is Unity, and they don't require you to create cache variables just to check key/gamepad inputs, for example. Simplicity is king, as long as it doesn't tank framerates. |
|
@raulsntos I updated it as discussed, now it will trigger warnings on every implicit |
Implicit
StringNameallocations fromstrings are fine, however they can hide the allocations from the user. This is even more important when they get repeatedly called in tight loops(*). Related issues: #106066 and #105750This analyzer issues a warning when these constant
StringNameallocations happen in any kind of loop:Note that only constant strings (as the compiler sees them, so it includes constant string concatenation, etc) are considered, something like this is still fine:
It will offer to automatically fix it by creating a
static readonly StringNamefield in the class and replace all allocations with it:It will also automatically reuse an existing such field, if one exists:
(*) Note: right now it only considers actual C# loops, it does not consider implicit frame loops like the ready or process functions. It would be easy to add if it's needed, however that would flag every single
StringNameimplicit conversion like in input checks. There would be a performance gain from it, but.. it makes the Godot API look kind of shaky if it's needed to such a high degree.Also note: This is fully compatible with the #106066 proposal, as it checks the semantic model for implicit conversions. So if a
vs 
stringoverload is present and the compiler successfully binds thestringto it there will be no implicit conversion to trigger the analyzer: