Skip to content

difflib._check_types allows string inputs instead of sequences of strings as documented #115801

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

Closed
lampwins opened this issue Feb 22, 2024 · 1 comment
Labels
type-bug An unexpected behavior, bug, or error

Comments

@lampwins
Copy link

lampwins commented Feb 22, 2024

Bug report

Bug description:

Both difflib.unified_diff and difflib.context_diff document that a and b input arguments are to be lists of strings. These functions perform argument type checking by way of difflib._check_types, however this function allows a and b to be direct str arguments. Technically this does not cause a failure in difflib.unified_diff (and I assume the same is true for context_diff but I have not tested it), however, for very large strings a and/or b, difflib.unified_diff is exponentially slower to calculate the diff, because the underlying SequenceMatcher is optimized to compare two lists of string, not two sequences of chars.

We can obviously see that the implementation of _check_types uses a seemingly naive type check of the first element in a and b which will pass for both the documented input of a list of strings, but also for a positive length string itself.

def _check_types(a, b, *args):
    # Checking types is weird, but the alternative is garbled output when
    # someone passes mixed bytes and str to {unified,context}_diff(). E.g.
    # without this check, passing filenames as bytes results in output like
    #   --- b'oldfile.txt'
    #   +++ b'newfile.txt'
    # because of how str.format() incorporates bytes objects.
    if a and not isinstance(a[0], str):
        raise TypeError('lines to compare must be str, not %s (%r)' %
                        (type(a[0]).__name__, a[0]))
    if b and not isinstance(b[0], str):
        raise TypeError('lines to compare must be str, not %s (%r)' %
                        (type(b[0]).__name__, b[0]))
    for arg in args:
        if not isinstance(arg, str):
            raise TypeError('all arguments must be str, not: %r' % (arg,))

This would be rather trivial to fix in _check_types but I want to first make sure that the documentation of a list of string is to be considered correct behavior?

CPython versions tested on:

3.8, 3.9, 3.10, 3.11, 3.12, 3.13, CPython main branch

Operating systems tested on:

Linux, macOS

Linked PRs

@serhiy-storchaka
Copy link
Member

I agree that passing strings instead of sequences of strings is most likely a user bug. But there is a tiny chance that this is used intentionally, so it is safer to not change this in maintained releases.

mrahtz pushed a commit to mrahtz/cpython that referenced this issue Jun 30, 2024
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants