I don't understand why you are removing this bit of code.

From my point of view, it looks like it does the right thing. If you ask for a parameter at position 1.. it will give you all the positional parameters except the first one. Why would we not want that?

Ah, I see a comment about dataflow inconsistencies. I would recommend to include them in the git history next time, so it's possible to know what they look like... We do currently use arguments/parameters in a slightly different way than most other dataflow libraries, which means we get inconsistency errors. (which is still something we should solve 😮‍💨)

Anyway, can you please show the inconsistency warnings, then we can discuss the right approach?

Anyway, can you please show the inconsistency warnings, then we can discuss the right approach?

Sure, you can see some here: https://github.com/github/semmle-code/actions/runs/11020219625/job/30604524761#step:15:830
(Found via the red x on the commit before they were fixed. But that will expire at some point.)

Interesting idea to quote them in the commit message, I will remember that 👍

By the same token, I should quote them here rather than link...
For this snippet:

def use_of_apply(func, args):
    apply(func, args)

we get alerts

stating that all parameters are now in conflict.

Interesting idea to quote them in the commit message, I will remember that 👍

You can commit the files in the CONSISTENCY directory as part of your commit when the warnings are introduced, then you don't need to quote anything in the commit message. (and obviously remove them once you resolve the problems).

(this is all assuming you run the consistency checks locally, which you can do by setting the --consistency-queries flag when using the CLI)

Ah, that is even more structured :-)

What I would have liked is some details around why we need this. Here's the details I've gathered myself

1. we only create these parameter positions when Argument[<n>..] is used in MaD

   codeql/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll

   Lines 60 to 62 in da5abc8

   	TPositionalParameterLowerBoundPosition(int pos) {
   	FlowSummaryImpl::ParsePositions::isParsedArgumentLowerBoundPosition(_, pos)
   	} or

2. Currently we only use that in a test-file (see this search)
3. So the addition of Argument[0..] in this PR caused us to start having a TPositionalParameterPosition available, which highlighted some problems.

We use DataFlowCallable.getParameter in multiple places, for example in ParameterNodeImpl:

dataflow library generally expects parameters to only be found at one position. (which is why we're now getting the consistency warnings on all positional arguments)

The one place that I could find where removing isPositionalLowerBound support from DataFlowFunction.getParameter could end up having impact is in type-tracking implementation:

However, since the assumption is that isPositionalLowerBound is ONLY used for "library callables", and those are not supported in callStep anyway, the change doesn't have any implications. (and flow-summary-impl uses parameterMatch internally, so no problem there)

That made me think, what about the "simple" library-callables defined with getACallSimple (not using API graph)? I think I found the logic that handles parameter positions for those, and I don't see anything about isPositionalLowerBound. I guess this issue hasn't come up since (a) we haven't used isPositionalLowerBound for anything, and (b) we only use getACallSimple in tests -- but the details are a bit foggy, so I might have gotten something wrong in my reasoning. _(I see some test failures in the PR that introduced isPositionalLowerBound, although the details are not available any longer (and not part of the git history)... I assume that must have been for the handling of flow-summaries for "library callables" that uses the API graph, for which we have tests).

Overall I think it looks good to remove this code from getParameter, but it wasn't really clear to me before spending almost an hour looking through these things. I think we might have a bug in our handling of getACallSimple, which honestly doesn't seem super important.

However, I would strongly recommend a comment explaining why isPositionalLowerBound shouldn't be handled in getParameter (so it doesn't just look like an omission when someone checks if all parameter positions are handled in there)