Support for large numbers of args/kws with Python 3.10 - #7866

@ehsantn and @benwilliamgraham thanks for doing an initial review.

@stuartarchibald could you label this PR as ready for review? Thanks.

I did a quick check on coverage and found the following lines to be never covered. I only checked a random 20% of the test suite and all the new tests thus the coverage data may not be 100% accurate. If these lines are not really dead, they should have more comments to explain them.

Thanks @sklam. I'll update my tests to add checks to two of the regions and I'll expand the comments on those regions.

Two of the areas that are uncovered were my attempts to be conservative against minor changes in CPython or the IR generated because I wasn't sure if my tests are exhaustive. If you feel like they are unnecessary I'm happy to remove them.

I did a quick check on coverage and found the following lines to be never covered. I only checked a random 20% of the test suite and all the new tests thus the coverage data may not be 100% accurate. If these lines are not really dead, they should have more comments to explain them.

Thanks @sklam. I'll update my tests to add checks to two of the regions and I'll expand the comments on those regions.

Two of the areas that are uncovered were my attempts to be conservative against minor changes in CPython or the IR generated because I wasn't sure if my tests are exhaustive. If you feel like they are unnecessary I'm happy to remove them.

@sklam I pushed an update to the tests and updated my comments.

I don't think at present the transform will be able to handle inline control flow at the call site. For example, the rather contrived:

import numba
import dis

@numba.njit
def my_func1(a=None, b=None, c=None, d=None, e=None, f=None, g=None, h=None, i=None, j=None, k=None, l=None, m=None, n=None, o=None, p=None):
    return my_func2(
        a=a,
        b=b,
        c=c,
        d=d,
        e=e,
        f=f,
        g=g,
        h=h,
        i=i,
        j=j,
        k=k,
        l=l,
        m=m,
        n=n,
        o=o,
        p = False if a else True
    )


@numba.njit
def my_func2(a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p):
    return

print(dis.dis(my_func1.py_func))
print(my_func1())

which gives a bytecode that has:

  6          96 LOAD_CONST              16 ('p')

 22          98 LOAD_FAST                0 (a)
            100 POP_JUMP_IF_FALSE       55 (to 110)
            102 LOAD_CONST              17 (False)
            104 MAP_ADD                  1

  6         106 CALL_FUNCTION_EX         1
            108 RETURN_VALUE

 22     >>  110 LOAD_CONST              18 (True)
            112 MAP_ADD                  1

  6         114 CALL_FUNCTION_EX         1

at the location where p is resolved and added to the map prior to the call.

A similar issue exists in the nargs>30 use case:

import numba
import dis

@numba.njit
def my_func1(a01, a02, a03, a04, a05, a06, a07, a08, a09, a10,
             a11, a12, a13, a14, a15, a16, a17, a18, a19, a20,
             a21, a22, a23, a24, a25, a26, a27, a28, a29, a30,
             a31,
             ):
    return my_func2(a01, a02, a03, a04, a05, a06, a07, a08, a09, a10,
                    a11, a12, a13, a14, a15, a16, a17, a18, a19, True if a01 else False,
                    a21, a22, a23, a24, a25, a26, a27, a28, a29, a30, a31)


@numba.njit
def my_func2(a01, a02, a03, a04, a05, a06, a07, a08, a09, a10,
             a11, a12, a13, a14, a15, a16, a17, a18, a19, a20,
             a21, a22, a23, a24, a25, a26, a27, a28, a29, a30,
             a31):
    return

print(dis.dis(my_func1.py_func))
print(my_func1(*range(31)))

which gives:

Unsupported op-code encountered: list_to_tuple(info=('$phi92.1',))

as I think the transform fails due to the branch occurring within the LOAD_FAST+LIST_APPEND sequence that occurs ahead of the LIST_TO_TUPLE.

@stuartarchibald Thank you for finding these cases! You are definitely correct and I need to work on a more complete solution.

@stuartarchibald Thank you for finding these cases! You are definitely correct and I need to work on a more complete solution.

No problem.

I think given the rarity of the occurrence of these patterns, it would acceptable to do the following:

• Update this patch to catch the cases where control flow occurs. Inform users that control flow on arguments within call sites is not supported and refer them to docs on how to adjust their code.
• Add tests for the above.

The most common use cases of this somewhat rare pattern would then be covered and workarounds for the uncommon use cases of this rare pattern would be shown to users. Essentially, this patch makes the rare patterns work much more often, so I think it's better that it is reviewed and merged with acknowledgement of the imperfection instead of trying to cover everything. This of course also doesn't preclude further fixes later. What do you think?

@sklam I made your requested changes.

@stuartarchibald I made your changes as well and added tests. Proper error handling requires some extra code, so there may need to be to be a more detailed review. I also added a test for objmode which fails when the definitions are not properly updated.

@stuartarchibald I made your requested changes. There are possibly two things that need to be looked at:

1. Should the "abundance of caution" check contain a different error like an unreachable assertion. I don't have enough confidence with the code structure to say its truly unreachable so I just added a comment.
2. Is the comment explaining when to check the call expression clear enough?

If both these changes seem fine, I think everything else should be a direct application of your feedback and we should be ready to merge. Thank you so much for the detailed review!

@stuartarchibald I made your requested changes.

@njriasan Many thanks.

There are possibly two things that need to be looked at:

1. Should the "abundance of caution" check contain a different error like an unreachable assertion. I don't have enough confidence with the code structure to say its truly unreachable so I just added a comment.

I think it can be left as-is for now. Given it cannot be handled at present, if it turns out that there's use cases in practice that will trigger it, I would guess someone will make a request for it/report it. I don't think it warrants an exception containing the reportable condition text because it's already known that it might happen and it can't be handled.

2. Is the comment explaining when to check the call expression clear enough?

Couple of minor things I've noted in #7866 (review) else looks good thanks.

If both these changes seem fine, I think everything else should be a direct application of your feedback and we should be ready to merge. Thank you so much for the detailed review!

No problem, thanks for your efforts on this.

Thank you @stuartarchibald. I made the requested changes.