Skip to content

Conversation

@XuehaiPan
Copy link
Contributor

@XuehaiPan XuehaiPan commented Jan 7, 2026

Description

This is a regression after #5948


Piggy-backed commit f16de66:

Switch typos pre-commit hook to a mirror repo (adhtruong/mirrors-typos) to fix pre-commit autoupdate selecting the mutable v1 tag instead of pinned versions. See crate-ci/typos#390

Suggested changelog entry:

  • Placeholder.

Copy link
Collaborator

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird warning, but makes sense.

rwgk added 2 commits January 7, 2026 11:04
The previous comment referenced the MSVC warning but didn't explain
why the code is structured as two statements. The revised comment
clarifies the intent: fetching the value first ensures well-defined
evaluation order.
Switch from crate-ci/typos to adhtruong/mirrors-typos because
pre-commit autoupdate confuses tags in the upstream repo, selecting
the mutable `v1` tag instead of pinned versions like `v1.41.0`.

See crate-ci/typos#390
@rwgk rwgk requested a review from henryiii as a code owner January 7, 2026 19:24
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple trivial commits.

Will merge after I see that the CI has finished.

@rwgk
Copy link
Collaborator

rwgk commented Jan 7, 2026

Hm, I don't remember seeing this failure (EDIT: it was resolved by a rerun):

CI / 🐍 (macos-latest, 3.14t, -DCMAKE_CXX_STANDARD=20) / 🧪 (pull_request)

_____________________________ test_async_callbacks _____________________________

    @pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads")
    def test_async_callbacks():
        # serves as state for async callback
        class Item:
            def __init__(self, value):
                self.value = value
    
        res = []
    
        # generate stateful lambda that will store result in `res`
        def gen_f():
            s = Item(3)
            return lambda j: res.append(s.value + j)
    
        # do some work async
        work = [1, 2, 3, 4]
        m.test_async_callback(gen_f(), work)
        # wait until work is done
        from time import sleep
    
        sleep(0.5)
>       assert sum(res) == sum(x + 3 for x in work)
E       assert 18 == 22
E        +  where 18 = sum([5, 7, 6])
E        +  and   22 = sum(<generator object test_async_callbacks.<locals>.<genexpr> at 0x4d311ace610>)

Item       = <class 'test_callbacks.test_async_callbacks.<locals>.Item'>
gen_f      = <function test_async_callbacks.<locals>.gen_f at 0x4d31217a2d0>
res        = [5, 7, 6, 4]
sleep      = <built-in function sleep>
work       = [1, 2, 3, 4]

../../tests/test_callbacks.py:186: AssertionError

@rwgk rwgk merged commit d36f5dd into pybind:master Jan 8, 2026
151 of 152 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 8, 2026
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Jan 8, 2026
@XuehaiPan XuehaiPan deleted the appease-C4866 branch January 8, 2026 04:33
Comment on lines +1084 to +1087
// Fetch value before indexing into kwargs to ensure well-defined
// evaluation order (MSVC C4866).
PyObject *const arg_in_arr = args_in_arr[n_args_in + i];
kwargs[PyTuple_GET_ITEM(kwnames_in, i)] = arg_in_arr;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning still exists. The problem may be that the order of key casting and assignment is not well-defined.

  C:\Users\runneradmin\AppData\Local\Temp\pip-build-env-tsdbyp__\overlay\Lib\site-packages\pybind11\include\pybind11\pybind11.h(1087,1): error C2220: the following warning is treated as an error [C:\Users\runneradmin\AppData\Local\Temp\tmph2yqzrcl.build-temp\Release\src\_C.vcxproj]
  C:\Users\runneradmin\AppData\Local\Temp\pip-build-env-tsdbyp__\overlay\Lib\site-packages\pybind11\include\pybind11\pybind11.h(1087,1): warning C4866: compiler may not enforce left-to-right evaluation order for call to 'pybind11::detail::object_api<pybind11::handle>::operator[]' [C:\Users\runneradmin\AppData\Local\Temp\tmph2yqzrcl.build-temp\Release\src\_C.vcxproj]

@rwgk
Copy link
Collaborator

rwgk commented Jan 8, 2026

Do you have a way to test locally first?

@XuehaiPan
Copy link
Contributor Author

Do you have a way to test locally first?

Yes, working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants