feat: Roslyn Diagnostics Analyzer for empty constructor #2104

IXLLEGACYIXL · 2024-01-13T20:30:31Z

PR Details

Adds a Warning for not having an Empty Constructor

Description

Diagnostics Analyzer for #2098 to throw at Compile Time a warning when an empty constructor is missing.

Types of changes

Docs change / refactoring / dependency upgrade ( i have to make with vaso a new diagnostics website )
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My change requires a change to the documentation.
I have added tests to cover my changes.
All new and existing tests passed. ( 66 fail as same with master )
I have built and run the editor to try this change out.

Kryptos-FR · 2024-01-13T21:16:11Z

sources/targets/Stride.Core.targets

-  <!-- Disable analyzers unless StrideEnableCodeAnalysis is set (note: PackageReference can't depend on external Condition) -->
-  <Target Name="DisableAnalyzersForStrideBuild"
-        BeforeTargets="CoreCompile"
-        Condition="'$(StrideEnableCodeAnalysis)' != 'true'">
-    <ItemGroup>
-      <!-- We want to include Stride analyzers by default -->
-      <AnalyzersNotToRemove Include="@(Analyzer)" Condition="$([System.String]::Copy(%(FullPath)).Contains('Stride'))" />
-      <!-- We want to include source generators by default -->
-      <AnalyzersNotToRemove Include="@(Analyzer)" Condition="$([System.String]::Copy(%(FullPath)).Contains('Generator'))" />
-      <AnalyzersNotToRemove Include="@(Analyzer)" Condition="$([System.String]::Copy(%(FullPath)).Contains('SourceGeneration'))" />
-      <AnalyzersToRemove Include="@(Analyzer)" Exclude="@(AnalyzersNotToRemove)" />
-      <Analyzer Remove="@(AnalyzersToRemove)"/>
-    </ItemGroup>
-  </Target>


This shouldn't be removed.

it doesnt make a difference, stride compiles fine without it

i dont know why it is there , do you know why it is there?
i can launch my game with it just fine

Don't make changes that are not needed. It was there for a reason, and unless your changes require it to be removed it shouldn't. A PR should have the minimal changes that are required for a given feature, nothing more.

If there is an argument to be made about removing that part, it should be on its own PR and discussed there.
If as you said "it doesn't make a difference", then leave it.

I believe one of the reason this was added was to speed-up the build by disabling analysis that is not required (esp. during CI), as well as removing a lot of warnings that would make the logs harder to read in case a certain build needs investigating.

After discussion on Discord, it appears that the Stylecop settings might be some Xenko leftovers. Maybe @xen2 can shine a light on this. But basically:

we are adding a package reference for Stylecop assembly for all projects that have <StrideCodeAnalysis>true</StrideCodeAnalysis>. This seems to be most core and engine projects (not applied to editor).

since these generates a lot of warnings in the build log, those analyzers are disabled by default (unless StrideEnableCodeAnalysis is also set to true.

however, some analyzers/generators still need to be enabled at all time. Hence this "complex" rule with a allow-list of analyzers to not remove.

We have two options:

Either we keep the stylecop rule, in which case we can add another exception for PolySharp in that "allow-list".

We decide we don't want stylecop anymore and we just remove all of that.
a. we do keep the first StrideCodeAnalysis section to use the proper ruleset

currently stylecop is added to each project, but no project has the StrideEnableCodeAnalysis so it gets removed immediately after

I think it was initially meant as an optional command line parameters when running a build.
Something like: msbuild -p:StrideEnableCodeAnalysis=true ...

Anyway, if #2105 is approved and merged, we won't need to discuss it anymore 😅

Kryptos-FR · 2024-01-13T21:16:22Z

sources/core/Stride.Core.CompilerServices/Stride.Core.CompilerServices.csproj

+    <PackageReference Include="PolySharp">
+      <PrivateAssets>all</PrivateAssets>
+      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
+    </PackageReference>
+


Where is it used? Is it required at runtime?

https://github.com/stride3d/stride/pull/2104/files#diff-015a4d048579b446685ff8c8dbddb5302ff100cd3fbfc1df1cef57c74a4032b6R66

its used here for pattern matching as i got told that i should do it that way ( funny thing is that this part is the only one that doesnt work )

Patter-matching is a feature of the language, it doesn't require an additional dependency. Your code seems to compile without Polysharp what does it add?

System.Index and System.Index.GetOffset

ive changed it to a foreach loop now and added the analyzer removal back in

Just to keep a trace of what we discussed on Discord:

netstandard2.0 is a requirement for analyzers/source generators. Nothing we can do about.

because of that, certain languages features aren't available (like index/spread operations on collections in C# 12), unless we use a library such as PolySharp to fill the gaps.

sources/core/Stride.Core.CompilerServices/Stride.Core.CompilerServices.csproj

sources/core/Stride.Core.CompilerServices.Tests/Stride.Core.CompilerServices.Tests.csproj

Kryptos-FR · 2024-01-15T09:40:09Z

Note: make sure to validate and merge #2105 first.

IXLLEGACYIXL · 2024-01-15T10:03:53Z

for the tests, i freshly cloned stride and now im back to the standard 24 tests that fail normally, so i just had a corrupted clone

that is fine

sources/core/Stride.Core.CompilerServices/Analyzers/STRDIAG010InvalidConstructor.cs

IXLLEGACYIXL · 2024-04-23T12:55:29Z

i will reopen when i know whats wrong with git lfs

This reverts commit 79e993f.

IXLLEGACYIXL · 2024-04-28T17:05:14Z

i think its ready to merge, should i change the name of the PR to use the new tags

Eideren

Looks great, thanks, just two small notes

sources/targets/Stride.Core.targets

sources/core/Stride.Core.CompilerServices/Common/SymbolExtensions.cs

IXLLEGACYIXL · 2024-05-21T21:42:59Z

completed the changes

IXLLEGACYIXL · 2024-05-21T21:51:20Z

the name of the PR should be changed to meet the tags for the new system that vaso made but i dont really know what it should be named to meet the categories

Eideren · 2024-05-30T08:51:50Z

Thanks !

IXLLEGACYIXL · 2024-05-30T12:31:04Z

stride3d/stride-docs#295
the docs PR for this analyzer

IXLLEGACYIXL added 11 commits January 9, 2024 23:03

add tryget attribute

4cda759

update packages

98076da

merge

cd73a71

revert

24f6059

add polysharp

efb3ac5

rewrite analyzer

8ddcdf8

fix dot

2b0b24e

fix dot

9ef35e5

fix dot

fda2676

use pattern matching

cd4383c

add shipped analyzers

d95fe27

IXLLEGACYIXL marked this pull request as draft January 13, 2024 20:55

Kryptos-FR reviewed Jan 13, 2024

View reviewed changes

IXLLEGACYIXL added 7 commits January 13, 2024 22:38

fix inheritance

bcbc64a

enable test

7814885

add strdiag010 test

7bb11d6

use referenced project instead of shared assembly path

3003c99

add class template

4ac4e43

use latest

1fdb3b2

impossible null

2387b1a

IXLLEGACYIXL commented Jan 13, 2024

View reviewed changes

sources/core/Stride.Core.CompilerServices.Tests/Stride.Core.CompilerServices.Tests.csproj Show resolved Hide resolved

IXLLEGACYIXL force-pushed the analyzer_empty_constructor branch from 0f37ffa to 2387b1a Compare January 13, 2024 23:29

IXLLEGACYIXL mentioned this pull request Jan 13, 2024

Remove stylecop #2105

Merged

8 tasks

IXLLEGACYIXL changed the title ~~[WIP] Analyzer empty constructor~~ Analyzer empty constructor Jan 15, 2024

manio143 reviewed Jan 15, 2024

View reviewed changes

IXLLEGACYIXL added 3 commits January 15, 2024 15:37

restrict to public

a219276

adjust message

850d4d7

add primary constructor test

db262d9

IXLLEGACYIXL added 3 commits January 15, 2024 18:02

add flipped arguments test

84c1376

inline pattern matching

4e81a8f

remove space

79e993f

IXLLEGACYIXL closed this Apr 23, 2024

IXLLEGACYIXL force-pushed the analyzer_empty_constructor branch from 79e993f to 3540261 Compare April 23, 2024 11:54

merge

05a8cda

Revert "remove space"

6b9902b

This reverts commit 79e993f.

IXLLEGACYIXL reopened this Apr 23, 2024

IXLLEGACYIXL added 3 commits April 23, 2024 15:34

remove analyzers after merge

b530e30

deleted too much

638333f

adjust style

b8240ed

IXLLEGACYIXL marked this pull request as ready for review April 28, 2024 17:06

Eideren reviewed Apr 28, 2024

View reviewed changes

sources/targets/Stride.Core.targets Outdated Show resolved Hide resolved

sources/core/Stride.Core.CompilerServices/Common/SymbolExtensions.cs Outdated Show resolved Hide resolved

IXLLEGACYIXL and others added 3 commits May 21, 2024 20:07

fix: add maybenull

d8103fc

Merge branch 'stride3d:master' into analyzer_empty_constructor

f7d5642

fix: remove spaces

92dba62

VaclavElias added area-Core Issue of the engine unrelated to other defined areas engineering and removed area-Core Issue of the engine unrelated to other defined areas labels May 23, 2024

IXLLEGACYIXL changed the title ~~Analyzer empty constructor~~ engineering : Roslyn Diagnostics Analyzer for empty constructor May 23, 2024

IXLLEGACYIXL changed the title ~~engineering : Roslyn Diagnostics Analyzer for empty constructor~~ Roslyn Diagnostics Analyzer for empty constructor May 23, 2024

IXLLEGACYIXL changed the title ~~Roslyn Diagnostics Analyzer for empty constructor~~ feat: Roslyn Diagnostics Analyzer for empty constructor May 23, 2024

IXLLEGACYIXL mentioned this pull request May 26, 2024

feat: add STRDIAG010 docs stride3d/stride-docs#295

Merged

Eideren merged commit 4cf0d08 into stride3d:master May 30, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Roslyn Diagnostics Analyzer for empty constructor #2104

feat: Roslyn Diagnostics Analyzer for empty constructor #2104

IXLLEGACYIXL commented Jan 13, 2024 •

edited

Loading

Kryptos-FR Jan 13, 2024

IXLLEGACYIXL Jan 13, 2024 •

edited

Loading

Kryptos-FR Jan 13, 2024

Kryptos-FR Jan 13, 2024 •

edited

Loading

IXLLEGACYIXL Jan 13, 2024

Kryptos-FR Jan 14, 2024

Kryptos-FR Jan 13, 2024

IXLLEGACYIXL Jan 13, 2024 •

edited

Loading

Kryptos-FR Jan 13, 2024

IXLLEGACYIXL Jan 13, 2024

Kryptos-FR Jan 13, 2024

Kryptos-FR commented Jan 15, 2024

IXLLEGACYIXL commented Jan 15, 2024

IXLLEGACYIXL commented Apr 23, 2024

IXLLEGACYIXL commented Apr 28, 2024

Eideren left a comment

IXLLEGACYIXL commented May 21, 2024

IXLLEGACYIXL commented May 21, 2024

Eideren commented May 30, 2024

IXLLEGACYIXL commented May 30, 2024

feat: Roslyn Diagnostics Analyzer for empty constructor #2104

feat: Roslyn Diagnostics Analyzer for empty constructor #2104

Conversation

IXLLEGACYIXL commented Jan 13, 2024 • edited Loading

PR Details

Description

Types of changes

Checklist

Choose a reason for hiding this comment

IXLLEGACYIXL Jan 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kryptos-FR Jan 13, 2024 • 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

IXLLEGACYIXL Jan 13, 2024 • 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

Kryptos-FR commented Jan 15, 2024

IXLLEGACYIXL commented Jan 15, 2024

IXLLEGACYIXL commented Apr 23, 2024

IXLLEGACYIXL commented Apr 28, 2024

Eideren left a comment

Choose a reason for hiding this comment

IXLLEGACYIXL commented May 21, 2024

IXLLEGACYIXL commented May 21, 2024

Eideren commented May 30, 2024

IXLLEGACYIXL commented May 30, 2024

IXLLEGACYIXL commented Jan 13, 2024 •

edited

Loading

IXLLEGACYIXL Jan 13, 2024 •

edited

Loading

Kryptos-FR Jan 13, 2024 •

edited

Loading

IXLLEGACYIXL Jan 13, 2024 •

edited

Loading