[Breaking change][WIP] Disallow accessing mutable param by value

[BenWibking]

PR Summary

Throw an exception if a mutable parameter is accessed by value.

This means that mutable parameters must be accessed with StateDescriptor::MutableParam and non-mutable parameters must be accessed with StateDescriptor::Param. This helps to avoid bugs in downstream application codes due to accidental use of pkg->Param with mutable params.

PR Checklist

• [ ] Code passes cpplint
• [ ] New features are documented.
• [ ] Adds a test for any bugs fixed. Adds tests for new features.
• [ ] Code is formatted
• [ ] Changes are summarized in CHANGELOG.md
• [x] Change is breaking (API, behavior, ...)
  • [ ] Change is additionally added to CHANGELOG.md in the breaking section
  • [ ] PR is marked as breaking
  • [ ] Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
• [ ] CI has been triggered on Darwin for performance regression tests.
• [ ] Docs build
• [ ] (@lanl.gov employees) Update copyright on changed files

[Yurlungur]

This is a reasonable restriction, I think... But it might be desirable to add a non-reference version without this restriction. If you pull a param with no intent to change it, it shouldn't necessarily be a problem.

[pgrete]

I'm now confused (trying to understand the original issue that cause this PR). Mutability for params only restricts the update of the value in the map (which is a pointer to an new object of the type put in Params), doesn't it? If we have an object inside Params that is not mutable, and we plain Get it, we can still modify the state of the object stored behind the pointer, correct? We can just not replace the object with another object (as it would result in changing the value of pointer).

[BenWibking]

I'm now confused (trying to understand the original issue that cause this PR). Mutability for params only restricts the update of the value in the map (which is a pointer to an new object of the type put in Params), doesn't it? If we have an object inside Params that is not mutable, and we plain Get it, we can still modify the state of the object stored behind the pointer, correct? We can just not replace the object with another object (as it would result in changing the value of pointer).

If that's the case, I don't understand why this change fixed the problem either: https://github.com/parthenon-hpc-lab/athenapk/blob/0a2a06315f9f424306f91b9a2b91403473192beb/src/pgen/precipitator.cpp#L299

I changed this from hydro_pkg->Param to hydro_pkg->MutableParam and then changed the next line from .Generate to ->Generate and it fixed the problem.

[Yurlungur]

I'm now confused (trying to understand the original issue that cause this PR). Mutability for params only restricts the update of the value in the map (which is a pointer to an new object of the type put in Params), doesn't it? If we have an object inside Params that is not mutable, and we plain Get it, we can still modify the state of the object stored behind the pointer, correct? We can just not replace the object with another object (as it would result in changing the value of pointer).

If that's the case, I don't understand why this change fixed the problem either: https://github.com/parthenon-hpc-lab/athenapk/blob/0a2a06315f9f424306f91b9a2b91403473192beb/src/pgen/precipitator.cpp#L299

I changed this from hydro_pkg->Param to hydro_pkg->MutableParam and then changed the next line from .Generate to ->Generate and it fixed the problem.

Ah I see. Get returns a const reference so it cannot be modified. If you assign by value, you can modify of course, but it won't change anything. If you assign by reference, const correctness will prevent you from modifying.

I would argue this is expected and reasonable behavior. Sometimes you will want to get the value of a mutable param without being able to update it.

[BenWibking]

I'm now confused (trying to understand the original issue that cause this PR). Mutability for params only restricts the update of the value in the map (which is a pointer to an new object of the type put in Params), doesn't it? If we have an object inside Params that is not mutable, and we plain Get it, we can still modify the state of the object stored behind the pointer, correct? We can just not replace the object with another object (as it would result in changing the value of pointer).

If that's the case, I don't understand why this change fixed the problem either: https://github.com/parthenon-hpc-lab/athenapk/blob/0a2a06315f9f424306f91b9a2b91403473192beb/src/pgen/precipitator.cpp#L299 I changed this from hydro_pkg->Param to hydro_pkg->MutableParam and then changed the next line from .Generate to ->Generate and it fixed the problem.

Ah I see. Get returns a const reference so it cannot be modified. If you assign by value, you can modify of course, but it won't change anything. If you assign by reference, const correctness will prevent you from modifying.

I would argue this is expected and reasonable behavior. Sometimes you will want to get the value of a mutable param without being able to update it.

Ah... I understand now. Yes, I agree that is reasonable.

@pgrete I think the correct thing to do to avoid this problem in the future would be to delete the copy constructor and copy assignment operator of the FewModesFT class in AthenaPK.

[BenWibking]

Closing in favor of https://github.com/parthenon-hpc-lab/athenapk/pull/110.

[BenWibking]

Hmm, that doesn't actually work, since the copy constructor is used by Params:

In file included from /Users/benwibking/athenapk/external/parthenon/src/mesh/mesh.hpp:37:
In file included from /Users/benwibking/athenapk/external/parthenon/src/application_input.hpp:22:
In file included from /Users/benwibking/athenapk/external/parthenon/src/bvals/boundary_conditions.hpp:22:
In file included from /Users/benwibking/athenapk/external/parthenon/src/interface/meshblock_data.hpp:27:
In file included from /Users/benwibking/athenapk/external/parthenon/src/interface/sparse_pack_base.hpp:30:
/Users/benwibking/athenapk/external/parthenon/src/interface/state_descriptor.hpp:132:25: error: call to deleted constructor of 'utils::few_modes_ft::FewModesFT'
    params_.Add<T>(key, value, is_mutable);
                        ^~~~~
/Users/benwibking/athenapk/src/pgen/cluster.cpp:408:16: note: in instantiation of function template specialization 'parthenon::StateDescriptor::AddParam<utils::few_modes_ft::FewModesFT>' requested here
    hydro_pkg->AddParam<>("cluster/few_modes_ft_v", few_modes_ft);
               ^
/Users/benwibking/athenapk/src/pgen/../utils/few_modes_ft.hpp:51:3: note: 'FewModesFT' has been explicitly marked deleted here
  FewModesFT(FewModesFT const &) = delete;
  ^
/Users/benwibking/athenapk/external/parthenon/src/interface/params.hpp:59:38: note: passing argument to parameter 'value' here
  void Add(const std::string &key, T value, bool is_mutable = false) {

@Yurlungur @pgrete is there an obvious solution I'm overlooking?

[BenWibking]

@Yurlungur why does Params copy-construct? That does not make sense to me.

[Yurlungur]

One solution would be to give params an emplace add so that you could put objects into params without copying them. That might be tricky to get right with all the I/O machinery though... i.e., if you want to restart this object you'll have problems there. You could play some dirty games under the hood and make your FewModesFT own pointers or references to things inside it you want to modify... then it wouldn't matter whether it's a mutable param or not.

[BenWibking]

emplace would make sense to me. I don't really understand how the restart machinery works... where would things start to go wrong there?

Making FewModesFT non-owning would be ideal IMO, but I will defer to @pgrete as to whether that is acceptable.

[Yurlungur]

@Yurlungur why does Params copy-construct? That does not make sense to me.

It's because the object is passed in so it needs to be copied. Actually we could maybe change this to a move. That would probably be fine.

[pgrete]

Making FewModesFT non-owning would be ideal IMO, but I will defer to @pgrete as to whether that is acceptable.

What do you mean @BenWibking ? I'm not sure I follow the suggestion.

[BenWibking]

Making FewModesFT non-owning would be ideal IMO, but I will defer to @pgrete as to whether that is acceptable.

What do you mean @BenWibking ? I'm not sure I follow the suggestion.

I just mean @Yurlungur's suggestion: "You could play some dirty games under the hood and make your FewModesFT own pointers or references to things inside it you want to modify... then it wouldn't matter whether it's a mutable param or not."

But that might not be worth it, or useful in general. I think the main issue is that the ownership semantics were not clear to me when I wrote the code.