Remove (not used)?

Absolutely! I thought I was rid of that...

If you want, you can replace it with key = any(KeyValuePair kvp).getKey().(StrConst).getS() to make it smaller.

Neat, done.

At some point you probably also want to implement the clearsContent predicate, for example dict.clear() will clear dictionary contents. However, that is easiest implemented if you have use-use flow.

Yes, it is still outstanding if we should make an effort to switch to use-use flow.

An alternative here is to introduce synthetic nodes that record the key that was read, so you get more precision. So something like this will work:

dict["a"] = "taint"
dict["b"] = "no taint"
dict = {k:v+"" for (k,v) in dict.items()}
Sink(dict["b"]) // not tainted

You will then add a readStep from dict with content TDictionaryElementContent(key) to TSynthesizedDictCompNode(key, v), a local step from TSynthesizedDictCompNode(key, v) to TSynthesizedDictCompNode(key, v+"") and a storeStep with content TDictionaryElementContent(key) from TSynthesizedDictCompNode(key, v+"") to the entire comprehension.

This is an interesting use of synthetic nodes that we should keep in mind. It sparked thoughts that I wrote down here. It is probably too much precision to strive for right now, but perhaps we should add some test cases to record the ambition :-)

Can be replaced with exists(any(TupleNode tn).getElement(index)) to make it smaller.

Neat, done.

I think we can probably limit these to only values that are actually used for indexing. This is probably best done by defining a separate class (e.g. named IntegerIndex) the instances of which are literals that appear in a subscripting operation. (I imagine this will not capture things like i = 5; y = list[i], but I don't know that this works in the present version anyway.)

I use the expression suggested by Tom. Indeed it will not catch your example and indeed that is not handled at present either.

Again, I think this can be usefully restricted (and also extended). I would disregard all string constants that are not used in subscripting or dictionary-building operations, but also potentially add anything that appears as the name of a keyword argument, to capture things like d = dict(x=1, y=2).

I use the expression suggested by Tom augmented to include names of keyword arguments.

I think doing it this way is probably plenty robust.

The way I see it, if dataflow has managed to track a list/set to this point, then I would feel pretty safe in assuming the pop method does what we imply here. Either way, I wouldn't want to introduce a dependence on Value, though it should be pretty safe in this case.

I have updated the comment(s).

This will only work for
x+1 for x in <sequence literal>, such as x+1 for x in [1,2,3] and not for

xs = [1,2,3]
ys = [x+1 for x in xs]

is that by purpose? (I can see the tests only use sequence literals, so tests won't tell you anything about this)

Thanks, no, that is not on purpose. I meant to change this once I had a more reliable implementation of getCompIter (which I hope we do now, but I would love input on how to do it properly, that still feels like it might need extractor changes).

I looked into how list comprehensions are exposed through QL. If we use the ListComp class directly instead of Comp, we can get the information we want (instead of your helper methods)

from ListComp lc
select lc, lc.getIterable(), lc.getIterationVariable(0).getAStore()

Basically, what we need is Comp.getIterable(). The method is available on all subclasses of Comp, so it just needs to be exposed in the same way as Comp.getFunction().

I didn't look into how nested comprehensions work, so I would test that out before blindly accepting that we should always use 0 for the index 😄

P.S. The getCompFor functionality is already implemented in Comp.getNthInnerLoop, which is just a private member predicate, but I'm not sure you would even need that with the functionality above.

Thanks for digging this out. As discussed, nested comprehensions will be dealt with in a separate PR.