Prefer ExactSpelling=true for known extern methods #4130

Mrnikbobjeff · 2020-09-04T16:07:15Z

Implementation of #35695 the warning level was not explicitely stated as far as I can tell. I also extended the proposal to look at the EntryPoint parameter, as we would probably not want to flag
[DllImport("user32.dll", EntryPoint = "CallMsgFilterA", ExactSpelling = true)] public XXX CallMsgFilter(...)

codecov · 2020-09-04T17:53:02Z

Codecov Report

Merging #4130 into master will increase coverage by 0.01%.
The diff coverage is 93.35%.

@@            Coverage Diff             @@
##           master    #4130      +/-   ##
==========================================
+ Coverage   95.78%   95.80%   +0.01%     
==========================================
  Files        1163     1173      +10     
  Lines      262725   266697    +3972     
  Branches    15829    16077     +248     
==========================================
+ Hits       251655   255508    +3853     
- Misses       9064     9130      +66     
- Partials     2006     2059      +53

Mrnikbobjeff · 2020-09-04T18:24:14Z

Another question left open is if we want to include name manged functions. excluding them should be easy, and in my analysis no name mangled function exhibited behaviour which we want to catch with this analyzer @jkotas

...s/Microsoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisTest.cs

...Microsoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApis.Fixer.cs

Mrnikbobjeff · 2020-09-07T16:20:54Z

@Evangelink thank you for the review, I believe I adressed all points which were not directly responded to and unclear.

…nto feature/ExactSpellingAnalyzer # Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

...Microsoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApis.Fixer.cs

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

...s/Microsoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisTest.cs

.../Core/Microsoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApis.cs

Evangelink

A couple more cosmetic changes and then it's ok on my side.

Tagging @mavasani for extra review.

...oft.NetCore.Analyzers/InteropServices/CSharpPreferIsExactSpellingIsTrueForKnownApis.Fixer.cs

.../Core/Microsoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApis.cs

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

Mrnikbobjeff · 2020-09-08T10:18:02Z

Thank you @Evangelink for the detailed review. For future PR's I have a way better understanding of desired formatting and can adapt accordingly. The remaining issues should be fixed now

mavasani · 2020-09-08T14:40:42Z

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

+            var str = MicrosoftNetCoreAnalyzersResources.ResourceManager.GetObject("KnownApiList", CultureInfo.InvariantCulture);
+            using var stringReader = new StringReader(str.ToString());
+            using var reader = XmlReader.Create(stringReader);
+            var knownApis = (KnownApi[])serializer.Deserialize(reader);


Why are we doing all these XML reader shenanigans? Can we please hard code the known APIs in source, just like all other analyzers do?

Well there are currently 160k entries for known methods. I initially used the new generator feature to generate a compile time initialised dictionary with the constants, but the resulting compile time was an order of magnitude larger than compiling without the generated dictionary

Well there are currently 160k entries.

We would absolutely need performance measurements with this analyzer enabled then. It seems the initial load be pretty expensive?

Yes the initial load is the heaviest part, it is below half a second though and once its done it is very cheap to query.

Excluding name mangled symbols this would shring to 96k method symbols

mavasani · 2020-09-08T14:41:46Z

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

+    {
+        [Serializable]
+#pragma warning disable CA1034 // Nested types should not be visible
+        public class KnownApi //Only made public as XmlSerializer requires it


Please delete and hard code the dictionary in source.

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

mavasani

Marking as request changes till we figure out the branching for post .NET5 analyzers. Tagging @jmarolf and @jeffhandley

Mrnikbobjeff · 2020-09-09T06:59:43Z

Marking as request changes till we figure out the branching for post .NET5 analyzers. Tagging @jmarolf and @jeffhandley

Once this is established I have branches already implemented for all analyers marked as api-approved in the dotnet/runtime repository with the feedback from this branch already ported. I would hold of sending a pr until this is figured out.

jmarolf · 2020-09-14T18:56:44Z

@mavasani which release is this intended for? .NET 5 or .NET 6 Preview 1?

mavasani · 2020-09-14T19:11:35Z

@jmarolf I believe all new .NET SDK API analyzers are now for post .NET5. @jeffhandley to confirm.

jeffhandley · 2020-09-15T23:37:44Z

I believe all new .NET SDK API analyzers are now for post .NET5.

That is correct. New analyzers should target 6.0 at this point.

jmarolf · 2020-09-15T23:56:52Z

alright then this getting merged into master is the correct thing. @mavasani can you unblock this merging?

Dismissing my block, I am hoping to review the PR later this week.

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

mavasani · 2020-10-12T22:46:49Z

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

+                return;
+            }
+
+            if (methods.Contains(methodName))


