Retarget analysers from .NETStandard 2.1 to 2.0 #692
Conversation
Patonymous
force-pushed
the
analyzers-retarget
branch
from
1b6873e to
bf34e40
Compare
Test Results 35 files 136 suites 1m 1s ⏱️ Results for commit 58fcd13. ♻️ This comment has been updated with latest results. |
src/Tools/LeanCode.CodeAnalysis/NetStandard21Compatibility/System.Range.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Okay, I checked what would be needed if we don't want to import the Index and Range and not include the NotNull attributes, and I conclude that it is not worth it. This will be literally 8 line change, whereas we import almost 500 lines of code to make the 8 work. Not worth it.
...odeAnalysis/NetStandard21Compatibility/System.Diagnostics.CodeAnalysis.NullableAttributes.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This is a heavy dependency and I wonder if it is worth it. What would be needed if we remove this and Range (roughly)?
There was a problem hiding this comment.
I suggest we just add a reference to Microsoft.Bcl.Memory and that would be it.
(I also just learned that this package exists because it's fairly new.)
There was a problem hiding this comment.
Or do that, that would be okay, provided that the package would not be in preview (although that is still much better than vendoring these).
There was a problem hiding this comment.
Thinking about it more - I fear that re-defining the types or importing them from Bcl-polyfills package would be problematic. MSBuild loads the analyzers in-proc, thus it will fail if types are re-imported, or if there is a need to load two versions of the same library. :(
There was a problem hiding this comment.
It should leave the preview stage next week (or at worst next month) but I'm not sure if we should even care about it, we are using the part of it that I'd expect be stable by now.
About multiple versions: if other analyzers having their own deps is not a problem already then this one shouldn't be either. But let's give it a try and see if anything breaks 🙂
There was a problem hiding this comment.
The main problem with
if other analyzers having their own deps is not a problem already
that I have is - we don't know that. Noone uses VS, we tend not to use too many analyzers. :P
However, let's indeed test that case - i.e. let's test if two analyzers with conflicting libraries work.
There was a problem hiding this comment.
I've added the Microsoft.Bcl.Memory package reference and will test if it works with msbuild
There was a problem hiding this comment.
Remember to test the case of two analyzers with conflicting references, e.g. one uses version X and the other version Y and both are used in a single project. I am pretty sure that this particular package won't be a problem now, but it might be when the next version comes up and other analyzers decide to use it.
src/Tools/LeanCode.CodeAnalysis/NetStandard21Compatibility/MissingFileMethods.cs
Outdated
Show resolved
Hide resolved
I'd try to import |
Patonymous
force-pushed
the
analyzers-retarget
branch
from
789a047 to
9860a90
Compare
src/Tools/LeanCode.CodeAnalysis/NetStandard21Compatibility/MissingFileMethods.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 suggestion.
Files not reviewed (2)
- Directory.Build.targets: Language not supported
- src/Tools/LeanCode.CodeAnalysis/LeanCode.CodeAnalysis.csproj: Language not supported
Comments skipped due to low confidence (1)
src/Tools/LeanCode.CodeAnalysis/NetStandard21Compatibility/MissingFileMethods.cs:33
- The error message 'Path is invalid' is unclear. Consider changing it to 'The provided path is either null, empty, or contains only white-space characters.'
throw new ArgumentException("Path is invalid");
| public static class MissingStringMethods | ||
| { | ||
| public static bool Contains(this string s, string value, StringComparison comparisonType) => | ||
| s.IndexOf(value, comparisonType) >= 0; |
There was a problem hiding this comment.
The method Contains should handle null values for the value parameter to avoid potential ArgumentNullException.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Targeting .NET Standard 2.0 allows us to support
msbuildused in Visual Studio.I've attempted not to modify the analysers source code and instead provided implementations of .NET System Corelib functionalities which were missing. Taking them from official implementation wherever possible and providing my own were it was simpler.