Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_weakrefset.WeakSet.__contains__ should not propagate TypeErrors #54569

Closed
tseaver opened this issue Nov 8, 2010 · 16 comments
Closed

_weakrefset.WeakSet.__contains__ should not propagate TypeErrors #54569

tseaver opened this issue Nov 8, 2010 · 16 comments
Labels
stdlib Python modules in the Lib dir

Comments

@tseaver
Copy link

tseaver commented Nov 8, 2010

BPO 10360
Nosy @birkenfeld, @rhettinger, @pitrou, @voidspace
Files
  • issue10360-redux.patch
  • 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 = <Date 2011-02-25.10:57:48.181>
    created_at = <Date 2010-11-08.14:12:42.012>
    labels = ['library']
    title = '_weakrefset.WeakSet.__contains__ should not propagate TypeErrors'
    updated_at = <Date 2011-02-25.10:57:48.180>
    user = 'https://bugs.python.org/tseaver'

    bugs.python.org fields:

    activity = <Date 2011-02-25.10:57:48.180>
    actor = 'georg.brandl'
    assignee = 'none'
    closed = True
    closed_date = <Date 2011-02-25.10:57:48.181>
    closer = 'georg.brandl'
    components = ['Library (Lib)']
    creation = <Date 2010-11-08.14:12:42.012>
    creator = 'tseaver'
    dependencies = []
    files = ['19547']
    hgrepos = []
    issue_num = 10360
    keywords = ['patch']
    message_count = 16.0
    messages = ['120743', '120768', '120770', '120771', '120772', '120774', '120779', '122033', '122036', '123199', '123249', '123252', '127614', '127615', '127643', '129351']
    nosy_count = 5.0
    nosy_names = ['georg.brandl', 'rhettinger', 'tseaver', 'pitrou', 'michael.foord']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue10360'
    versions = ['Python 3.1', 'Python 2.7']

    @tseaver
    Copy link
    Author

    tseaver commented Nov 8, 2010

    Because application code which tests for the presence of
    an object in a WeakSet may not be able to ensure that the
    object is weak-referenceable, the set should just return
    False for any object passed to '__contains__' which causes
    'ref' to raise a TypeError.

    Patch forthcoming.

    @tseaver tseaver added the stdlib Python modules in the Lib dir label Nov 8, 2010
    @birkenfeld
    Copy link
    Member

    A result from IRC is that to be consistent with normal sets, only TypeErrors coming from ref() should be caught, but not those TypeErrors from the actual membership test (i.e. the hash functioon of the value).

    @tseaver
    Copy link
    Author

    tseaver commented Nov 8, 2010

    A new version of the patch, which only traps TypeErrors raises
    by ref(item). The test now ensures that hashing errors are
    propagated, for compatibility with standard sets.

    @tseaver
    Copy link
    Author

    tseaver commented Nov 8, 2010

    "One more time, with feeling."

    Sorry for the noise, the last patch was not against the 2.7 trunk.

    @tseaver
    Copy link
    Author

    tseaver commented Nov 8, 2010

    FTR, the patch applies cleanly to the py3k branch, too. (Michael
    pointed out that the original code was copied directly from there).

    @pitrou
    Copy link
    Member

    pitrou commented Nov 8, 2010

    What is the behaviour of WeakKeyDictionary here? I don't really agree that TypeError should be silenced.

    @tseaver
    Copy link
    Author

    tseaver commented Nov 8, 2010

    WeakKeyDictionary's __contains__:

        def __contains__(self, key):
            try:
                wr = ref(key)
            except TypeError:
                return 0
            return wr in self.data

    @rhettinger
    Copy link
    Contributor

    I don't have any other insights on this one. Assigned by to Antoine who appears to have put some thought into it.

    @rhettinger rhettinger assigned pitrou and unassigned rhettinger Nov 22, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Nov 22, 2010

    Well if WeakKeyDictionary silences the TypeError from ref(), it sounds consistent for WeakSet to silence it too.

    @birkenfeld
    Copy link
    Member

    I agree; committed in r86960.

    If anyone wants to change this to raise TypeError for objects that are both unhashable and unrefable, please speak up.

    @tseaver
    Copy link
    Author

    tseaver commented Dec 3, 2010

    This fix needs backporting to the 3.1 and 2.7 branches as well.

    @tseaver tseaver reopened this Dec 3, 2010
    @birkenfeld
    Copy link
    Member

    Don't worry, it will be ported.

    @tseaver
    Copy link
    Author

    tseaver commented Jan 31, 2011

    georg.brandl (2010-12-03 10:20):

    Don't worry, it will be ported.

    When? Why would it *not* be merged to the 2.7 and 3.1 branches at the
    time the bug is closed? Delaying the port only increases the chance
    that the fix will fall between the cracks.

    In particular, I would like confirmation that the fix *will* be ported
    before 2.7.2 is released: this bug causes the 2.7 Zope buildbots
    to fail, which means they are disabled until the fix is released.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 31, 2011

    Let's reopen until then.

    @pitrou pitrou reopened this Jan 31, 2011
    @pitrou pitrou removed their assignment Jan 31, 2011
    @birkenfeld
    Copy link
    Member

    Because I don't think porting every change immediately is worth my time when I can do it much faster in mass-merges.

    I know other developers do this differently, but since I use svnmerge to do my mass-merges, there won't be any falling through cracks.

    @birkenfeld
    Copy link
    Member

    Backported to 3.1 in r88556, and 2.7 in r88564.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants