Various fixes to C# warnings #65532

raulsntos · 2022-09-08T18:16:02Z

Suppress CS0108 caused by shadowing members in derived classes.
- Fixes Multiple C# classes hiding inherited members in GodotSharp #15763
Suppress CA1716 caused by parameter names matching C# keywords.
- Follow up to Clean a number of C# warnings #87518.

Extracted to different PRs

Suppress REFL045 in a false positive scenario.
- Extracted to [.NET] Better .editorconfig setup in modules/mono/ #88570.
Suppress CA1710 caused by Array not having the Collection suffix.
- Extracted to Clean a number of C# warnings #87518.
Suppress CA1716 caused by parameter names matching C# keywords.
- Extracted to Clean a number of C# warnings #87518.
Use SymbolEqualityComparer in ScriptPathAttributeGenerator to fix RS1024.
- Extracted to C#: Use SymbolEqualityComparer #87342.
Suppress dependencies of SourceGenerators package to fix NU5128.
- Extracted to C#: Suppress NU5128 warning #79501.
Suppress nullability warnings in source generators because we already have a Debug.Assert (but it's missing the nullability annotations in netstandard2.0).
- Extracted to Clean a number of C# warnings #87518.
Implement disposable pattern in GodotSynchronizationContext (and make it sealed).
- Extracted to C#: Implement disposable pattern and seal GodotSynchronizationContext class and related #72053.
Fix reference to NotificationSceneInstantiated in the documentation.
- Follow up to Rename NOTIFICATION_INSTANCED to NOTIFICATION_SCENE_INSTANTIATED #64410.
- Extracted to Fix C# examples in documentation for 4.0 #72462.
Fix RS2008: Enable analyzer release tracking.
- Implemented by C#: Add analyzer release tracking #80343.

TODO

Fix CA1001: Type owns disposable field but is not disposable.
- Array and Dictionary: This was already discussed a bit in Merge .NET 6 branch with master #64089 (comment) but I think it's not as straightforward as the rest of the fixes and it should probably be handled in a separate PR.
- Variant: It already implements IDisposable so I don't know why it complains.
  - Seems like it's a bug in the analyzer: False positive for CA1001 triggered by structs dotnet/roslyn-analyzers#6151.

modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/Godot.SourceGenerators.csproj

neikeq · 2022-09-09T19:56:44Z

modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/MustBeVariantAnalyzer.cs

@@ -47,9 +47,9 @@ private void AnalyzeNode(SyntaxNodeAnalysisContext context)
                var typeSymbol = sm.GetSymbolInfo(typeSyntax).Symbol as ITypeSymbol;
                Debug.Assert(typeSymbol != null);

-                var parentSymbol = sm.GetSymbolInfo(parentSyntax).Symbol;
+                var parentSymbol = sm.GetSymbolInfo(parentSyntax!).Symbol;


If all these come from the Debug.Assert, then I think it would be better to do "if null throw". This way, you can also include useful information if the value happen to be null.

However, if those don't come from previous assertions, I would be very careful about that. I would only do that if it's very clear the value cannot be null.

neikeq · 2022-09-09T20:01:33Z

modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/ScriptMethodsGenerator.cs

@@ -135,7 +136,9 @@ INamedTypeSymbol symbol

            source.Append("#pragma warning disable CS0109 // Disable warning about redundant 'new' keyword\n");

-            source.Append($"    public new class MethodName : {symbol.BaseType.FullQualifiedName()}.MethodName {{\n");
+            Debug.Assert(symbol.BaseType != null, "Symbol always has a base type because every script class must inherit from a Godot type; otherwise, the generator wouldn't be visiting this class.");


I really don't like the fact flow analysis can't understand the value cannot be null after this check in .NET Standard 2.0. I think throwing an exception would be better.

Actually, wouldn't this check be excluded when publishing the package?

[System.Diagnostics.Conditional("DEBUG")] public static void Assert (bool condition);

Calls to this function are only compiled in Debug configurations. It's the same as:

#if DEBUG Debug.Assert(foo); #endif

Yes, the call is excluded, that's why I'm using it in these cases because they will never be null so I wanted to avoid the unnecessary check but it might still be useful during development to notice if we're doing something wrong.

In this case, since we are filtering the classes with SelectGodotScriptClasses these classes must always inherit from at least Godot.Object so the symbol.BaseType will never be null.

neikeq · 2022-09-09T20:05:12Z

modules/mono/editor/bindings_generator.cpp

-					"'Missing XML comment for publicly visible type or member'\n");
+					"'Missing XML comment for publicly visible type or member'\n"
+					"#pragma warning disable CA1716 // Disable warning: "
+					"'Identifiers should not match keywords'\n");


I don't know how to feel about this. I would like to keep this in sight. We may want to rename those in the future. The Object one if fine to disable, because it's very obvious.

Can this be done inside an #if? If so, we could have a configuration to enable this warning, while having it disabled by default.

It can and it sounds good to me. I don't really want to suppress these warnings but solving them properly would require renaming which seems unlikely to happen in a timely manner and after the beta we won't want to break compatibility.

I've surrounded all these suppressions like so:

#if !EXTRA_WARNINGS #pragma warning disable CA1716 #endif

neikeq · 2022-09-09T20:07:45Z

modules/mono/editor/bindings_generator.cpp

-				  "#pragma warning disable CS1573 // Disable warning: "
-				  "'Parameter has no matching param tag in the XML comment'\n");
+				  "#pragma warning disable CA1716 // Disable warning: "
+				  "'Identifiers should not match keywords'\n");


Ditto. Can CS0108 and CA1716 be done inside an #if?

