Align Constrained Execution Region with netcore for merging netcore/netfx

[MichelZ]

Attempting to align CER changes from netfx to netcore for code merging

[MichelZ]

Unfortunately it seems the diff that github creates looks really complicated.
The changes are not complicated at all... I'll probably try to simplify this with multiple smaller PR's after gathering some feedback

[MichelZ]

Please use ?w=1 in the GitHub URL to ignore whitespace changes, and the diff looks much much cleaner!!

[benrr101]

I think I see what you're going for here, but.... I don't really think this is the right approach. As far as I can tell, this is basically adding a massive amount of no-ops to the netcore code just so it matches the structure of the netfx code. While yeah it'll definitely make a merge easier, I'm not sure what your plan is for actually merging the two files after it the files are modified enough to look like each other.

My thought is that if there's something that can be done the some on netcore and netfx, we should try to get them doing things the same way (without making huge changes to the code). If it can't be done the same in both, eg constrained execution is only needed in netfx, then the merged version should have the constrained execution specific code wrapped in #if NETFRAMEWORK.

[MichelZ]

I'm not sure what your plan is for actually merging the two files after it the files are modified enough to look like each other.

Ideally, all the code is of course identical in the end, so you can just take one version of the file and move it to the shared project. But even if it's not 100% identical, doing it this way, and slowly aligning them still (IMHO) is a low-risk way to ease the way to merging them in the (hopefully near) future. If you think this is not a viable way to go - I'll stop with these, that's not an issue for me

My thought is that if there's something that can be done the some on netcore and netfx, we should try to get them doing things the same way (without making huge changes to the code). If it can't be done the same in both, eg constrained execution is only needed in netfx, then the merged version should have the constrained execution specific code wrapped in #if NETFRAMEWORK.

I'm happy to wrap them around #if NETFRAMEWORK, if that's the issue.
While deciding what to do, I was looking at the TdsParserStateObject.netcore.cs file, for example here:

SqlClient/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.netcore.cs

Lines 312 to 320 in 1742793

	stateObj.SendAttention(mustTakeWriteLock: true);
	PacketHandle syncReadPacket = default;
	RuntimeHelpers.PrepareConstrainedRegions();
	bool shouldDecrement = false;
	try
	{
	Interlocked.Increment(ref _readingCount);
	shouldDecrement = true;

it does not wrap the call into a conditional.

On another note, What does the #if NETFRAMEWORK give you in this case here?
We do agree that a merged version needs to have the CER in for netfx support, right? So it needs to be there.
The methods are marked as [Conditional("NETFRAMEWORK")], so they get optimized away in the netcore version, right?
To the point: the code also has a lot of Debug.Assert, and they are not wrapped in #if DEBUG

Just let me know what you prefer, and I'll adjust to that

[codecov]

Codecov Report

Attention: Patch coverage is 56.84211% with 123 lines in your changes missing coverage. Please review.

Project coverage is 72.45%. Comparing base (4bc9ee6) to head (759e6e0).
Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
...core/src/Microsoft/Data/SqlClient/SqlDataReader.cs	56.84%	123 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2969      +/-   ##
==========================================
- Coverage   72.50%   72.45%   -0.05%     
==========================================
  Files         288      288              
  Lines       59529    59498      -31     
==========================================
- Hits        43159    43109      -50     
- Misses      16370    16389      +19     

Flag	Coverage Δ
addons	92.58% <ø> (ø)
netcore	75.38% <56.84%> (-0.06%)	⬇️
netfx	70.92% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[benrr101]

@MichelZ Sorry if I sounded a bit harsh previously - I definitely appreciate the effort you're putting in, but just want to make sure we're working towards the same goals. It's also worth noting that I've only been (sorta) in charge of merging the two projects for a month or two. If there's an difference between older merging and newer merging, that's likely why. So, to address your points:

• I think it's nice if we can have the code be identical, but it won't always be beneficial to have it be identical.
  • Eg, if netfx uses byte[] while netcore uses Span, we could migrate netfx to the Span. In that case, we still have the same functionality in netfx and netcore, but just aren't doing it with the same code.
  • Eg, if netfx does something extra that is not needed or not compatible with netcore, then we should retain the netfx code, wrapped in an #if NETFRAMEWORK. In this case, we want to only include the functionality in netfx, and make it clear that it only applies to netfx.
• I'm honestly not super sure how much the compiler optimized away [Conditional] methods. I agree that it optimizes out the method, but since there's nothing else in that class, does it also optimize out the class?
  • Although in most cases we don't wrap Debug.Assert in #if DEBUG, there are a handful where we have to. Even if Debug.Assert becomes a no-op, expressions assigned to the arguments are still evaluated, even if they can't compile.
  • Ultimately, my philosophy on [Conditional] is that it's less obvious when reading the code that the call to a [Conditional] method does something verses doesn't do anything. Wrapping the method and calls to it #if NETFRAMEWORK makes it abundantly clear that the call only applies to one framework, and makes it easier to find blocks of similar functionality if we ever drop netfx support.
  • If it seems like there's a disconnect between Debug.Assert and other conditional method, I think it's mostly because Debug.Assert comes with the framework, and it's behavior is well known, regardless of the project we're working on. While it's fine to have project-specific conventions (ie, "oh this method doesn't do anything in netfx"), we can cut down on the learning curve and gotchas. For a project of this size, any reduction to those is greatly appreciated.
• Re TdsParserStateObject.netcore.cs, that seems to me like an oversight. That class is obscenely large, so it doesn't surprise me something slipped through while merging it.

[MichelZ]

Understood, thanks.
I'll wrap them in #IF NETFRAMEWORK to make it clear then