@jeffhandley I think someone from the runtime team should review the core analyzer semantics in terms of the analyzer implementation matching the recommended API usage.

It seems missing some scenarios which i commented here , but i would defer to @elinor-fung for final expected scenarios

Mrnikbobjeff · 2020-10-13T08:20:28Z

@mavasani I integrated your formatting and spelling changes. The null checks you suggested are all redundant though, as we would check non-nullable value types for null. We have to check on their reference type properties, but these checks are already there

...oft.NetCore.Analyzers/InteropServices/CSharpPreferIsExactSpellingIsTrueForKnownApis.Fixer.cs

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

...s/Microsoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisTest.cs

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

mavasani

Please retarget this PR to https://github.com/dotnet/roslyn-analyzers/tree/release/6.0.1xx-preview1

buyaa-n · 2020-12-07T23:27:19Z

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

+            {
+                return;
+            }
+


Suggested change

if (isExactSpelling) // Bail out

return;

}

buyaa-n · 2020-12-07T23:30:54Z

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

+                if (isExactSpelling) //Bail out if known method has parameter set to true
+                    return;


Suggested change

if (isExactSpelling) //Bail out if known method has parameter set to true

return;

buyaa-n · 2020-12-07T23:31:15Z

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

+                    return;
+                }
+
+                if (!isCharSetUnicode && !isExactSpelling && methodName.EndsWith("A", StringComparison.Ordinal))


Suggested change

if (!isCharSetUnicode && !isExactSpelling && methodName.EndsWith("A", StringComparison.Ordinal))

if (!isCharSetUnicode && methodName.EndsWith("A", StringComparison.Ordinal))

buyaa-n · 2020-12-08T00:05:46Z

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

+                }
+            }
+
+            if (methods.Contains(methodName + "A") && !isCharSetUnicode)


I think this should be within the search condition above (row 138 if (methods.Contains(methodName))) and seems it is missing scenarios with W suffix @jkotas suggested :

When we see "Xxxx" in the entrypoint, the ExactSpelling autofixer needs to decide whether to suggest:

"XxxxW" + ExactSpelling=true. This should be suggested when we see "XxxxW" in the database.
"Xxxx" + ExactSpelling=true. This should be suggested when we see "Xxxx" and no "XxxxW" in the database
Leave the API alone. This should be the case when neither "Xxxx" nor "XxxxW" is in the database

If methodName exist in DB (already covered with if (methods.Contains(methodName)) condition):

If methodName + "W" exist we suggest adding "W" suffix and setting ExactSpelling=true

If methodName + "W" does not exist we suggest only setting ExactSpelling=true

Probably we want to do same for "A" suffix but from from @elinor-fung's comment seems suggesting ExactSpelling=true when CharSet=CharSet.Ansi might not important or might not much helpful, if we want to cover it the rule should be same

If methodName + "A" exist we suggest adding "A" suffix and setting ExactSpelling=true

If methodName + "A" does not exist we suggest only setting ExactSpelling=true

buyaa-n · 2020-12-08T00:07:40Z

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs

