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.

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

Analyzer: Adjust code for numeric IntPtr. #72348

Closed
teo-tsirpanis opened this issue Jul 17, 2022 · 7 comments
Closed

Analyzer: Adjust code for numeric IntPtr. #72348

teo-tsirpanis opened this issue Jul 17, 2022 · 7 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@teo-tsirpanis
Copy link
Contributor

With the advert of the numeric IntPtr feature, nint is an alias for System.IntPtr just like int is an alias for System.Int32. We should add analyzers and fixers to make it clear to user code, especially to those who think that IntPtr represents an opaque pointer-sized value. Here's what the analyzers should do:

  • IDE0049 should be updated to suggest converting IntPtr to nint and UIntPtr to nuint.
  • U?IntPtr.Zero0 or (nu?int)0 if it would introduce ambiguity
  • new U?IntPtr(x)x or (nu?int)x if it would introduce ambiguity, or unchecked((nu?int)x) if we are in a checked context.
  • x.ToU?Int32()checked((u?int)x) or (u?int)x if we are in a checked context
  • x.ToU?Int64()unchecked((u?long)x) or (u?long)x if we are in an unchecked context
  • x.ToPointer()(void*) x
  • Convert explicit casts between U?IntPtr and u?int or u?long, to nu?int, while taking the overflow checking context into account (too many to list).

These analyzers and fixers should be triggered only if the underlying runtime supports numeric IntPtr. Some of them might be legal even without it but it's not necessary to make it more complicated.

@teo-tsirpanis teo-tsirpanis added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer labels Jul 17, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 17, 2022
@ghost
Copy link

ghost commented Jul 17, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

With the advert of the numeric IntPtr feature, nint is an alias for System.IntPtr just like int is an alias for System.Int32. We should add analyzers and fixers to make it clear to user code, especially to those who think that IntPtr represents an opaque pointer-sized value. Here's what the analyzers should do:

  • IDE0049 should be updated to suggest converting IntPtr to nint and UIntPtr to nuint.
  • U?IntPtr.Zero0 or (nu?int)0 if it would introduce ambiguity
  • new U?IntPtr(x)x or (nu?int)x if it would introduce ambiguity, or unchecked((nu?int)x) if we are in a checked context.
  • x.ToU?Int32()checked((u?int)x) or (u?int)x if we are in a checked context
  • x.ToU?Int64()unchecked((u?long)x) or (u?long)x if we are in an unchecked context
  • x.ToPointer()(void*) x
  • Convert explicit casts between U?IntPtr and u?int or u?long, to nu?int, while taking the overflow checking context into account (too many to list).

These analyzers and fixers should be triggered only if the underlying runtime supports numeric IntPtr. Some of them might be legal even without it but it's not necessary to make it more complicated.

Author: teo-tsirpanis
Assignees: -
Labels:

api-suggestion, area-System.Runtime, code-analyzer, code-fixer

Milestone: -

@jeffhandley
Copy link
Member

@tannergooding Do the details of this proposal match your expectations for the analyzer you had in mind?

@jeffhandley jeffhandley added this to the 7.0.0 milestone Jul 27, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 27, 2022
@tannergooding
Copy link
Member

Yes, this looks to capture the most relevant details.

@jkotas
Copy link
Member

jkotas commented Jul 27, 2022

It would be nice for the code-fixer to suggest replacing IntPtr with actual pointer types where the code is actually operating on pointers.

Blank replacing IntPtr with nint will often make code harder to understand. See discussion in #70297

@dakersnar dakersnar modified the milestones: 7.0.0, Future Aug 8, 2022
@dakersnar
Copy link
Contributor

dakersnar commented Aug 8, 2022

We're planning on getting an analyzer out that flags the known behavioral changes for 7.0, code fixers will be a future consideration (post 7.0).

@jeffhandley
Copy link
Member

jeffhandley commented Aug 12, 2022

I thought we had a separate issue tracking the analyzer itself, but we don't. Leaving a comment here to capture what the analyzer work is separate from the fixer as proposed in the description. From there, we can spawn off issue(s) as needed.

  • IDE0049 should be updated to suggest converting IntPtr to nint and UIntPtr to nuint.
  • Code simplification opportunities now available that could be suggested (but these are not changes in behavior to existing usage)
    • We need a new analyzer proposal issue filed for this, and that issue will be put into the "Future" milestone; defaulted to "Info"
      • U?IntPtr.Zero0 or (nu?int)0 if it would introduce ambiguity
      • new U?IntPtr(x)x or (nu?int)x if it would introduce ambiguity, or unchecked((nu?int)x) if we are in a checked context.
      • x.ToU?Int32()checked((u?int)x) or (u?int)x if we are in a checked context
      • x.ToU?Int64()unchecked((u?long)x) or (u?long)x if we are in an unchecked context
      • x.ToPointer()(void*) x
  • Change in behavior in .NET 7 where the user-defined operators will no longer be called (change to existing behavior)
    • We have a new analyzer proposal issue filed for this, and we still want to target .NET 7 RC2 for it; it would default to "Warning"
    • Places where the addition, subtraction, and explicit cast operators were used on U?IntPtr, while taking the overflow checking context into account
      • IntPtr
        • operator +(IntPtr, int): Unchecked: same behavior; Checked: may overflow now when it didn't before; Wrap in unchecked context to prevent the overflow and preserve behavior
        • operator -(IntPtr, int): Unchecked: same behavior; Checked: may overflow now when it didn't before; Wrap in unchecked context to prevent the overflow and preserve behavior
        • explicit operator IntPtr(long): Checked: same behavior; Unchecked: will not throw an overflow exception when it could have before in 32-bit contexts; Wrap in a checked context to ensure the exception will be thrown as before in 32-bit contexts
        • explicit operator void*(IntPtr): Unchecked: same behavior; Checked: may overflow now when it didn't before; Wrap in unchecked context to prevent the overflow and preserve behavior
        • explicit operator IntPtr(void*): Unchecked: same behavior; Checked: may overflow now when it didn't before; Wrap in unchecked context to prevent the overflow and preserve behavior
        • explicit operator int(IntPtr): Checked: same behavior; Unchecked: will not throw an overflow exception when it could have before in 64-bit contexts; Wrap in checked context to ensure the exception will be thrown as before in 64-bit contexts
      • UIntPtr
        • operator +(UIntPtr, int): Unchecked: same behavior; Checked: may overflow now when it didn't before; Wrap in unchecked context to prevent the overflow and preserve behavior
        • operator -(UIntPtr, int): Unchecked: same behavior; Checked: may overflow now when it didn't before; Wrap in unchecked context to prevent the overflow and preserve behavior
        • explicit operator UIntPtr(ulong): Checked: same behavior; Unchecked: will not throw an overflow exception when it could have before in 32-bit contexts; Wrap in a checked context to ensure the exception will be thrown as before in 32-bit contexts
        • explicit operator uint(UIntPtr): Checked: same behavior; Unchecked: will not throw an overflow exception when it could have before in 64-bit contexts; Wrap in checked context to ensure the exception will be thrown as before in 64-bit contexts
  • Leave this issue open to capture the code-fixer work, which will remain in the "Future" milestone

@buyaa-n
Copy link
Member

buyaa-n commented Mar 12, 2024

Looks only the 2nd check box is pending (Code simplification opportunities now available that could be suggested (but these are not changes in behavior to existing usage)) for which an issue was created #74518, so closing this issue

@buyaa-n buyaa-n closed this as completed Mar 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
None yet
Development

No branches or pull requests

6 participants