Control access to state variables in transform method

[cmacmackin]

This is a major refactor of how components work. They are now required to create a Permissions object in their constructor, which contains information on which state variables the transform method is allowed to read and write. Trying to read/write a variable without permission will result in a runtime error. This should help ensure people are more thoughtful in their design of components and prevent inadvertent side-effects. It also provides necessary information for implementing the automatic ordering of the execution of components (#384).

Closes #383

[ZedThree]

I think this is broadly the right direction, although I haven't got a feel for exactly how ergonomic it is yet or how all the pieces connect exactly

[ZedThree]

Does this now include #399? Is that on purpose?

[cmacmackin]

Does this now include #399? Is that on purpose?

Yes. I didn't want to add all the permission for the old closure implementation and then have to re-do it for the new one. The plan is to merge #399 first though. Then we can rebase and do final review on this one.

[codecov]

Codecov Report

❌ Patch coverage is 62.75072% with 390 lines in your changes missing coverage. Please review.
✅ Project coverage is 29.52%. Comparing base (e0f1ebf) to head (a2ac1e8).
⚠️ Report is 12 commits behind head on master.

Files with missing lines	Patch %	Lines
src/vorticity.cxx	0.00%	28 Missing ⚠️
src/polarisation_drift.cxx	0.00%	21 Missing ⚠️
src/classical_diffusion.cxx	0.00%	18 Missing ⚠️
include/detachment_controller.hxx	0.00%	15 Missing ⚠️
src/sheath_boundary_simple.cxx	0.00%	15 Missing ⚠️
include/fixed_temperature.hxx	0.00%	13 Missing ⚠️
include/set_temperature.hxx	0.00%	13 Missing ⚠️
src/sheath_boundary_insulating.cxx	0.00%	12 Missing ⚠️
include/fixed_fraction_radiation.hxx	0.00%	11 Missing ⚠️
src/relax_potential.cxx	0.00%	11 Missing ⚠️
... and 49 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
+ Coverage   26.41%   29.52%   +3.11%     
==========================================
  Files          90       94       +4     
  Lines        8105     8768     +663     
  Branches     1133     1228      +95     
==========================================
+ Hits         2141     2589     +448     
- Misses       5745     5914     +169     
- Partials      219      265      +46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ZedThree]

I've made a bunch of mostly minor comments. I realised why I've been struggling to get my head around some of the implementation details, and that's because the design feels a little backwards. We have an array where each index is a permission level and the element is a region. But if we look at say unix permissions, we have a structure where the position is the user/group and the value is the permission level (and this is a bit-field, being able to set read | write permissions).

What if we had something like this:

enum class AccessRights { none, read_if_set, read, write, final_ };

enum class PermissionRegions { interior, boundaries };

struct AccessPermission {
  AccessRights interior = AccessRights::none;
  AccessRights boundaries = AccessRights::none;

  auto operator[](PermissionRegions region) {
    switch(region) {
    case PermissionRegions::interior:
       return interior;
    case PermissionRegions::boundaries:
       return boundaries;
    }
  }
};

Is this way round easier to reason about? It feels a lot simpler. To take the examples from the AccessRights docstring, we'd now have:

auto only_read_if_set = AccessPermission {
  .interior = AccessRights::read_if_set,
  .boundaries = AccessRights::read_if_set
};

auto read_only = AccessPermission {
  .interior = AccessRights::read,
  .boundaries = AccessRights::read
};

auto write_boundaries = AccessPermission {
  .interior = AccessRights::none,
  .boundaries = AccessRights::write
};

auto read_write_everywhere = AccessPermission {
  .interior = AccessRights::write,
  .boundaries = AccessRights::write
};

auto final_write_boundaries_read_interior = AccessPermission {
  .interior = AccessRights::read,
  .boundaries = AccessRights::write
};

Checking if a AccessPermission is valid for a given region and access level is just:

read_only[region] >= AccessRights::read

This way round would of course also work with an array, but I keep suggesting structs because names are really important, and the lack of them in the current design makes e.g. applyLowerPermissions very difficult to parse. I'm not even sure it's needed this proposed design.

Applying the higher of two permissions is easier:

auto result = AccessPermission {
  .interior = std::max(lhs.interior, rhs.interior),
  .boundaries = std::max(lhs.boundaries, rhs.boundaries),
};

[cmacmackin]

I do agree that it is more intuitive to store the level of access for each region then the regions with each level of access. In fact, that was my original plan. The reason I changed it was that @bendudson was expressing some concerns about performance overhead. I'm pretty sure what I've done is more performant. Checking whether we have permission to read or write a particular variable will involved:

• 1 std::map lookup
• 1 one array-indexing
• 1 bitwise-&
• 1 equality comparison.

This remains constant regardless of how many regions we define (e.g., if we start using all the same types of regions defined in BOUT++).

Assuming you need to check the permissions over n regions, your approach would involved

• 1 std::map lookup
• n calls to AccessPermissions::Operator[] (I'm assuming the compiler can heavily optimise the switch-case block to be similarly efficient to array-indexing)
• n greater-than-or-equal-to comparisons
• iterating over all n regions of interest

When we only have two regions there is little to no difference here. If we ever break down the boundaries into more fine-grained regions (e.g., RGN_NOBNDRY, RGN_XGUARDS, RGN_YGUARDS, RGN_ZGUARDS, RGN_CORNERS) then the savings start to become more significant. Moreover, the ability to use a bit-set to express the the desired region makes it straightforward to handle cases like RGN_ALL, RGN_NOX, RGN_GUARDS, etc.

So, I guess the fundamental question is whether we should take a YAGNI approach to keep the implementation more intuitive based on the current features. I think what I've done is more future-proof but that future might never come. The performance probably isn't a big deal, especially since these checks will likely be switched off when running big simulations.

[cmacmackin]

The other problem with refactoring the way permissions are stored along the lines @ZedThree suggests is that I'm only on this project for another week, so it's not clear I'd have time to do that, get it reviewed, etc.

[cmacmackin]

@ZedThree I've created issues for all of your comments I have not implemented. Could you mark this as approved (or at least remove the request for changes)?

[bendudson]

Thanks @cmacmackin ! This is a quite monumental effort. Very impressive. I'm slightly sorry to see the Wild West tamed, but this is a much more organised and robust solution. Thanks for the manual writeup, and of course all the examples in components. I think this will be good to work with, and it removes some of the footguns that kept going off in our saloon.

I don't promise that I've read every line or tried this code out. I also don't promise that I haven't. I think this is the right direction to go in, and we can refine it as we use it more. Thanks Chris!

[mikekryjak]

I think this documentation will be hard to understand by people new to C++, and could use a couple of practical examples, for example based on evolve_density or similar. One thing that was a bit confusing in the beginning was that you set the permissions using names which are then substituted later. I was intuitively looking for a line saying something like setPermissions and couldn't find it, which was confusing.

[cmacmackin]

@ZedThree I believe I have addressed or opened issues for all of your comments. Could you please approve the changes?

@mikekryjak I've updated the documentation as you requested. Could you confirm whether this is ready to be merged?

[mikekryjak]

Thanks for the docs improvements @cmacmackin, it reads great now.