Allowing Metadata to change after construction is unsafe

[Yurlungur]

Recently @brryan and I tracked down a bug in Phoebus that was caused by the following pattern in a package initialization:

Metadata m({some set of flags});
if (condition) {
  m.Set(Metadata::FillGhost);
}

This was a problem because no prolongation/restriction ops were being set for the variable with this metadata. Default prolongation/restriction ops are provided, but are only automatically registered in the constructor if IsRefined() returns true at construct time, not later. So the metadata object was being created with empty prolongation/restriction ops and then set to prolongate and restrict at boundary communication time.

I have created a pull request, #843 , to resolve this particular issue, but I think it exposes a deeper issue with metadata: we perform sanity checks on metadata only at initialization, but flags can be set post-initialization to whatever is desired. I would propose that in the long term, we make Metadata an immutable, so that flags can not be changed after initialization. There are a few places in the infrastructure where Metadata::Set is called:

1. In sparse pools, this is used to set certain properties of each variable in the pool
2. In swarms, the metadata is set to particle for swarm variables and to swarm for swarms.

We could either change how this functionality is written to construct immutable metadata objects, or we could make Metadata::Set private and expose this machinery to swarms and pools in particular but not downstream codes.

Or we could continue to be careful about guard rails in metadata, as I did with my PR, and let this be for now. What do people think? Pinging @lroberts36 , @pgrete @bprather @brryan for your thoughts.

By the way there is one obvious place this can happen beyond the prolongation/restriction, which is sparse thresholds. If Metadata is constructed dense and then set to sparse, the thresholds will not be set correctly.

[lroberts36]

I think my preferred solution in a vacuum would be to create a MetaDataDescriptor class that can be used to set flags, sparse information, etc. and is mutable. Then, only allow MetaData to be constructed from a MetaDataDescriptor object and have all fields of MetaData be const. Refactoring to use this might be more work than it is worth though.

[pgrete]

I'm definitely in favor of handling this more explicitly (over adding [and potentially forgetting] guard rails at multiple places) which should also lower maintenance effort.

Whether the explicit handling implemented through immutable Metadata or clearer modification methods [which ideally would be called in the constructor, too] I have no strong opinion. In the long run I might see advantages going with the latter in terms of flexibility/extensibility (e.g., adjusting a sparse threshold at runtime).

[jdolence]

As part of #921, I'm adding something a few of us have talked about where variables added to a package automatically get a metadata flag set that corresponds to the package in which they are initialized. I think this requires I call Set from the AddField function in StateDescriptor (and probably corresponding places for sparse and particles).