Flags toString is a little dangerous

[opotowsky]

Converting unordered flags to a string may or may not be dangerous. A downstream repo certainly is using it in a dangerous way. Here's the code:

armi/armi/reactor/flags.py

Lines 180 to 188 in 96a5c88

	def _toString(cls, typeSpec):
	"""
	Make flag from string and fail if any unknown words are encountered.
	Notes
	-----
	This converts a flag from ``Flags.A|B`` to ``'A B'``
	"""
	return str(typeSpec).split("{}.".format(cls.__name__))[1].replace("|", " ")

Maybe this is a #wontfix thing and people just need to not do dangerous things. But I wanted to demonstrate how this could be used in a dangerous way.

Say you want to get a parameter, rxFuelAxialExpansionCoeffPerTemp. You have the following info: type = "Fuel" and flags = AXIAL | EXPANSION.

In python 3.9 when you call flags.toString(), you get "Axial Expansion". You can remove the space and use that to construct your parameter name. In python 3.11, you get "Expansion Axial", in which case you need to know ahead of time to flip the flags before moving forward.

[john-science]

Your "expansion axial" example there feels like it should be caught by our unit tests. We run the same tests in Python 3.9 - 3.12, you'd think we would have noticed this.

Perhaps we need to add a test?

[john-science]

Wait, is someone calling the private _to String() outside ARMI?

[jakehader]

No,

armi/armi/reactor/flags.py

Line 304 in 96a5c88

def toString(cls, typeSpec):

is being called.

[opotowsky]

@jakehader shared this, which might explain it

But @john-science was writing a unit test and found the ordering is not guaranteed in py39. So the toString method will likely return an alphabetized version.

I would argue the toString method should only be used on individual flags, personally. So it should only accept a single flag. Then we don't need to worry about ordering at all.

[john-science]

We just found out that Flags.toString() in Python 3.9, for multiple flags, returns them in random order every time. There is currently NO guarantee about flag ordering from this method.

I don't like that, so I'll just fix it to be alphabetical. Consistency is better than randomness.

[keckler]

Did this get turned into an issue? I do not see one, but am just wondering if this is actually something that will be done or not?

[john-science]

The PR is up: #1912