Trwalke/ruleset updates #3189

trwalke · 2022-02-25T04:53:41Z

Fixes #
Ruleset update

Changes proposed in this request
Enabling critical Async Threading rules
Adding ruleset from asp.net repo

trwalke · 2022-02-25T04:54:53Z

.editorconfig

+dotnet_diagnostic.VSTHRD010.severity = warning
+
+# Use AsyncLazy<T>
+dotnet_diagnostic.VSTHRD011.severity = none #error requires the use of Microsoft.VisualStudio.Threading to get access to the AsyncLazy<T> type


inorder to enable this we need to use Microsoft.VisualStudio.Threading but the package is not compatable with our .net 45 targets. same with some other older targets.

@bgavrilMS

Yeah, bummer.

trwalke · 2022-02-25T05:02:43Z

I fixed a bunch of errors already but these rules produce an extra 100+ warnings and it may be easier to disable the problematic rules but not sure. Thoughts @bgavrilMS? Should I keep going?

VSTHRD002	Avoid problematic synchronous waits	Critical	2nd rule	Warning
VSTHRD003	Avoid awaiting foreign Tasks	Critical	3rd rule	Warning

Reference Here

src/client/Microsoft.Identity.Client/ITokenCache.cs

bgavrilMS · 2022-02-25T13:49:05Z

src/client/Microsoft.Identity.Client.Desktop/WamInterop.cs

@@ -14,20 +14,20 @@ namespace Microsoft.Identity.Client.Platforms
 {
    internal static class WebAuthenticationCoreManagerInterop
    {
-        public static IAsyncOperation<WebTokenRequestResult> RequestTokenForWindowAsync(IntPtr hWnd, WebTokenRequest request)


These operations are async, but not in the traditional "way", but in the Windows way. You can create your own async API which can be await-ed by simply implementing an interface. The "async" suffix should be here. Please disable the rule for this file.

bgavrilMS · 2022-02-25T13:50:21Z

tests/devapps/XamarinDev/XamarinDev/AcquirePage.xaml.cs

@@ -190,7 +190,7 @@ private async void OnAcquireSilentlyClickedAsync(object sender, EventArgs e)
            }
        }

-        private async void OnAcquireClickedAsync(object sender, EventArgs e)
+        private async Task OnAcquireClickedAsync(object sender, EventArgs e)


Does the app continue to work? I was under the impression that async event handlers cannot use Task. This is the whole reason why we still have async void allowed in the language.

gladjohn · 2022-03-02T16:51:11Z

src/client/Microsoft.Identity.Client/Cache/Keys/MsalCacheKeys.cs

@@ -61,7 +61,6 @@ public static string GetiOSAccountKey(string homeAccountId, string environment)
            return stringBuilder.ToString().ToLowerInvariant();
        }

-
        public static string GetiOSServiceKey(


no changes here, same with some many other files.

gladjohn · 2022-03-02T16:53:50Z

src/client/Microsoft.Identity.Client/Platforms/net5win/Net5WamInterop.cs

-                requestPtr, 
-                webAccountPtr, 
-                ref guid, 
+                requestPtr,


was this edit based on a rule?

gladjohn · 2022-03-02T17:00:05Z

tests/devapps/WebApi/Controllers/S2SAndOboController.cs

        [HttpGet]
-#pragma warning disable UseAsyncSuffix // Use Async suffix
+        [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "VSTHRD200:Use \"Async\" suffix for async methods", Justification = "<Pending>")]


few pending justifications?

bgavrilMS · 2022-03-03T10:32:02Z

This is a huge PR that touches a lot of files, so it must be closed quickly, otherwise merging will be hell.

trwalke · 2022-03-09T16:53:16Z

src/client/Microsoft.Identity.Client/Microsoft.Identity.Client.csproj

@@ -140,6 +140,7 @@
    <Compile Include="$(PathToMsalSources)\**\*.cs" Exclude="$(PathToMsalSources)\obj\**\*.*" />
    <Compile Remove="$(PathToMsalSources)\Platforms\**\*.*;$(PathToMsalSources)\Resources\*.cs" />
    <Compile Remove="$(PathToMsalSources)\PlatformsCommon\PlatformNotSupported\ApiConfig\SystemWebViewOptions.cs" />
+    <Compile Remove="GlobalSuppressions3.cs" />


Probably dont need this?

trwalke · 2022-03-10T05:59:28Z

Getting some strange errors around .net 4.8 after merging in master. Are you aware of any changes to .net 48?

bgavrilMS · 2022-03-10T10:35:35Z

@trwalke - yeah, I'm trying to convert all our dev apps to net48, since net47 and net461 are no longer installed by default with VS.

pmaytak

Main comment is that it could be cleaner to disable some rules in the test editorconfig, instead of adding pragma disable or suppression files. Also can maybe add editorconfigs to different folders, since they are hierarchical.

This should also be merged right after the WAM bug fix PR before any other ones.

pmaytak · 2022-03-14T23:47:26Z

.editorconfig

+dotnet_diagnostic.VSTHRD110.severity = none
+
+#------------------------------------------------------------------------------------------------------------------------------------------------------------#
+# Rules from aspnet


Add a link here to the page with rules descriptions. I think it's this one, https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/, but may be something else.

