bpo-5945: mapping.rst: PyMapping_Check(list) returns 1 by jbarlow83 · Pull Request #144 · python/cpython

jbarlow83 · 2017-02-17T21:50:38Z

As discussed below, in Python 3, PyMapping_Check(list) returns 1. This behavior is justified by a developer as consistent with Python 3's internals. It is however surprising to C-API users (speaking from experience) and because it deviates from collections.abc.Mapping.

I think it is better to be vague than describe the implementation details.

https://mail.python.org/pipermail/python-dev/2009-May/089445.html

https://bugs.python.org/issue5945

the-knights-who-say-ni · 2017-02-17T21:50:39Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

serhiy-storchaka · 2017-02-18T09:16:05Z

Use double space after sentence-ending period. The line is too long, wrap it at column 79. Make collections.abc.Mapping a reference.

louisom · 2017-02-24T13:04:47Z

Should the commit rebase into one commit?

jbarlow83 · 2017-02-24T20:01:44Z

@grapherd The PR can be accepted as a squash merge in one click that will have the same effect as manually squashing it.

As discussed below, in Python 3, PyMapping_Check(list) returns 1. This behavior is justified by a developer as consistent with Python 3's internals. It is however surprising to C-API users (speaking from experience) and because it deviates from collections.abc.Mapping. https://mail.python.org/pipermail/python-dev/2009-May/089445.html Squashed a few revisions

jbarlow83 · 2017-02-27T20:09:30Z

Rebased to HEAD and squashed

Mariatta · 2017-03-06T06:10:25Z

I think this looks good now. @serhiy-storchaka would you like to take another look? I can do the backports.
Thanks :)

rhettinger · 2017-03-06T07:12:21Z

No backports please.

serhiy-storchaka · 2017-03-06T07:52:52Z

This is just the documentation fix. I think it should be backported.

But proposed wording doesn't LGTM. See my comment on the tracker.

jbarlow83 · 2017-03-10T21:46:26Z

On bpo serhiy-storchaka wrote:

Proposed wording doesn't looks much informative to me. Maybe just say that PyMapping_Check() also returns 1 for sequences that support slicing? And recommend PyMapping_Check() && !PySequence_Check() for true mapping test?

There is no recommendation to use PyMapping_Check() && !PySequence_Check() in that thread. In fact developers recommend against both PyMapping_Check and PySequence_Check as well:

(Author: Benjamin Peterson) Personally, I think PyMapping_Check and PySequence_Check should be deprecated and removed. Like their Python counterparts, operator.isMappingType() and operation.isSequenceType(), they are unreliable at best in the face of not LBYL and abcs

So I think it is appropriate to keep the proposed wording since it steers users to ABC checks and away from these APIs. If there is a change perhaps it should be explicitly "not recommended" in the documentation.

…iew_new Replace copied code by a limited API function. This fixes the assertion error caused by commit 2eea952.

Stackless contributes two tests to builtins: TaskletExit and TaskletExit.__init__. Therefore we have to adjust the limit. Add missing changelog entries (python#143, python#144).

brettcannon · 2018-02-21T01:53:27Z

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

jbarlow83 · 2018-02-23T01:11:53Z

I have made the requested changes; please review again.

Given that several core developers suggested or concurred with deprecating this API (Benjamin Peterson, GvR, Raymond Hettinger) I marked it not recommended as an alternative to officially deprecating.

I think it is best to omit the suggestion to usePyMapping_Check() && !PySequence_Check(). It appears this test does not work in certain cases, because PyMapping_Check is actually testing an CPython implementation detail. Instead I recommended PyObject_IsInstance which steers the developer toward the modern ABC API.

bedevere-bot · 2018-02-23T01:11:55Z

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

…iew_new Replace copied code by a limited API function. This fixes the assertion error caused by commit 2eea952. (cherry picked from commit 096ad87)

Stackless contributes two tests to builtins: TaskletExit and TaskletExit.__init__. Therefore we have to adjust the limit. Add missing changelog entries (python#143, python#144). (cherry picked from commit 7327e4b)

willingc

@jbarlow83 Thanks for the PR and the changes. To keep this PR from further languishing, I suppose it may be most efficient to update the wording to incorporate @serhiy-storchaka's suggestion.

willingc · 2018-05-21T04:44:33Z

- Return ``1`` if the object provides mapping protocol, and ``0`` otherwise. This
- function always succeeds.
+ **Not recommended.** Return ``1`` if the object provides the C-API mapping
+ protocol, and ``0`` otherwise. This function always succeeds. The C-API


I recommend using @serhiy-storchaka's suggested wording to move this PR forward.
~~The C-API mapping protocol is not equivalent to :class:collections.abc.Mapping, and also returns 1 for sequences that support slicing. Use :c:func::PyObject_IsInstance instead.~~
PyMapping_Check() also returns 1 for sequences that support slicing.

jbarlow83 · 2018-05-21T05:01:10Z

@willingc Done

willingc · 2018-05-21T05:02:40Z

Thanks @jbarlow83 for the quick response.

@serhiy-storchaka This looks good to me now. If you have no further changes, I recommend merging. Thanks.

ilevkivskyi · 2018-06-04T10:34:54Z

Superseded by #7029

…/100 PURE (D) implementation per theologian 00:02:12Z scoping audit (overturned A1 'classloader.h C++-only' framing as incomplete) + supervisor 00:04:33Z hold-lift (terminal goal pre-authorized via memory line 9 ZERO-C++). Phase 3D status: 99/100 PURE-CONVERTED + 1 PARTIAL (emitLoadMethodStatic) → 100/100 PURE-CONVERTED. Phase 3D close criteria as Alex memory-line-9 directive: pure C, no C++ compiler needed for emit body. CHANGES: (1) Python/jit/hir/builder.h: 2 friend declarations for new bridges. - friend int ::hir_builder_preloader_invoke_method_slot_c(void*, PyObject*) - friend void ::hir_builder_state_static_method_stack_push_cpp(void*, void*) (2) Python/jit/hir/builder.cpp: - HIRBuilder::emitLoadMethodStatic shrinks from 33 lines to 9 lines (pure type marshaling: just calls C body with bc_instr.oparg() + code_). - hir_builder_emit_load_method_static_c extern decl signature shrunk: drops is_classmethod, vte_state_offset, vte_load_offset, is_static, out_entry_func params; takes opcode oparg + code instead. - 2 new bridge implementations: - hir_builder_preloader_invoke_method_slot_c: returns slot from invokeMethodTarget (was C++-only access via private preloader_). - hir_builder_state_static_method_stack_push_cpp: pushes Register* to static_method_stack_ (mirrors existing _pop_cpp bridge). (3) Python/jit/hir/builder_emit_c.c: - #include cinderx/StaticPython/classloader.h (transitively pulls vtable.h with _PyType_VTable + _PyType_VTableEntry struct defs). A1 framing was incomplete: vtable.h is already extern "C" wrapped + pure typedef struct (no class/template/namespace), safe in C TU. - Forward-decl 3 bridges used in C body. - Body adds C-side derivations (was C++ stub responsibility): * arg = PyTuple_GET_ITEM(code->co_consts, oparg) (constArg equivalent) * descr = PyTuple_GET_ITEM(arg, 0) * is_classmethod = _PyClassLoader_IsClassMethodDescr(arg) * slot = hir_builder_preloader_invoke_method_slot_c(builder, descr) * is_static = via existing hir_builder_invoke_method_target_c bridge * vte_state/load_offset = offsetof + slot arithmetic, wrapped in VTableByteOffset (P5 wrapper from push 58) - Conditional static_method_stack push moved from C++ stub to C body via hir_builder_state_static_method_stack_push_cpp bridge. - .v unwrap at the 2 boundaries to hir_c_create_load_field_reg (raw intptr_t API). VERIFICATION pending: testkeeper rebuild + 30x all 8 W-2A+W-2B sentinels (must remain 240/240 PASS, behavior-equivalent expected) + Phoenix suite + ABBA + dual-arch + INVOKE_*-style structural test (test_phoenix_partial_conversions.py). Per pythia python#144 python#2 + supervisor 00:04:33Z: Alex notification will follow push 60 landing ('Phase 3D 100/100 PURE achieved via (D) — minimal scope ~30-60min, terminal goal directly served per memory line 9'). Per feedback_dont_ask_just_do.md: terminal goal already authorized; (D) is implementation path serving that authorization, not new scope. Auth chain: theologian scoping audit 00:02:12Z + theologian go-direct 00:03:38Z + supervisor hold-lift 00:04:33Z + generalist (D) feasibility verification 00:02:38Z.

the-knights-who-say-ni added the CLA not signed label Feb 17, 2017

Mariatta removed the CLA not signed label Feb 18, 2017

the-knights-who-say-ni added the CLA signed label Feb 18, 2017

jbarlow83 changed the title ~~mapping.rst: PyMapping_Check(list) returns 1~~bpo-5945: mapping.rst: PyMapping_Check(list) returns 1Feb 18, 2017

serhiy-storchaka requested changes Feb 18, 2017
View reviewed changes

serhiy-storchaka added the docs Documentation in the Doc dir label Feb 18, 2017

berkerpeksag added needs backport to 3.5 labels Feb 19, 2017

jbarlow83 force-pushed the docs-pymapping-check branch from e9ad4cc to 0c7abc1Compare February 27, 2017 20:08

brettcannon removed the needs backport to 3.5 label Aug 11, 2017

akruis pushed a commit to akruis/cpython that referenced this pull request Dec 20, 2017
Stackless issuepython#144: Replace copied code in pricklepit.c dictv…
096ad87
…iew_new Replace copied code by a limited API function. This fixes the assertion error caused by commit 2eea952.

brettcannon added the awaiting changes label Feb 21, 2018

Update mapping.rst
be98854

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 23, 2018

willingc reviewed May 21, 2018
View reviewed changes

Remove alternative to PyMapping_Check
463418b

willingc approved these changes May 21, 2018
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 21, 2018

ilevkivskyi closed this Jun 4, 2018

Uh oh!

Uh oh!

Conversation

jbarlow83 commented Feb 17, 2017• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Feb 17, 2017

Uh oh!

serhiy-storchakaFeb 18, 2017

Choose a reason for hiding this comment

Uh oh!

louisom commented Feb 24, 2017

Uh oh!

jbarlow83 commented Feb 24, 2017

Uh oh!

jbarlow83 commented Feb 27, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mariatta commented Mar 6, 2017

Uh oh!

rhettinger commented Mar 6, 2017

Uh oh!

serhiy-storchaka commented Mar 6, 2017

Uh oh!

jbarlow83 commented Mar 10, 2017

Uh oh!

brettcannon commented Feb 21, 2018

Uh oh!

jbarlow83 commented Feb 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Feb 23, 2018

Uh oh!

willingc left a comment

Choose a reason for hiding this comment

Uh oh!

willingcMay 21, 2018

Choose a reason for hiding this comment

Uh oh!

jbarlow83 commented May 21, 2018

Uh oh!

willingc commented May 21, 2018

Uh oh!

ilevkivskyi commented Jun 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

jbarlow83 commented Feb 17, 2017•
edited by bedevere-bot
Loading

jbarlow83 commented Feb 27, 2017•
edited
Loading

jbarlow83 commented Feb 23, 2018•
edited
Loading