Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[.NET] Clean diagnostic rules #88469

Merged

Conversation

paulloz
Copy link
Member

@paulloz paulloz commented Feb 18, 2024

Move the following diagnostics into static readonly fields: GD0101, GD0102, GD0103, GD0104, GD0105, GD0106, GD0107, GD0201, GD0202, GD0203, GD0301, GD0302, GD0303, GD0401, GD0402.

To be more consistent, the titles for the following diagnostics were modified: GD0101, GD0105, GD0106, GD0302, GD0303, GD0401, GD0402. A subsequent update of the documentation repo is needed.

Tests for the following diagnostics were created: GD0201, GD0202, GD0203.

@paulloz paulloz added this to the 4.3 milestone Feb 18, 2024
@paulloz paulloz requested a review from a team as a code owner February 18, 2024 00:09
@paulloz paulloz changed the title C#: Clean diagnostic rules [.NET] Clean diagnostic rules Feb 18, 2024
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for this. I think we should merge this first, then update #87253 and #88371 to follow the same style.

Comment on lines +147 to +152
context.ReportDiagnostic(Diagnostic.Create(
Common.TypeArgumentParentSymbolUnhandledRule,
typeArgumentSyntax.GetLocation(),
parentSymbol.ToDisplayString()
));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like GD0303 shouldn't be a diagnostic, I was originally throwing an exception instead but neikeq was against it because it would stop the analyzer (see #64731 (comment)).

The way I see it diagnostics are problems in user code, but this is an internal error (a bug in the generator) and it should be unreachable anyway because we should handle all the kinds of type argument symbols. So I don't think it should be a diagnostic.

I would love to hear what you think we should do here, but it's not a blocker to merge this PR.

@paulloz paulloz force-pushed the dotnet/cleaner-diagnostic-rules branch from facf876 to 87a5f48 Compare February 18, 2024 09:29
@paulloz
Copy link
Member Author

paulloz commented Feb 18, 2024

Applied your comments, and made a few more improvements:

  • Always use '[SomeAttribute]' instead of SomeAttribute
  • Always use "type parameter" instead of "type argument"
  • Fix the location of GD0401 and GD0402 to not underline the whole class in IDEs

I agree with you for GD0303, but I'm more inclined to retire it in another PR (early in the 4.4 cycle?).

@raulsntos
Copy link
Member

Always use "type parameter" instead of "type argument"

These are not the same thing though:

class MyType<T> {} // T is a type parameter.
new MyType<string>(); // string is a type argument.

So you can only add the [MustBeVariant] attribute to a type parameter. And you can only check if a type argument is Variant-compatible.

I agree with you for GD0303, but I'm more inclined to retire it in another PR

Sounds good to me.

@paulloz
Copy link
Member Author

paulloz commented Feb 18, 2024

These are not the same thing though

So GD0301 and GD0303 should say argument, and GD0302 should say parameter?

Move the following diagnostics into static readonly fields: GD0101, GD0102, GD0103, GD0104, GD0105, GD0106, GD0107, GD0201, GD0202, GD0203, GD0301, GD0302, GD0303, GD0401, GD0402.

To be more consistent, the titles for the following diagnostics were modified: GD0101, GD0105, GD0106, GD0302, GD0303, GD0401, GD0402. A subsequent update of the documentation repo is needed.

Tests for the following diagnostics were created: GD0201, GD0202, GD0203.
@raulsntos
Copy link
Member

Yes, GD0301 should say argument and GD0302 should say parameter.

I think GD0303 is a bit more confusing but I think it should say parameter. It's reported when we're unable to find the type parameter that corresponds to a type argument, because we're not handling the kind of symbol that contains the type parameter. But to be honest, I don't really care if we'll be removing it later.

@paulloz paulloz force-pushed the dotnet/cleaner-diagnostic-rules branch from 1c798ed to 5981886 Compare February 18, 2024 16:19
@paulloz
Copy link
Member Author

paulloz commented Feb 18, 2024

Should be good then.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

@akien-mga akien-mga merged commit 9f48330 into godotengine:master Feb 18, 2024
16 checks passed
@paulloz paulloz deleted the dotnet/cleaner-diagnostic-rules branch February 19, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants