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

New Analyzer: Prevent behavioral change in built-in operators for IntPtr and UIntPtr #6153

Merged
merged 8 commits into from
Sep 21, 2022

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented Sep 8, 2022

Fixes dotnet/runtime#74022

With numeric IntPtr feature, System.IntPtr and System.UIntPtr gained some built-in operators (conversions, unary and binary). Those now might throw when overflowing within checked context or not throw in unchecked context compared to the previous "user-defined" operators , which is causing behavioral change in .NET 7.

This PR only includes the analyzer, fixer is not planned in 7.0 and also the preferred fix might be: IDE0049 should be updated to suggest converting IntPtr to nint and UIntPtr to nuint. So fixer need to be added after it added to IDE0049

Detect places where the behavioral changes would be happening and war, scenarios:

  • For IntPtr
    • operator +(IntPtr, int): Unchecked: same behavior; Checked: may overflow now when it didn't before;
    • operator -(IntPtr, int): Unchecked: same behavior; Checked: may overflow now when it didn't before;
    • explicit operator IntPtr(long): Checked: same behavior; Unchecked: will not throw an overflow exception when it could have before in 32-bit contexts;
    • explicit operator void*(IntPtr): Unchecked: same behavior; Checked: may overflow now when it didn't before;
    • explicit operator IntPtr(void*): Unchecked: same behavior; Checked: may overflow now when it didn't before;
    • explicit operator int(IntPtr): Checked: same behavior; Unchecked: will not throw an overflow exception when it could have before in 64-bit contexts;
  • For UIntPtr
    • operator +(UIntPtr, int): Unchecked: same behavior; Checked: may overflow now when it didn't before;
    • operator -(UIntPtr, int): Unchecked: same behavior; Checked: may overflow now when it didn't before;
    • explicit operator UIntPtr(ulong): Checked: same behavior; Unchecked: will not throw an overflow exception when it could have 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;
  • These analyzer should be triggered only if the underlying runtime supports numeric IntPtr feature, i.e. check if System.Runtime.CompilerServices.RuntimeFeature.NumericIntPtr is available in corelib as

Suggested severity : "Warning"
Suggested category : "Reliability"

CC @jeffhandley @tannergooding @eerhardt

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #6153 (305e134) into main (a26d1a2) will increase coverage by 0.00%.
The diff coverage is 97.08%.

Additional details and impacted files
@@            Coverage Diff            @@
##             main    #6153     +/-   ##
=========================================
  Coverage   96.03%   96.03%             
=========================================
  Files        1348     1354      +6     
  Lines      309832   311095   +1263     
  Branches     9967    10006     +39     
=========================================
+ Hits       297534   298775   +1241     
- Misses       9900     9910     +10     
- Partials     2398     2410     +12     

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

I left a couple comments on the messages, but it looks good to me. We should wait for signoff from @tannergooding on the functional behavior though.

@buyaa-n
Copy link
Member Author

buyaa-n commented Sep 19, 2022

After testing with runtime and a few other repos it showed that there is no many cases that the behavioral change is a real issue and needs wrapping with checked/unchecked, so we decided to degrade the severity level to info.

@jmarolf @mavasani @JoeRobich this the last analyzer we would like to merge in 7.0, I don't see any 7.0 branch created in the repo, is the main is the correct branch for .Net 7changes?

@buyaa-n
Copy link
Member Author

buyaa-n commented Sep 21, 2022

Confirmed that the main is the correct branch, merging

@buyaa-n buyaa-n merged commit 66c3174 into dotnet:main Sep 21, 2022
@buyaa-n buyaa-n deleted the int-ptr branch September 21, 2022 17:39
@github-actions github-actions bot added this to the vNext milestone Sep 21, 2022
333fred added a commit to 333fred/roslyn-analyzers that referenced this pull request Sep 29, 2022
* upstream/main:
  Localized file check-in by OneLocBuild Task: Build definition ID 830: Build ID 2004411
  Run CI using the new VM images
  Localized file check-in by OneLocBuild Task: Build definition ID 830: Build ID 2000239
  New Analyzer: Prevent behavioral change in built-in operators for IntPtr and UIntPtr (dotnet#6153)
  GM interfaces implementations can be self constrained (dotnet#6168)
  Localized file check-in by OneLocBuild Task: Build definition ID 830: Build ID 1993001
  New Analyzer: Implement Generic Math Interfaces Correctly  (dotnet#6126)
@jmarolf jmarolf modified the milestones: vNext, .NET 7.x Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Analyzer] Prevent behavioral change in built-in operators for IntPtr and UIntPtr
8 participants