Add workflow to automatically check code style issues for PRs

[TSRBerry]

This PR was originally planned to contain a lot more things, but formatting already took a while and I didn't expect our codebase to report so many issues (via dotnet-format).

A new workflow will now be triggered for every PR which will run dotnet-format to check for style issues.
If the workflow doesn't succeed the author of the PR will need to make some style changes, before we are able to merge the PR. The issues dotnet-format found will be uploaded as an artifact but also printed to the log of the workflow run.

This allows reviewers to focus more on actual code review instead of adding a lot of comments about minor styling issues. This will hopefully make reviews a little bit easier and authors still get feedback about code styling with the workflow.

I was able to automate silencing some of the issues dotnet-format reported with this script.

---

Since this PR required a few other code changes to make dotnet-format happy, feel free to suggest improvements/changes to those as well.

Feedback about this approach would also be appreciated!

---

List of code style/quality rules we need to discuss:

• IDE0044: Add readonly modifier

• IDE0051: Remove unused private member

• IDE0052: Remove unread private member

• IDE0059: Remove unnecessary value assignment

• IDE0060: Remove unused parameter

• IDE0066: Use switch expression

• IDE1006: Naming rule violation

• CS0169: The private field 'class member' is never used

• CS0414: The private field 'field' is assigned but its value is never used

• CS0649: Field 'field' is never assigned to, and will always have its default value 'value'

• CA1822: Mark members as static

• CA1834: Use StringBuilder.Append(char) for single character strings

[gdkchan]

I took a quick look, didn't look at everything yet since the diff is pretty large.

It might be easier to review if the change moving everything to a src sub-directory is done on a separate PR. GitHub is currently showing this:

The diff you're trying to view is too large. We only load the first 3000 changed files.

So I guess it is not showing all the changes.

About the style changes, the only one I'm not sure about is the new() change. I'm not sure if we should be enforcing it everywhere, it sounds like something that would come up quite often. There are a few other changes that I would probably take a while to get used to, but I don't think they would be triggered often enough to annoy me.

There are a few warnings that seems to be disabled with pragmas quite often (like IDE0066). I didn't actually look at the number of times we need to disable it, but if it happens too often, maybe it's better to just set csharp_style_prefer_switch_expression = false on the editorconfig to avoid needing to disable it many times.

[TSRBerry]

About the style changes, the only one I'm not sure about is the new() change. I'm not sure if we should be enforcing it everywhere, it sounds like something that would come up quite often. There are a few other changes that I would probably take a while to get used to, but I don't think they would be triggered often enough to annoy me.

There are a few warnings that seems to be disabled with pragmas quite often (like IDE0066). I didn't actually look at the number of times we need to disable it, but if it happens too often, maybe it's better to just set csharp_style_prefer_switch_expression = false on the editorconfig to avoid needing to disable it many times.

My intention was to enforce the existing rules from editorconfig, but also consider how we address styling in PRs currently. If I made changes here which don't reflect the code style of Ryujinx, I should revert them and adapt editorconfig to match the code style instead.

[github-actions]

Download the artifacts for this pull request:

• ryujinx-Release-1.1.0+2ad213f-linux_x64
• ryujinx-Release-1.1.0+2ad213f-win_x64

Experimental GUI (Avalonia)

• ava-ryujinx-Release-1.1.0+2ad213f-linux_x64
• ava-ryujinx-Release-1.1.0+2ad213f-macos_universal
• ava-ryujinx-Release-1.1.0+2ad213f-win_x64

GUI-less (SDL2)

• sdl2-ryujinx-headless-Release-1.1.0+2ad213f-linux_x64
• sdl2-ryujinx-headless-Release-1.1.0+2ad213f-win_x64

Only for Developers

• ava-ryujinx-Debug-1.1.0+2ad213f-linux_x64
• ava-ryujinx-Debug-1.1.0+2ad213f-macos_universal
• ava-ryujinx-Debug-1.1.0+2ad213f-win_x64
• ryujinx-Debug-1.1.0+2ad213f-linux_x64
• ryujinx-Debug-1.1.0+2ad213f-win_x64
• sdl2-ryujinx-headless-Debug-1.1.0+2ad213f-linux_x64
• sdl2-ryujinx-headless-Debug-1.1.0+2ad213f-win_x64

[gdkchan]

My intention was to enforce the existing rules from editorconfig, but also consider how we address styling in PRs currently. If I made changes here which don't reflect the code style of Ryujinx, I should revert them and adapt editorconfig to match the code style instead.

I was talking about cases where #pragma is being used to disable warnings around specific code. If we need to do that, then we are not enforcing the style in all cases. I think if we need to do that often, it makes more sense to just change the editorconfig and disable the code style rule, rather than littering the code with #pragmas.

About the switch expression change, I like switch expressions, but I'm not a fan of switch expression with or patterns, especially the automatic fix that seems to make rather large chains of ors in the same line which makes it quite long and ugly. It would be nice if we could only enable it for the trivial cases, but I don't think this is possible.

[gdkchan]

Unrelated to the discussion above, but it would be nice if we could catch naming violations too. For example, I can write a method like public static void DO_SOMETHING() which does not match the naming we use for methods at all, but it would not flag that as invalid. I think there is an analyser that can detect that, but there's no automatic fix.

[TSRBerry]

I was talking about cases where #pragma is being used to disable warnings around specific code. If we need to do that, then we are not enforcing the style in all cases. I think if we need to do that often, it makes more sense to just change the editorconfig and disable the code style rule, rather than littering the code with #pragmas.

Ah I misunderstood then! I didn't want to change too many things at once, so I added the #pragmas first, so we could discuss whether we want to disable these rules or enforce them properly.

(I should have kept track of them and added a list here, oh well.)

About the switch expression change, I like switch expressions, but I'm not a fan of switch expression with or patterns, especially the automatic fix that seems to make rather large chains of ors in the same line which makes it quite long and ugly. It would be nice if we could only enable it for the trivial cases, but I don't think this is possible.

Hmm, I'll have to think about this some more, but it doesn't sound impossible to me, since analyzers should be able to detect logical patterns. We might be able to add an analyzer to this.

I think there is an analyser that can detect that, but there's no automatic fix.

It's surprising there isn't but we might as well add one. I think it could save us a lot of work too.

[gdkchan]

Ah I misunderstood then! I didn't want to change too many things at once, so I added the #pragmas first, so we could discuss whether we want to disable these rules or enforce them properly.

That's fair, I guess let others chime in so that we can decide what we should enforce and what we should disable on the .editorconfig.

Hmm, I'll have to think about this some more, but it doesn't sound impossible to me, since analyzers should be able to detect logical patterns. We might be able to add an analyzer to this.

Maybe a bit overkill to have our own analyzer just to enforce switch expressions to be used in some cases.

It's surprising there isn't but we might as well add one. I think it could save us a lot of work too.

There is an issue on the Roslyn tracking the implementation of one (dotnet/roslyn#14983), but who knows when it will be done.

[TSRBerry]

Maybe a bit overkill to have our own analyzer just to enforce switch expressions to be used in some cases.

It sounds fun to implement (currently), so I might add something to Ryujinx.CustomTasks if you don't mind! :D