JointIndex should not be iterable

[AlexandreAmice]

What happened?

It should not be possible to write the loop

for (JointIndex joint_index(0); joint_index < plant.num_joints();
       ++joint_index)

as MultibodyPlant's joint indexes need not be contiguous (e.g. after calling RemoveJoint on the plant pre-finalize)

I think one way to prevent this footgun would be to remove the increment/decrement methods from JointIndex. If this is an acceptable change, should this be done for all typesafe indices? (I haven't thought about this)

The problem of course is that one could write:

for (int i = 0; i < plant.num_joints();
       ++i) {
JointIndex joint_index(i);
}

Another possible solution would be to make multibody plant a friend of JointIndex and make the JointIndex constructor protected.

Version

No response

What operating system are you using?

No response

What installation option are you using?

No response

Relevant log output

No response

[AlexandreAmice]

cc @agarwal-abhinav who unearthed the bug in #21860 which was introduced in #21308

[sherm1]

We would need a new type for indices that are not iterable. I've been using the term "ordinal" for consecutive, iterable indices (there is a type JointOrdinal in use by the topology code). Many of the existing index types are currently intended to be iterable but as we make more elements removable that will likely change. Maybe we should rename TypeSafeIndex to TypeSafeOrdinal, and invent a new, non-iterable class for anything that shouldn't be iterated.

[joemasterjohn]

To me, the problem stems beyond whether this particular TypeSafeIndex is iterable (or explicitly construct-able). It would still be easy for a user to do bad things with out-of-date indices (even without iterability or explicit construction):

std::vector<JointIndex> indices = plant.GetJointIndices();
...
plant.RemoveJoint(plant.get_joint(indices[i]));
...
...
for(JointIndex index : indices) {
  DoSomething(plant.get_joint(index));  // ERROR joint index was removed
}

The problem of "stale" indices still remains one way or another. We thought about this quite a bit in #21397 and #20711 and decided the simplest thing was to strongly encourage the use of GetJointIndices() when looping over joints (after removal), and otherwise leave the burden of accounting for removed joint indices on the user (should they decide to keep indices around for a while).

It could be nice to expose the "ordinal" idea that we use internally. Maybe even disallow the use of APIs that expect JointIndex (e.g. MbP::get_joint()) after a removal has happened, and force the user to use "ordinal" APIs that have simper semantics? This might be overkill though, as the most common use-case remains: no joint removal / contiguous joint indices.

[amcastro-tri]

I think that at a minimum we should document in get_joint() the need to call GetJointIndices() to obtain valid indexes.
AFAICT RemoveJoint() does a good job documenting this requirement.

I agree that not being able to write such loops would be best, but that'll rqeuire a larger effort.