[.NET] Better `.editorconfig` setup in `modules/mono/` #88570

paulloz · 2024-02-19T21:23:21Z

Remove explicit warnings on CA1716, CA1304 (already included in the level of analysis we run)
Do not silence CA1805
Properly fix REFL045
Properly fix CS1572, CS1573
Suppress CS0618 in InvokeGodotClassMethod
Only silence CA1707, CA1711, CA1720 on a more limited scope
Temporarily silence CS1734 in generated code
Enforce (not really) naming conventions
Enforce (not really) Allman braces

raulsntos

Looks great, thanks for working on this.

modules/mono/glue/GodotSharp/GodotSharp/Core/DelegateUtils.cs

modules/mono/.editorconfig

raulsntos · 2024-02-20T03:12:21Z

modules/mono/.editorconfig

+# Diagnostics to prevent defensive copies of `in` struct parameters
+resharper_possibly_impure_method_call_on_readonly_variable_highlighting = error
+
+# Severity levels for dotnet_naming_rule only affect IDE environments.


Do the naming rules affect dotnet format? If so, our CI check should enforce them.

It does, and doesn't. It's dependent on the severity level we set for IDE1006. We can set it on suggestion if we don't want to block PRs, or on warning if we want to. One thing to note is IDE1006 won't apply changes automatically, so it would be listed as dotnet format issues, but no corrections would be visible in the diff. If that make sense?

(I personally think we should set it as suggestion, and go from there)

it would be listed as dotnet format issues, but no corrections would be visible in the diff

But would it show up in the Files changed tab? (like the analyzer tracking diagnostic) If so, I think that may be good enough. But I guess suggestion won't show, so we'd have to go with warning.

I'm not entirely against setting that as warning, but it means we'd need to scrap the current codebase of formatting issues. Also, am I right in the rules I defined? Because it's not entirely consistent at the moment.

At a first glance it looks correct, although they're probably not exhaustive enough (for example it doesn't cover parameters, local variables, properties, etc). For reference, here's the .editorconfig used by roslyn: https://github.com/dotnet/roslyn/blob/6eb91fbee2f416285dcc7603e8c8769f78ed0aa8/.editorconfig#L65-L143

As for the current codebase, it's likely that we're not consistent because we didn't start closely following the code-style until somewhat recently. We'll have to do that at some point but I think it's fine to set the severity to suggestion for now.

Yes, I only added rules for things where I saw naming issues for now. I'll add more exhaustive ones before un-drafting 👍

Basically:

Everything is PascalCase

Non-public fields are _camelCase

Unless they are const / static readonly, in which case they are PascalCase

Local variables are camelCase

paulloz · 2024-02-21T23:51:38Z

I'm un-drafting this one.

There is an additional commit that I left separate. It fixes the indentation in the generated GD_constants.cs file, I needed it to be able to run dotnet format without being flooded by bad indentation errors. If I'm not too stupid, content shouldn't have changed, only it doesn't trip the formatter any more. It can either be squashed or dropped.

I wrote additional naming rules in the .editorconfig file, but left the diagnostic itself on suggestion. I'm linking some logs of a dotnet format run with its severity set on warning for reference. It's not that bad after all.

raulsntos · 2024-02-23T20:18:54Z

From what I can see, most of the violations are from InteropStructs.cs, NativeFuncs.cs and Internal.cs. I think we should just exclude those files.

As for the generated files, it looks like most violations are due to virtual methods having a _ prefix. I wonder if we can allow that prefix (without it being required) when the method is virtual.

I'm not sure what the rule should be for static readonly, it seems we use _camelCase for them a lot but the rules in this PR say it should be PascalCase. For reference, the Roslyn repo uses s_camelCase. I don't have a strong preference as long as we're consistent, but using PascalCase may conflict with other members (for example Vector3._zero and Vector3.Zero).

It would be great to also add a rule to require tuples to use explicit naming and use PascalCase, there's dotnet_style_explicit_tuple_names but it seems we can't specify naming rules for tuple elements yet (dotnet/roslyn#38705).

paulloz · 2024-02-23T20:39:31Z

I'm not sure what the rule should be for static readonly, it seems we use _camelCase for them a lot but the rules in this PR say it should be PascalCase.

Yeah, we kind of use both currently. With the PascalCase version more in the source generator part of the codebase. I really don't have a strong opinion, my usual take is to treat them as consts.

paulloz · 2024-02-23T20:42:03Z

As for the generated files, it looks like most violations are due to virtual methods having a _ prefix. I wonder if we can allow that prefix (without it being required) when the method is virtual

Sadly, I don't think that's a thing.

raulsntos · 2024-02-24T00:46:08Z

As for the generated files, it looks like most violations are due to virtual methods having a _ prefix. I wonder if we can allow that prefix (without it being required) when the method is virtual

Sadly, I don't think that's a thing.

Looks like there's an issue for this: dotnet/roslyn#46205. Until then, since we should only be using the _ prefix in generated virtual methods, maybe we can just add an exception for files in the Generated directory. Or surround the generated method with #pragma disable.

paulloz · 2024-02-24T12:05:33Z

Removed the rule on static readonly (I noticed that currently, private ones are _camelCase, and public ones are PascalCase, matching the rules for other fields)
Silence IDE1006 in the Generated directories
Ignore IDE1006 in
- NativeFuncs.cs
- NativeFuncs.extended.cs
- InteropStructs.cs
- Internal.cs
Did a bit of cleaning (not a lot)
- Removed a bunch of [SuppressMessage("ReSharper", "...")] attribute to replace them with // ReSharper comments (I'm not sure if we build releases using CODE_ANALYSIS or not)
- Fixed naming rule violations in GodotSharp
- Fixed naming rule violations in Godot.SourceGenerators.*

raulsntos · 2024-02-24T14:10:02Z

modules/mono/glue/GodotSharp/GodotSharp/Core/DelegateUtils.cs

@@ -771,10 +771,10 @@ public static godot_variant ConvertToVariant(object? obj)
                    [typeof(Variant)] = (in godot_variant variant) => VariantUtils.ConvertTo<Variant>(variant),
                };

-            [SuppressMessage("ReSharper", "RedundantNameQualifier")]
+            // ReSharper disable RedundantNameQualifier


Shouldn't this be disable once like the others? Then we don't need to restore it later.

Suggested change

// ReSharper disable RedundantNameQualifier

// ReSharper disable once RedundantNameQualifier

disable once would only affect the next line, here we affect the entire method.

Ah I see. What is this suppressing anyway? Is it because of the Godot.Collections fully-qualified names? If so we may want to suppress this diagnostic everywhere, since we prefer to fully qualify Godot's collections.

Yes, it's because we're inside the Godot namespace, and we specify Godot.Whatever.

modules/mono/glue/GodotSharp/GodotSharp/Core/GodotObject.base.cs

modules/mono/glue/GodotSharp/.editorconfig

modules/mono/editor/bindings_generator.cpp

modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs

modules/mono/build_scripts/build_assemblies.py

modules/mono/glue/GodotSharp/GodotSharpEditor/Compat.cs

modules/mono/glue/GodotSharp/GodotSharp/Compat.cs

modules/mono/build_scripts/build_assemblies.py

paulloz · 2024-02-27T18:10:51Z

I'll squash once we know there's nothing more we wanna add to this specific PR.

raulsntos

I think this looks good enough already, we can always add more rules to .editorconfig later. Thanks for the great cleanup!

New rules: - Do not silence CA1805 any more - Limit where we silence CA1707, CA1711, CA1720 - Enforce severity=warning for IDE0040 - Enforce Allman style braces - Enforce naming conventions (IDE1006 is still severity=suggestion) Fixes: - Fix REFL045, CS1572, CS1573 - Suppress CS0618 when generating `InvokeGodotClassMethod` - Fix indent when generating GD_constants.cs - Temporarily silence CS1734 in generated code - Fix a lot of naming rule violations Misc.: - Remove ReSharper comments for RedundantNameQualifier - Remove suppression attributes for RedundantNameQualifier - Remove severity=warnings for CA1716, CA1304 (already included in the level of analysis we run)

paulloz · 2024-02-27T19:42:35Z

Here's to hoping I didn't break everything 🥂

akien-mga · 2024-02-27T20:24:00Z

Thanks!

aaronfranke · 2024-02-29T20:54:45Z

Enforce (not really) naming conventions

When did the naming convention for private members change to PascalCase?

raulsntos · 2024-03-01T02:55:20Z

The changes in this PR are only meant to add the conventions that we've already been following to the .editorconfig, so they can be enforced by dotnet format in CI. Private fields are still following the _camelCase naming that has always been our codestyle.

I guess you're referring to the few constant names that were changed from _camelCase to PascalCase in this PR. As discussed in the PR, we haven't been consistent throughout the codebase on the constant naming style so we just chose the one that is also recommended by the .NET naming guidelines.

paulloz added enhancement topic:dotnet topic:codestyle labels Feb 19, 2024

