gh-140650: fix SystemError in io.BufferedWriter.close when closed errors

[inventshah]

Restores the 3.14 behavior of emitting a TypeError. Preservation of the sign helps in the error checking here:

cpython/Modules/_io/bufferedio.c

Lines 556 to 563 in 3dab11f

	/* gh-138720: Use IS_CLOSED to match flush CHECK_CLOSED. */
	r = IS_CLOSED(self);
	if (r < 0)
	goto end;
	if (r > 0) {
	res = Py_NewRef(Py_None);
	goto end;
	}

db68bfc changed from buffered_closed to IS_CLOSED.

• Issue: SystemError in io.BufferedWriter #140650

[serhiy-storchaka]

This does not look right. If IS_CLOSED() returns a positive value and sets an error, there is a bug in IS_CLOSED().

[cmaloney]

@serhiy-storchaka : Overall problem is code assumes IS_CLOSED can't result in an exception being set but if a user-provided implementation does unusual things (ex. closed = NotImplemented) then that breaks.

Another option here is "Exception" can be turned into a True result for "IS_CLOSED" + clear the exception but that seems like hiding an implemention issue rather than helping resolve it to me.

[serhiy-storchaka]

IS_CLOSED() should return -1 on error.

[cmaloney]

Also update CHECK_CLOSED to handle IS_CLOSED returning non-zero + setting an exception. In particular, if there's an exception CHECK_CLOSED should always return.

[inventshah]

Should I still be using PyErr_Occurred() or based on @serhiy-storchaka's feedback, should we make IS_CLOSED() return -1 on exception and check the sign in both CHECK_CLOSED() and _io__Buffered_close_impl()?

[serhiy-storchaka]

PyErr_Occurred() is only used when we cannot distinguish the value signalling an error from a valid value, like in PyLong_AsLong(). IS_CLOSED() returns -1 on error, 0 -- false, 1 -- true. Or so it should be.

[efimov-mikhail]

LGTM

[serhiy-storchaka]

This is not accurate. Bad things happen when the value of the closed attribute cannot be converted to boolean.

[inventshah]

In addition to the "cannot be converted to a boolean" case, this patch also fixes cases where closed is defined as a managed attribute equivalent to

@property
def closed(self):
    raise Exception()

Should the news entry just focus on the first type error case, or mention fixing this case as well?

[efimov-mikhail]

Maybe we should add tests on this behavior too?

[inventshah]

Updated the news to reflect both cases, and added a test for the other behavior. I wonder if we need a line mentioning that exceptions from .closed are no longer swallowed by the corresponding methods (like .write(), .flush())