pmaytak · 2022-03-14T23:51:26Z

.editorconfig

+dotnet_diagnostic.CA1507.severity = warning
+
+# CA1725: Parameter names should match base declaration
+dotnet_diagnostic.CA1725.severity = suggestion


Should probably be a warning or error for public API. But should probably be left for a seprate PR.

pmaytak · 2022-03-14T23:55:41Z

.editorconfig

+
+# IDE0036: Order modifiers
+csharp_preferred_modifier_order = public,private,protected,internal,static,extern,new,virtual,abstract,sealed,override,readonly,unsafe,volatile,async:suggestion
+dotnet_diagnostic.IDE0036.severity = warning


Maybe set this to none (or at least for now), I don't think we follow the order and might create a lot of warnings.

pmaytak · 2022-03-14T23:59:14Z

.editorconfig

+dotnet_style_allow_multiple_blank_lines_experimental = false
+dotnet_diagnostic.IDE2000.severity = warning
+
+# CA1018: Mark attributes with AttributeUsageAttribute


These seem to be duplicates of ones above.

pmaytak · 2022-03-15T00:11:29Z

tests/devapps/DesktopTestApp/GlobalSuppressions.cs

@@ -0,0 +1,8 @@
+// This file is used by Code Analysis to maintain SuppressMessage


Instead of GlobalSuppressions and pragma disable in code, can these rules be disabled in tests editorconfig for all tests?

…library-for-dotnet * 'master' of github.com:AzureAD/microsoft-authentication-library-for-dotnet: (39 commits) OBO for SP with RT support (AzureAD#3266) (AzureAD#3280) Update some projects to net48 (AzureAD#3269) Bump Source Link version (tool change) (AzureAD#3275) Update changelog.txt (AzureAD#3282) Default to WebView1 instead of WebView2 when using AAD or ADFS authorities (AzureAD#3276) Revert "OBO for SP with RT support (AzureAD#3266)" OBO for SP with RT support (AzureAD#3266) Add perf tests for Acquire Token calls with serialization cache and Builders (AzureAD#3250) Minor fix to SuggestedCacheKey comment and update NuGet icon in Readme. (AzureAD#3268) MSAL changelog 4.43.0 (AzureAD#3263) Bogavril/3251 (AzureAD#3255) Fix for AzureAD#3248 - use the correct plugin to sing out (AzureAD#3253) Performance project improvements (AzureAD#3064) Reuse lists in token cache filtering logic. (AzureAD#3233) Update andorid install script to use andorid 30 (AzureAD#3243) Fix for UWP packaging (AzureAD#3239) Pass account to Auth result (AzureAD#3199) App ported to Lab4 (AzureAD#3229) Trwalke/ruleset updates (AzureAD#3189) Conditional Access for Android (AzureAD#3210) ...

trwalke commented Feb 25, 2022

View reviewed changes

src/client/Microsoft.Identity.Client/ITokenCache.cs Show resolved Hide resolved

bgavrilMS reviewed Feb 25, 2022

View reviewed changes

gladjohn reviewed Mar 2, 2022

View reviewed changes

trwalke commented Mar 9, 2022

View reviewed changes

Static anlyzer rules around async

ed09a75

bgavrilMS force-pushed the trwalke/ruleset_updates branch from 088a2ba to ed09a75 Compare March 11, 2022 14:55

fix

374ed1c

bgavrilMS marked this pull request as ready for review March 11, 2022 17:19

bgavrilMS approved these changes Mar 11, 2022

View reviewed changes

pmaytak self-requested a review March 14, 2022 19:29

pmaytak approved these changes Mar 15, 2022

View reviewed changes

bgavrilMS merged commit 5e29d34 into master Mar 15, 2022

bgavrilMS deleted the trwalke/ruleset_updates branch March 15, 2022 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trwalke/ruleset updates #3189

Trwalke/ruleset updates #3189

trwalke commented Feb 25, 2022

trwalke Feb 25, 2022

bgavrilMS Mar 2, 2022

trwalke commented Feb 25, 2022 •

edited

bgavrilMS Feb 25, 2022

bgavrilMS Feb 25, 2022

gladjohn Mar 2, 2022

gladjohn Mar 2, 2022

gladjohn Mar 2, 2022

bgavrilMS commented Mar 3, 2022

trwalke Mar 9, 2022

trwalke commented Mar 10, 2022

bgavrilMS commented Mar 10, 2022

pmaytak left a comment

pmaytak Mar 14, 2022

pmaytak Mar 14, 2022

pmaytak Mar 14, 2022

pmaytak Mar 14, 2022

pmaytak Mar 15, 2022

		@@ -0,0 +1,8 @@
		// This file is used by Code Analysis to maintain SuppressMessage

Trwalke/ruleset updates #3189

Trwalke/ruleset updates #3189

Conversation

trwalke commented Feb 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trwalke commented Feb 25, 2022 • edited

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

Choose a reason for hiding this comment

bgavrilMS commented Mar 3, 2022

Choose a reason for hiding this comment

trwalke commented Mar 10, 2022

bgavrilMS commented Mar 10, 2022

pmaytak left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trwalke commented Feb 25, 2022 •

edited