+
+            if (methods.Contains(methodName))
+            {
+                if (isCharSetUnicode && !isExactSpelling && methodName.EndsWith("W", StringComparison.Ordinal))


!isExactSpelling better to be moved before DB search ( before if (methods.Contains(methodName))), here and all other occurrences below

Suggested change

if (isCharSetUnicode && !isExactSpelling && methodName.EndsWith("W", StringComparison.Ordinal))

if (isCharSetUnicode && methodName.EndsWith("W", StringComparison.Ordinal))

mavasani · 2021-05-20T17:00:41Z

PR needs to be retargeted to release\6.0.xx branch. Do we still plan to target this PR for .NET6 release?

buyaa-n · 2022-02-15T18:39:02Z

Closing this PR as there were no activity for a while, @Mrnikbobjeff feel free to reopen the PR after the feedbacks addressed and when its ready for review

Schilli, Niklas added 2 commits September 4, 2020 17:44

Initial commit

6eaf0d1

comments

ff7cbc3

Mrnikbobjeff requested a review from a team as a code owner September 4, 2020 16:07

Schilli, Niklas added 4 commits September 4, 2020 18:09

removed unused locals

1118a4c

Formatting and increasing test covereage

6c2fb3e

This is known to be true due to analyzer checks

8409990

Removed already checked statements in fixer

dd9cf49

Removed unused parameters

33bc260

Renaming of variables

b5cd301

jkotas reviewed Sep 4, 2020

View reviewed changes

...s/Microsoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisTest.cs Outdated Show resolved Hide resolved

Schilli, Niklas added 2 commits September 6, 2020 10:11

All cases should be covered now, increasing test coverage some more

f0c4a0a

Spacing

9de9282

Evangelink reviewed Sep 7, 2020

View reviewed changes

Code review

937c7a1

Merge branch 'master' of https://github.com/dotnet/roslyn-analyzers i…

7966838

…nto feature/ExactSpellingAnalyzer # Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

Evangelink reviewed Sep 7, 2020

View reviewed changes

Schilli, Niklas added 2 commits September 7, 2020 20:01

Second review

5fe70b9

Regenerate XLF

25e8ee1

Evangelink reviewed Sep 8, 2020

View reviewed changes

Last review, more quotes in resources, fixing test namespace

9dfa936

mavasani reviewed Sep 8, 2020

View reviewed changes

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs Outdated Show resolved Hide resolved

mavasani reviewed Sep 8, 2020

View reviewed changes

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md Show resolved Hide resolved

mavasani reviewed Sep 8, 2020

View reviewed changes

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs Outdated Show resolved Hide resolved

mavasani previously requested changes Sep 8, 2020

View reviewed changes

Schilli, Niklas added 2 commits September 8, 2020 17:36

wrong comment

b0a48c8

Changed some sealed modifiers, renaming of codeaction

b0e3e0f

mavasani reviewed Oct 12, 2020

View reviewed changes

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs Show resolved Hide resolved

mavasani reviewed Oct 12, 2020

View reviewed changes

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs Outdated Show resolved Hide resolved

mavasani reviewed Oct 12, 2020

View reviewed changes

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs Outdated Show resolved Hide resolved

mavasani reviewed Oct 12, 2020

View reviewed changes

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs Show resolved Hide resolved

mavasani reviewed Oct 12, 2020

View reviewed changes

...crosoft.NetCore.Analyzers/InteropServices/PreferIsExactSpellingIsTrueForKnownApisAnalyzer.cs Outdated Show resolved Hide resolved

mavasani reviewed Oct 12, 2020

View reviewed changes

Review

fe8e01b

Evangelink reviewed Oct 13, 2020

View reviewed changes

Review

9b1e556

mavasani suggested changes Dec 3, 2020

View reviewed changes

buyaa-n reviewed Dec 8, 2020

View reviewed changes

carlossanlop mentioned this pull request Feb 5, 2021

User Story: .NET 6.0 developers can verify more APIs for their correct usage to speed up their development dotnet/runtime#43617

Closed

Base automatically changed from master to main March 5, 2021 02:20

mavasani assigned jmarolf and buyaa-n May 20, 2021

jmarolf changed the base branch from main to release/6.0.1xx May 20, 2021 19:33

jeffhandley mentioned this pull request Aug 22, 2021

.NET 7.0 developers can verify more APIs for correct usage to speed up their development dotnet/runtime#57797

Closed

18 tasks

jeffhandley added the Area-Microsoft.CodeAnalysis.NetAnalyzers label Feb 6, 2022

buyaa-n closed this Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer ExactSpelling=true for known extern methods #4130

Prefer ExactSpelling=true for known extern methods #4130

Mrnikbobjeff commented Sep 4, 2020 •

edited

Loading

codecov bot commented Sep 4, 2020 •

edited

Loading

Mrnikbobjeff commented Sep 4, 2020

Mrnikbobjeff commented Sep 7, 2020 •

edited

Loading

Evangelink left a comment

Mrnikbobjeff commented Sep 8, 2020

mavasani Sep 8, 2020

Mrnikbobjeff Sep 8, 2020 •

edited

Loading

mavasani Sep 8, 2020

Mrnikbobjeff Sep 8, 2020 •

edited

Loading

Mrnikbobjeff Sep 8, 2020 •

edited

Loading

mavasani Sep 8, 2020

Mrnikbobjeff Sep 8, 2020

mavasani left a comment

Mrnikbobjeff commented Sep 9, 2020

jmarolf commented Sep 14, 2020

mavasani commented Sep 14, 2020

jeffhandley commented Sep 15, 2020

jmarolf commented Sep 15, 2020

mavasani Oct 12, 2020

buyaa-n Dec 8, 2020

Mrnikbobjeff commented Oct 13, 2020

mavasani left a comment

buyaa-n Dec 7, 2020

buyaa-n Dec 7, 2020

buyaa-n Dec 7, 2020

buyaa-n Dec 8, 2020

buyaa-n Dec 8, 2020

mavasani commented May 20, 2021

buyaa-n commented Feb 15, 2022

+    if (isExactSpelling) // Bail out
+        return;
+    }

		if (isExactSpelling) //Bail out if known method has parameter set to true
		return;

	if (!isCharSetUnicode && !isExactSpelling && methodName.EndsWith("A", StringComparison.Ordinal))
	if (!isCharSetUnicode && methodName.EndsWith("A", StringComparison.Ordinal))

	if (isCharSetUnicode && !isExactSpelling && methodName.EndsWith("W", StringComparison.Ordinal))
	if (isCharSetUnicode && methodName.EndsWith("W", StringComparison.Ordinal))

