CS0177 weird behavior

[levicki]

Please consider the following code:

// Case 1 - CS0177 error
private bool GetSecureKeyDetail(UInt32 Timeout, out UInt32 FuncMask)
{
	IntPtr ResultPtr = IntPtr.Zero;

	bool Succeeded = false;

	try {
		Int32 hr = GetInfo(WFS_INF_PIN.SECUREKEY_DETAIL, IntPtr.Zero, Timeout, out ResultPtr);

		if (hr == WFS_ERR.SUCCESS) {
			WFSRESULT Result = Marshal.PtrToStructure<WFSRESULT>(ResultPtr);

			WFSPINSECUREKEYDETAIL SKDetail = Marshal.PtrToStructure<WFSPINSECUREKEYDETAIL>(Result.Buffer);

			WFSPINFUNCKEYDETAIL FKDetail = Marshal.PtrToStructure<WFSPINFUNCKEYDETAIL>(SKDetail.FuncKeyDetail);

			FuncMask = FKDetail.FuncMask;

			Succeeded = true;
		}
	} finally {
		if (!Succeeded) {
			FuncMask = 0;
		}
		if (ResultPtr != IntPtr.Zero) {
			FreeResult(ResultPtr);
		}
	}

	return Succeeded;
}

This code generates CS0177 error which I believe should not be generated because if I am not mistaken FuncMask will always be written.

On the other hand, if I modify the code to look like this:

// Case 2 - no CS0177 error
private bool GetSecureKeyDetail(UInt32 Timeout, out UInt32 FuncMask)
{
	IntPtr ResultPtr = IntPtr.Zero;

	bool Succeeded = false;

	try {
		Int32 hr = GetInfo(WFS_INF_PIN.SECUREKEY_DETAIL, IntPtr.Zero, Timeout, out ResultPtr);

		if (hr == WFS_ERR.SUCCESS) {
			WFSRESULT Result = Marshal.PtrToStructure<WFSRESULT>(ResultPtr);

			WFSPINSECUREKEYDETAIL SKDetail = Marshal.PtrToStructure<WFSPINSECUREKEYDETAIL>(Result.Buffer);

			WFSPINFUNCKEYDETAIL FKDetail = Marshal.PtrToStructure<WFSPINFUNCKEYDETAIL>(SKDetail.FuncKeyDetail);

			FuncMask = FKDetail.FuncMask;

			Succeeded = true;
		} else {
			FuncMask = 0;
		}
	} finally {
		if (ResultPtr != IntPtr.Zero) {
			FreeResult(ResultPtr);
		}
	}

	return Succeeded;
}

The CS0177 error goes away, but if I am not mistaken it should be generated because if Marshal.PtrToStructure throws then FuncMask won't be written at all.

Please share your thoughts on CS0177 correctness, should this be reported as an issue or not?

[CyrusNajmabadi]

This is by design. The language doesn't track the relationship between disparate variables. If you would like that, you'll need to start a discussion on csharplang (or add to an existing one).

[levicki]

Actual error cases are detected.

And yet my examples show they are not. Two out of two wrong detections — 100% failure rate.

It doesn't claim with certainty.

Yes it does — showing an error means it is certain that what I wrote is wrong and it wants me to correct it, while showing a warning would mean it isn't certain and it wants me to double-check the correctness myself.

Only when it is certain is it ok.

And yet my code example 2 shows how its certainness is plain incorrect because variable can end up uninitialized.

Because then users could trivially run into uninitialized variables, which we think is bad.

They can already trivially run into uninitialized variables as demonstrated above with case 2.

You're welcome to suggest changes to the language.

Fat lot of good will that do when we can't even agree that the spec is broken.

[CyrusNajmabadi]

Two out of two wrong detections — 100% failure rate.

I don't know how you're computing this. In one case it's a not a failure. We can't prove it's safe, so we report that.

For your second case, this is working as intended. In the case of an exception, the out variables don't need to be written (just like return values aren't provided). Why? Because there was an exception, so the caller won't complete on the call anyways to write into the out-vars they pass in, or execute any assignments of the call the make.

Yes it does — showing an error means it is certain that what I wrote is wrong and it wants me to correct it

No. It means it hasn't proven it is safe. When it can't prove it is safe, it is not legal as per the the rules of the language. Again, if you want the language to change here you have to petition the language.

[CyrusNajmabadi]

And yet my code example 2 shows how its certainness is plain incorrect because variable can end up uninitialized.

The variable is not initialized. But the caller cannot observe that. Why? Because an exception was thrown. In all cases where the code runs to an actual method exit successfullly (non-exceptionally) the value must be written.

They can already trivially run into uninitialized variables as demonstrated above with case 2.

Your code does not demontrate it at all. It demonstrates not writing in the case of an exception. It does not demonstrate any code being able to run into the uninitialized variable.

Fat lot of good will that do when we can't even agree that the spec is broken.

This attitude is not helpful. Right now the language is spec'ed very specifically here, and the compiler is abiding by that specification. If you want the compiler to do something else, you will need to petition for a language change. That's how things work here.

[CyrusNajmabadi]

Simplified:

In the first case, there appears to be a non-exceptional codepath where the variable is not assigned. This is because the language doesn't track the relationship between variables, so we don't understand how to use the 'Succeeded' variable to inform things here.

If you want that to be supported, you'll have to make a language changes request.

In the second case, we can see that in all non-exceptional paths it is assigned. And it exceptional paths it isn't(which is 100% ok). This is again working just as spec'ed in the language. There's no indication that there is anything even wrong in this case. This works very much as intended wrt exceptional vs non-exceptional flows. You could of course request a change, but no argument has been put forth about why this behavior is undesirable for the second case.

[CyrusNajmabadi]

Intuition, for anyone interested (though I recommend reading the actual spec). All out variables need to have been written by the time any 'end point' of the method is reached. You can reach those end points by hitting a return statement, or naturally hitting the end of the method block itself. Where naturally is normal (non-exceptional) code flow.

In the second example, the only way to naturally hit the actual end points of the method always go through codepaths that assign the out var. The only way to not hit it is with an exception. But that's completely fine. A more trivial way if seeing that is just:

void M(out int x) => throw new Not implemented exception();

This is entirely fine as the caller of M would never know the difference. Either you assign 'x' and return successfully, and this that see that value. Or you throw, and don't assign, but then the caller never can tell because the exception continues to bubble up.

[CyrusNajmabadi]

Precise spec rules.

Every output parameter of a function member or anonymous function shall be definitely assigned (§9.4) before the function member or anonymous function returns normally.

Definite assignment: https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/variables.md#94-definite-assignment

All output parameters of a function member shall be definitely assigned at each location where the function member returns (through a return statement or through execution reaching the end of the function member body).

[levicki]

If you want that to be supported, you'll have to make a language changes request.

I would definitely want that to be supported because to me it makes sense -- compiler should not claim an error because it can't "see" the assignment.

As for the second case I stand corrected — this is actually why I started this topic as a discussion because I suspected I might be overlooking something. I will now mark your comment which I expected to get instead of "works to spec, closed" as an answer. You have my genuine thanks for engaging further despite my attitude.

[CyrusNajmabadi]

I would definitely want that to be supported because to me it makes sense -- compiler should not claim an error because it can't "see" the assignment.

Then please either open a discussion or continue a discussion at dotnet/csharplang. That's a request to change the language rules. If that change is approved, roslyn will be updated to follow the spec. Thanks.

[CyrusNajmabadi]

Intentionally locking. This question has been asked and answered. The latest responses are all things that should go to dotnet/csharplang instead. Locking this so that that happens and the requests for language changing doing continue happening here.