neikeq · 2022-09-09T20:09:33Z

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

@@ -5,6 +5,9 @@
 using System.Runtime.CompilerServices;
 using Godot.NativeInterop;

+// Suppress warning because it complaints about Array not having the Collection suffix.
+#pragma warning disable CA1710 // Identifiers should have correct suffix


Really nonsense warning. We may want to disable it directly in .editorconfig.

neikeq · 2022-09-09T20:11:02Z

modules/mono/glue/GodotSharp/GodotSharp/Core/Extensions/ObjectExtensions.cs

@@ -1,6 +1,8 @@
 using System;
 using Godot.NativeInterop;

+#pragma warning disable CA1716 // Disable warning: 'Identifiers should not match keywords'


Ditto. Can this be done inside an #if?

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

neikeq · 2022-09-09T20:18:03Z

Fix RS2008: Enable analyzer release tracking.
* Not sure how to fix this one. The quick fix in VSCode does nothing.

I think this is about https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md

neikeq · 2022-09-09T20:22:19Z

Fix CA1001: Type owns disposable field but is not disposable.
* [ ]  `Array` and `Dictionary`: This was already discussed a bit in

This won't be a simple change. It will require some design decisions.

raulsntos · 2022-09-10T00:12:43Z

Fix RS2008: Enable analyzer release tracking.
* Not sure how to fix this one. The quick fix in VSCode does nothing.
I think this is about https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md

Yes but since the quick fix doesn't seem to do anything in VSCode, should I create the markdown files manually? Suppress the warning?

Fix CA1001: Type owns disposable field but is not disposable.
* [ ]  `Array` and `Dictionary`: This was already discussed a bit in
This won't be a simple change. It will require some design decisions.

I don't intend to solve this warning in this PR but I wanted to mention it since it's somewhat related.

raulsntos · 2023-01-25T16:19:36Z

Rebased and extracted breaking changes to #72053.

- Suppress REFL045 in a false positive scenario. - Suppress CS0108 caused by shadowing members in derived classes. - Move CA1716 to pragmas in order to allow re-enabling it with a custom define.

paulloz · 2024-05-02T17:18:15Z

Can we close this one? The only remaining point is the several instances of CA1001, and this PR does not fix them. We should track that particular issue in its own thread instead.

raulsntos · 2024-05-04T02:02:11Z

Yes, this has been superseded by the multiple PRs that are now linked in the PR description.

raulsntos added bug enhancement topic:dotnet breaks compat labels Sep 8, 2022

raulsntos added this to the 4.0 milestone Sep 8, 2022

raulsntos requested a review from a team September 8, 2022 18:16

neikeq reviewed Sep 9, 2022

View reviewed changes

raulsntos force-pushed the dotnet/various-warnings branch from 3bfb090 to c58945b Compare September 10, 2022 01:26

raulsntos mentioned this pull request Jan 25, 2023

C#: Implement disposable pattern and seal GodotSynchronizationContext class and related #72053

Merged

raulsntos force-pushed the dotnet/various-warnings branch from c58945b to 69b8697 Compare January 25, 2023 16:18

raulsntos removed the breaks compat label Jan 25, 2023

akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023

YuriSizov modified the milestones: 4.1, 4.2 Jun 14, 2023

This was referenced Jul 13, 2023

C#: Fix MSVC dotnet builds failing if running dev_mode #79351

Merged

C#: Suppress NU5128 warning #79501

Merged

raulsntos modified the milestones: 4.2, 4.3 Oct 4, 2023

This was referenced Jan 18, 2024

C#: Use SymbolEqualityComparer #87342

Merged

Clean a number of C# warnings #87518

Merged

Various fixes to C# warnings

ec730d8

- Suppress REFL045 in a false positive scenario. - Suppress CS0108 caused by shadowing members in derived classes. - Move CA1716 to pragmas in order to allow re-enabling it with a custom define.

raulsntos force-pushed the dotnet/various-warnings branch from 69b8697 to ec730d8 Compare January 31, 2024 01:29

raulsntos closed this May 4, 2024

raulsntos deleted the dotnet/various-warnings branch May 4, 2024 02:02

raulsntos removed this from the 4.3 milestone May 4, 2024

raulsntos added the archived label May 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various fixes to C# warnings #65532

Various fixes to C# warnings #65532

raulsntos commented Sep 8, 2022 •

edited

Loading

neikeq Sep 9, 2022

neikeq Sep 9, 2022

neikeq Sep 9, 2022

raulsntos Sep 9, 2022 •

edited

Loading

neikeq Sep 9, 2022

neikeq Sep 9, 2022

raulsntos Sep 10, 2022 •

edited

Loading

neikeq Sep 9, 2022

neikeq Sep 9, 2022

neikeq Sep 9, 2022

neikeq commented Sep 9, 2022

neikeq commented Sep 9, 2022

raulsntos commented Sep 10, 2022

raulsntos commented Jan 25, 2023

paulloz commented May 2, 2024

raulsntos commented May 4, 2024

Various fixes to C# warnings #65532

Various fixes to C# warnings #65532

Conversation

raulsntos commented Sep 8, 2022 • edited Loading

Extracted to different PRs

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulsntos Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulsntos Sep 10, 2022 • edited Loading

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

neikeq commented Sep 9, 2022

neikeq commented Sep 9, 2022

raulsntos commented Sep 10, 2022

raulsntos commented Jan 25, 2023

paulloz commented May 2, 2024

raulsntos commented May 4, 2024

raulsntos commented Sep 8, 2022 •

edited

Loading

raulsntos Sep 9, 2022 •

edited

Loading

raulsntos Sep 10, 2022 •

edited

Loading