C#: Improve binding generator checks/passes

[Repiteo]

This is in a similar vein to my other PR, #79351, but it's more involved as it directly changes codeblocks within bindings_generator.cpp so it warrants seperation imo. The main changes are

• Enum types will now properly utilize pascal case when referenced in the documentation
  • Migrated to C#: Implement proxy_name for EnumInterface #80628
• paramref now properly tagged in documentation, unless they're part of a signal
  • Migrated to C#: paramref now properly tagged in documentation #80630
• Implemented basic docstrings for public auto-generated methods/values
• Fixed delegate docstring logic so it doesn't cause double-summaries
  • Migrated to C#: Fixed delegate docstring logic #80631
• Generated methods now include their argument types, preventing ambiguous references with Compat.cs
  • Migrated to C#: Include argument types in generated methods #80629
• Fixed core documentation attempting to reference editor api
  • Migrated to C#: Dereference editor types in core documentation #80632
• Properly append "new" to methods hiding inherited names
• Implement warning logs

[RedworkDE]

Diffs caused by this: https://gist.github.com/RedworkDE/731e64edd3002d34416bb91c4eb3d8f5

[Repiteo]

The logic for those "new" checks will take more time to think about than I expected, so I'll remove that section for now. So instead of focusing on clearing up warnings specifically, this is now more about QOL for bindings_generator overall; the OP has been updated accordingly

[RedworkDE]

https://gist.github.com/RedworkDE/46aeeffb844a61b9cae78117ba18f389 diffs got a whole lot bigger, generally look good, but I haven't looked at this in detail yet.

[Repiteo]

Ohhh yeah, they're way bigger thanks to the documentation additions

[raulsntos]

I haven't reviewed the diff yet, but I have looked at the code changes and left some comments.

Some of these changes overlap with #79239. I prefer the documentation I wrote but I'm biased 😄.

[Repiteo]

I preferred your documentation too, so I just used that instead where applicable. Because it didn't use specific class references, I was able to trim a TON of the excess generation size via <inheritdoc/>

[RedworkDE]

Are there any particular reasons to use <inheritdoc/> here beyond that it makes the generated code a bit smaller? IMO size of the generated code is not really a metric that matters until you reach like millions of generated LOC.

[Repiteo]

The size is the only real advantage I can think of, but at the same time I can't think of a reason not to do it if the documentation would otherwise be identical. That is to say, if we think it's worth making specific class references in the documentation, then we could drop <inheritdoc/> without hesitation

[Repiteo]

New update that fixes core documentation attempting to reference editor api. If I can figure out a way to safely handle CS0108, that'll be all generated warnings accounted for!

Also, in fixing that last bit, I realized that the api_type check isn't 100% thorough, as both ResourceImporterMP3 and ResourceImporterOggVorbis are considered core types, when they're supposed to be editor-specific. Though that might be something that needs to be resolved in core, as they appear to be handled directly in their modules & I'm not sure if that needs to be somehow migrated to register_editor_types or what

[Repiteo]

@raulsntos I think I've got something figured out for the "new" situation! This updated setup brings back the old method, but utilizes a newly added _log_warn to output notices to the console during generation. This should address the concern of erroneously hiding away critical information, while simultaneously addressing the warnings in the generated code

[raulsntos]

Can you split the PR by diagnostic Id? I think it'd be easier to review that way. Some of the changes in this PR are straight-forward and I'd like to approve them but I'm not so sure about the other changes.

Are there any particular reasons to use <inheritdoc/> here beyond that it makes the generated code a bit smaller?

The size is the only real advantage I can think of, but at the same time I can't think of a reason not to do it if the documentation would otherwise be identical.

The documentation will be identical, but adding extra code to the bindings generator (which is written manually) to make generated code cleaner doesn't seem like an improvement to me. In general, we should prioritize cleaner code in the non-generated code since it's the code that we have to write and maintain ourselves, the generated code can be as long and repetitive as we want since it's machine-generated.

New update that fixes core documentation attempting to reference editor api.

I think it's weird that GodotSharp API is referencing GodotSharpEditor API, I'd like to take a better look at why that's happening.

I think I've got something figured out for the "new" situation! This updated setup brings back the old method, but utilizes a newly added _log_warn to output notices to the console during generation.

So the warning is silenced in the .NET compiler and then we print it manually to the console? I'm not sure how people that compile with warnaserror would feel about that, since it feels like we're bypassing it. I do like it more than completely hiding it though.

[Repiteo]

The documentation will be identical, but adding extra code to the bindings generator (which is written manually) to make generated code cleaner doesn't seem like an improvement to me. In general, we should prioritize cleaner code in the non-generated code since it's the code that we have to write and maintain ourselves, the generated code can be as long and repetitive as we want since it's machine-generated.

That… Is a very good point. It should be safe to revert that then

So the warning is silenced in the .NET compiler and then we print it manually to the console? I'm not sure how people that compile with warnaserror would feel about that, since it feels like we're bypassing it. I do like it more than completely hiding it though.

Maybe "warning" is a misnomer, and it should be something more like _log_notice. This technically handles the warning so it's not exactly something warnaserror should be picking up, but it shouldn't be hiding away the process either

Can you split the PR by diagnostic Id?

That being, the type of warning/case that is addressed? That should be simple enough if so

[raulsntos]

Maybe "warning" is a misnomer, and it should be something more like _log_notice.

I don't mind the name. I was just thinking that if someone is using warnaserror they probably want these to fail the build. Otherwise, I imagine they would've disabled the diagnostic. You can always change the diagnostic level for individual diagnostics.

dotnet_diagnostic.CS0108.severity = none

That being, the type of warning/case that is addressed?

Yeah, the diagnostic Id reported by MSBuild. I think splitting the PR by the list mentioned in the PR description should be good:

• Enum types will now properly utilize pascal case when referenced in the documentation

This one causes CS1574.

• paramref now properly tagged in documentation, unless they're part of a signal

This doesn't actually cause a diagnostic, but I still think it'd be better as a separate PR.

• Implemented basic docstrings for public auto-generated methods/values

This one doesn't cause a diagnostic either, but I feel like it would be better separated too.

• Fixed delegate docstring logic so it doesn't cause double-summaries

I guess this one doesn't cause a diagnostic? But I would also separate it.

• Generated methods now include their argument types, preventing ambiguous references with Compat.cs

This one causes CS0419.

[Repiteo]

Closing as everything here has been implemented/superseded elsewhere.