paulloz added this to the 4.x milestone Feb 19, 2024

raulsntos reviewed Feb 20, 2024

View reviewed changes

paulloz force-pushed the dotnet/warnings-editorconfig-cleanup branch 3 times, most recently from 1bdaa45 to 8b9c594 Compare February 21, 2024 22:36

paulloz marked this pull request as ready for review February 21, 2024 23:51

paulloz requested a review from a team as a code owner February 21, 2024 23:51

paulloz mentioned this pull request Feb 25, 2024

C#: Add deprecated message to generated bindings #88730

Merged

akien-mga changed the title ~~[.NET] Better .editorconfig setup in modules/mono/~~ [.NET] Better .editorconfig setup in modules/mono/ Feb 25, 2024

raulsntos reviewed Feb 26, 2024

View reviewed changes

paulloz force-pushed the dotnet/warnings-editorconfig-cleanup branch from 30d6054 to 621ff8c Compare February 26, 2024 20:08

raulsntos reviewed Feb 26, 2024

View reviewed changes

modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs Outdated Show resolved Hide resolved

paulloz force-pushed the dotnet/warnings-editorconfig-cleanup branch from 621ff8c to 0fa7abf Compare February 27, 2024 09:01

raulsntos reviewed Feb 27, 2024

View reviewed changes

raulsntos modified the milestones: 4.x, 4.3 Feb 27, 2024

paulloz force-pushed the dotnet/warnings-editorconfig-cleanup branch from 0fa7abf to 801cea0 Compare February 27, 2024 16:21

raulsntos approved these changes Feb 27, 2024

View reviewed changes

paulloz force-pushed the dotnet/warnings-editorconfig-cleanup branch from 801cea0 to 139a5df Compare February 27, 2024 19:23

akien-mga merged commit a64cb8e into godotengine:master Feb 27, 2024
16 checks passed

paulloz deleted the dotnet/warnings-editorconfig-cleanup branch February 27, 2024 22:00

This was referenced Feb 27, 2024

Various fixes to C# warnings #65532

Closed

Fix C# epsilon compiler error on double precision build #88994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[.NET] Better `.editorconfig` setup in `modules/mono/` #88570

[.NET] Better `.editorconfig` setup in `modules/mono/` #88570

paulloz commented Feb 19, 2024 •

edited

raulsntos left a comment

raulsntos Feb 20, 2024

paulloz Feb 20, 2024 •

edited

raulsntos Feb 20, 2024

paulloz Feb 20, 2024

raulsntos Feb 21, 2024

paulloz Feb 21, 2024 •

edited

paulloz commented Feb 21, 2024

raulsntos commented Feb 23, 2024

paulloz commented Feb 23, 2024

paulloz commented Feb 23, 2024

raulsntos commented Feb 24, 2024 •

edited

paulloz commented Feb 24, 2024 •

edited

raulsntos Feb 24, 2024

paulloz Feb 26, 2024

raulsntos Feb 26, 2024

paulloz Feb 26, 2024

paulloz commented Feb 27, 2024

raulsntos left a comment

paulloz commented Feb 27, 2024

akien-mga commented Feb 27, 2024

aaronfranke commented Feb 29, 2024

raulsntos commented Mar 1, 2024

	// ReSharper disable RedundantNameQualifier
	// ReSharper disable once RedundantNameQualifier

[.NET] Better .editorconfig setup in modules/mono/ #88570

[.NET] Better .editorconfig setup in modules/mono/ #88570

Conversation

paulloz commented Feb 19, 2024 • edited

raulsntos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulloz Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulloz Feb 21, 2024 • edited

Choose a reason for hiding this comment

paulloz commented Feb 21, 2024

raulsntos commented Feb 23, 2024

paulloz commented Feb 23, 2024

paulloz commented Feb 23, 2024

raulsntos commented Feb 24, 2024 • edited

paulloz commented Feb 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulloz commented Feb 27, 2024

raulsntos left a comment

Choose a reason for hiding this comment

paulloz commented Feb 27, 2024

akien-mga commented Feb 27, 2024

aaronfranke commented Feb 29, 2024

raulsntos commented Mar 1, 2024

[.NET] Better `.editorconfig` setup in `modules/mono/` #88570

[.NET] Better `.editorconfig` setup in `modules/mono/` #88570

paulloz commented Feb 19, 2024 •

edited

paulloz Feb 20, 2024 •

edited

paulloz Feb 21, 2024 •

edited

raulsntos commented Feb 24, 2024 •

edited

paulloz commented Feb 24, 2024 •

edited