Rename Py_SAFE_DOWNCAST

[pitrou]

BPO	19692
Nosy	@tim-one, @loewis, @pitrou, @vstinner, @tiran, @methane, @skrah, @CuriousLearner, @shihai1991
PRs	gh-63891: Rename Py_SAFE_DOWNCAST to _Py_DOWNCAST #15090

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2013-11-22.14:24:55.387>
labels = ['interpreter-core', '3.8', 'type-feature', '3.7', '3.9']
title = 'Rename Py_SAFE_DOWNCAST'
updated_at = <Date 2019-09-15.04:11:33.749>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2019-09-15.04:11:33.749>
actor = 'shihai1991'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2013-11-22.14:24:55.387>
creator = 'pitrou'
dependencies = []
files = []
hgrepos = []
issue_num = 19692
keywords = ['patch']
message_count = 14.0
messages = ['203764', '203766', '203767', '203782', '203787', '203789', '203790', '203855', '348946', '348951', '348952', '349029', '349049', '352460']
nosy_count = 9.0
nosy_names = ['tim.peters', 'loewis', 'pitrou', 'vstinner', 'christian.heimes', 'methane', 'skrah', 'CuriousLearner', 'shihai1991']
pr_nums = ['15090']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue19692'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

[pitrou]

Py_SAFE_DOWNCAST's name is a bit misleading: it isn't safe except in debug mode. I propose to rename it to Py_DOWNCAST, so that developers are reminded that the burden of the sanity checks is on them.

[tiran]

+1

[vstinner]

I like Py_DOWNCAST name.

[pitrou]

Actually, _Py_DOWNCAST may be better (not a public API).

[loewis]

-1. The macro name doesn't claim the cast to be safe, i.e. it's not "Py_SAFELY_DOWNCAST", but "safe downcast", i.e. it's an assertion that the cast actually has been verified as being safe.

[pitrou]

-1. The macro name doesn't claim the cast to be safe, i.e. it's not
"Py_SAFELY_DOWNCAST", but "safe downcast", i.e. it's an assertion that
the cast actually has been verified as being safe.

It's not an assertion, it's a cast.
Otherwise it should be named Py_ASSERT_SAFE_DOWNCAST (and it should only
do the assertion, not the cast).

[tim-one]

Goodness. Name it

_Py_DOWNCAST_AND_IN_DEBUG_MODE_ASSERT_UPCASTING_THE_RESULT_COMPARES_EQUAL_TO_THE_ORIGINAL_ARGUMENT

;-)

[pitrou]

Well, I obviously won't fight very hard for this one.

But I would like to point out that APIs with "safe" (not "safely" :-)) in their name usually imply that the API is safe, not that the input has been sanitized beforehand.

For example in the stdlib: pprint.saferepr, string.safe_substitute, xmlrpc.client.SafeTransport. In the C API: Py_TRASHCAN_SAFE_BEGIN.

[shihai1991]

Rename Py_SAFE_DOWNCAST in PR_15090.

In the C API: Py_TRASHCAN_SAFE_BEGIN and Py_TRASHCAN_SAFE_END should be removed or keep it due to compatibility?

In the stdlib: Looks that it's not changed is ok.

[skrah]

Why would one not abort() in release mode? If the cast is inexact, the results will usually be so wrong that even on a web server a hard exit is preferable.

I don't think the check costs much time with branch prediction.

[vstinner]

I dislike this macro. A cast is either safe or unsafe. If it is safe, (type)var would be better. If it is unsafe, well, it would be better to add a runtime check. No? (I mean better error reporting than abort() pnly in debug mode.)

[methane]

While Py_SAFE_DOWNCAST is not documented, it doesn't start with underscore.
How many 3rd party code are broken by changing the name?

[shihai1991]

It's a big problem, no ability to answer this question :(