Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CA1838 Avoid 'StringBuilder' parameters for P/Invokes #8113

Merged
merged 15 commits into from
Nov 11, 2022
Merged

CA1838 Avoid 'StringBuilder' parameters for P/Invokes #8113

merged 15 commits into from
Nov 11, 2022

Conversation

elachlan
Copy link
Contributor

@elachlan elachlan commented Nov 4, 2022 •
edited
Loading

Fixes #3983

Analyzer:

Work based on:

TODO:

winforms/src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Helpers/NativeMethods.vb

Lines 30 to 33 in 15ab89b

#Disable Warning CA1838 ' Avoid 'StringBuilder' parameters for P/Invokes
<DllImport("user32", CharSet:=CharSet.Auto, PreserveSig:=True, SetLastError:=True)>
Friend Shared Function GetWindowText(hWnd As IntPtr, <Out(), MarshalAs(UnmanagedType.LPTStr)> lpString As StringBuilder, nMaxCount As Integer) As Integer
#Enable Warning CA1838 ' Avoid 'StringBuilder' parameters for P/Invokes

Related: #7887

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned elachlan Nov 4, 2022
@elachlan elachlan changed the title Initial work to remove string builders from PInvoke CA1838 Avoid 'StringBuilder' parameters for P/Invokes Nov 4, 2022
elachlan
elachlan commented Nov 4, 2022
View reviewed changes
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs
@@ -10791,12 +10791,15 @@ public new bool ResizeRedraw
public new void WndProc(ref Message m) => base.WndProc(ref m);
}

private static string GetClassName(IntPtr hWnd)
private static unsafe string GetClassName(HWND hWnd)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this move to the PInvoke namespace?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is another place where we call PInvoke.GetClassName in our codebase. If this is the case, we should move this to PInvoke namespace and call to this method in the other places we are using PInvoke.GetClassName

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't move this because its a wrapper and isn't used in the other calls. I am happy to still move it. The other uses avoid string allocations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think this is a dup, feel free to remove this. I can't recall exact reasons why I copied it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since its a wrapper of the pinvoke and we don't reuse it, I think it can stay here.

@elachlan
Copy link
Contributor Author

elachlan commented Nov 4, 2022

I don't think the vb.net PInvokes are able to avoid stringbuilder. We would have to pull it into the PInvoke class and then have a wrapper that returns the needed information.

@ghost ghost added the draft draft PR label Nov 4, 2022
lonitra
lonitra reviewed Nov 4, 2022
View reviewed changes
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs
@@ -10791,12 +10791,15 @@ public new bool ResizeRedraw
public new void WndProc(ref Message m) => base.WndProc(ref m);
}

private static string GetClassName(IntPtr hWnd)
private static unsafe string GetClassName(HWND hWnd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is another place where we call PInvoke.GetClassName in our codebase. If this is the case, we should move this to PInvoke namespace and call to this method in the other places we are using PInvoke.GetClassName

JeremyKuhne
JeremyKuhne requested changes Nov 4, 2022
View reviewed changes
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a number of comments. Biggest thing is not to try and guess what the exact max allowable path will be (outside of constraining to not go over 32K characters for sanity).

@ghost ghost added waiting-author-feedback The team requires more information from the author and removed waiting-author-feedback The team requires more information from the author labels Nov 4, 2022
@ghost ghost added waiting-author-feedback The team requires more information from the author and removed waiting-author-feedback The team requires more information from the author labels Nov 9, 2022
elachlan
elachlan commented Nov 10, 2022
View reviewed changes
src/System.Windows.Forms.Primitives/src/Properties/AssemblyInfo.cs Outdated
@@ -6,6 +6,7 @@

[assembly: System.Runtime.InteropServices.ComVisible(false)]

[assembly: InternalsVisibleTo("Microsoft.VisualBasic.Forms, PublicKey=00000000000000000400000000000000")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have worked but we are still getting build errors. I don't know why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build is failing for other reasons:

##[error]src\System.Windows.Forms.Primitives\src\Windows\Win32\PInvoke.GetModuleFileNameLongPath.cs(6,1): error IDE0005: (NETCORE_ENGINEERING_TELEMETRY=Build) Using directive is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it has the failures:

error BC30389: (NETCORE_ENGINEERING_TELEMETRY=Build) 'Windows.Win32.PInvoke' is not accessible in this context because it is 'Friend'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Microsoft.VisualBasic.Forms doesn't reference System.Windows.Forms.Primitives project directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and I personally don't think it should.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IVT is not transitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RussKie Did you want me to remove the Microsoft.VisualBasic.Forms changes and we can work out what to do for it in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defer the decision to @lonitra and @JeremyKuhne.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Elachan it's probably better to do the VB stuff separately for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it.

AArnott
AArnott previously approved these changes Nov 10, 2022
View reviewed changes
@elachlan elachlan requested review from JeremyKuhne and removed request for RussKie November 10, 2022 20:55
@RussKie
Copy link
Member

RussKie commented Nov 11, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

JeremyKuhne
JeremyKuhne approved these changes Nov 11, 2022
View reviewed changes
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your effort and patience here!

@JeremyKuhne JeremyKuhne merged commit 93dd384 into dotnet:main Nov 11, 2022
@ghost ghost added this to the 8.0 Preview1 milestone Nov 11, 2022
@elachlan elachlan deleted the CA1838 branch November 11, 2022 23:00
@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AArnott AArnott AArnott left review comments

@RussKie RussKie RussKie left review comments

@lonitra lonitra lonitra left review comments

@JeremyKuhne JeremyKuhne JeremyKuhne approved these changes

Assignees

@elachlan elachlan

Labels
None yet
Projects
None yet
Milestone
8.0 Preview1
Development

Successfully merging this pull request may close these issues.

Review use of CA1838 warning - Avoid 'StringBuilder' parameters for P/Invokes
6 participants
@elachlan @JeremyKuhne @AArnott @KlausLoeffelmann @RussKie @lonitra