Prefer ExactSpelling=true for known extern methods #4130

Prefer ExactSpelling=true for known extern methods #4130

Conversation

Mrnikbobjeff commented Sep 4, 2020 • edited Loading

codecov bot commented Sep 4, 2020 • edited Loading

Codecov Report

Mrnikbobjeff commented Sep 4, 2020

Mrnikbobjeff commented Sep 7, 2020 • edited Loading

Evangelink left a comment

Choose a reason for hiding this comment

Mrnikbobjeff commented Sep 8, 2020

Choose a reason for hiding this comment

Mrnikbobjeff Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mrnikbobjeff Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Mrnikbobjeff Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

Mrnikbobjeff commented Sep 9, 2020

jmarolf commented Sep 14, 2020

mavasani commented Sep 14, 2020

jeffhandley commented Sep 15, 2020

jmarolf commented Sep 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mrnikbobjeff commented Oct 13, 2020

mavasani 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

mavasani commented May 20, 2021

buyaa-n commented Feb 15, 2022

Mrnikbobjeff commented Sep 4, 2020 •

edited

Loading

codecov bot commented Sep 4, 2020 •

edited

Loading

Mrnikbobjeff commented Sep 7, 2020 •

edited

Loading

Mrnikbobjeff Sep 8, 2020 •

edited

Loading

Mrnikbobjeff Sep 8, 2020 •

edited

Loading

Mrnikbobjeff Sep 8, 2020 •

edited

Loading