Possible inconsistencies when using "getDeclarations()"

[MichalKekely]

Issue

The issue is that iterating over getDeclarations() for example on IR::P4Action (or possibly other nodes) actually boils down to iterating over internal ordered_map of the parameter declarations held by IR::IndexedVector. The problem is that the order of those declarations might be different than the actual order of the items in the vector itself (which could be iterated directly for example by using getParameters()).

Replication

Most of the time this does not happen and both actually return the same order of elements. But I found out that for example a P4 action construction like this:

typedef bit<4> my_type;
action a(my_type x, bit<4> b)

Manifests this issue. So calling getDeclarations() on the a action node returns the order of parameters b, x, but the getParameters returns the correct order of x, b.

Possible causes

My guess is that the x parameter uses a custom type and the type gets replaced at some point by the base type, which essentially creates a new node. I tried to dump the parameters and indeed the x parameter has a lot higher node ID. Is it possible that the internal ordered_map of IR::IndexedVector is simply ordered by the node/declaration IDs and therefore since the x parameter was modified by some passes it "moves" in the ordered_map's order? If so is this an issue that should be fixed or is the getDeclarations simply a method that does not guarantee anything about the order of the declarations it returns (in which case it might be made clear somewhere)?

[fruffy]

Possible related: https://github.com/p4lang/p4c/issues/43? More funky behavior by the IR::IndexVector class.

[mihaibudiu]

I don't think we promise anywhere that getDeclarations() return results in a particular order. It's not related to #43, there is no mutation here.

[vlstill]

I don't think we promise anywhere that getDeclarations() return results in a particular order. It's not related to #43, there is no mutation here.

I agree. I don't think getDeclaration should promise any ordering. The ordering did probably change in this particular case recently, with reprecement of ordered_map by string_map in indexed vector.

I suggest this should be closed as won't fix. We might add an explicit comment to getDeclaration, but I think it is unlikely it will be read (unless it is replicated in all declarations of getDeclaration at least).

[asl]

I agree. I don't think getDeclaration should promise any ordering. The ordering did probably change in this particular case recently, with reprecement of ordered_map by string_map in indexed vector.

string_map does preserve insertion order. I remember things got broken when this invariant was not fulfilled. So, we do have code that relies on this IIRC.