What does "delete" mean in an OverrideEnvironment?

[mwichmann]

An OverrideEnvironment maps a dict on top of a dict: underneath is the _dict attribute of the calling Environment, and on top is the overrides attribute of the OE. Attribute fetches pull from overrides first, then from the backing dict. Assignment is to overrides only, you don't update the base Environment when assigning (__setattr__).

The OverrideEnvironment class has a __delitem__ method, but what it should actually do is not obvious. Let's illustrate with a contrived example:

Consider override env over where:

overrides = {'a': 1, 'b': 2}
base = {'b': 3, 'c': 4}

Then:

• over['a'] returns 1: it's only in overrides.
• over['b'] returns 2: from overrides, which wins over base.
• over['c'] returns 4: fetched from base because it wasn't found in overrides.

Currently,

1. del over['a'] deletes 'a' from overrides.
2. del over['b'] deletes 'b' from overrides and base.
3. del over['c'] deletes 'c' from base.

Case 1 is "obvious". Cases 2 and 3 are less so. Should deleting an item from our dictionary "view" ever modify the backing dictionary? That seems contrary to the spirit of a "temporary override" that is used to keep from having to change your active env. Case 2 is especially interesting - it is apparently designed so that if you delete from your view, then the key you deleted is no longer in your view, but it does that by modifying the backing dict.

For comparison purposes, Python introduced a ChainMap class (in 3.3 I believe) which provides a rough approximation of this approach - the definition there is you can have a chained list of dicts, with attribute access taking place on a left-to-right lookup basis, but only the first dict in the chain is allowed to be modified. The same example would work that way there:

1. del over['a'] deletes 'a' from overrides.
2. del over['b'] deletes 'b' from overrides .
3. del over['c'] raises exception, not allowed to delete from base.

But it's not obvious this is what we want either: now over['b'] returns 3 (the value from base) instead of 2 (the now-deleted value from overrides), so we didn't actually delete it from our view.

So that's an interpretation question: does 'del' mean delete from the view, or does it mean delete the overriden version but possibly let a value from the base show though?

This sounds nitpicky: code which creates an override isn't terribly likely to do any deleting, it's normally interested in just adding one or more values to create the overrride. But an OverrideEnvironment, created by the factory function, is just returned and then passed around like any other env, somebody being passed one of those doesn't know it's anything special, and it would likely be too expensive to put isinstance checks all over the place for something which isn't the "normal case" - most times an env is a real env. I think there's a general feeling that "callbacks" and other build-time things should not modify an env they're passed, but I don't believe that's ever explicitly stated nor is there any reasonable way to enforce that ("code reviews" withing the SCons codebase itself, I guess, but doesn't help for external code).

It's possible (the implementation is easy enough) to record keys explicitly requested for deletion, delete them only from the override, use the saved delete info to refuse to serve them from the underlying environment, which would solve the "if was deleted, don't let it look like it still exists" question in a different way than we do now. Or could take an approach more like ChainMap which doesn't delete from the underlying but still serves up from there on request. Or one could make __delitem__ entirely disallowed.

===

There's another instance of "leakage" through to the underlying env that's much harder to prevent - if the value for a given key is a mutable object, changing it through the override view changes the same object. Maybe that's a separate topic (and it's a general Python topic, not unique to SCons - calls for things like immutable dicts have been considered too niche to add to core, and the PEPs have been rejected, so that's going nowhere).

Just for grins, here's an extension to EnvironmentTests that shows this leakage:

    def test_Append(self) -> None:
        """Test the OverrideEnvironment Append() method for leakage"""
        env, env2, env3 = self.envs
        # initial state already proven by test___init__

        # add a new key to base env, with a mutable value
        env.Append(QQQ=deque(['q1', 'q2', 'q3']))
        self.assertEqual(env2['QQQ'], deque(['q1', 'q2', 'q3']))
        env2.Append(QQQ='q4')
        self.assertEqual(env2['QQQ'], deque(['q1', 'q2', 'q3', 'q4']))
        # did it leak?
        self.assertNotIn('q4', env['QQQ'])

leading to:

Traceback (most recent call last):
  File "/home/mats/github/scons/SCons/EnvironmentTests.py", line 3979, in test_Append
    self.assertNotIn('q4', env['QQQ'])
AssertionError: 'q4' unexpectedly found in deque(['q1', 'q2', 'q3', 'q4'])

[bdbaddog]

Modifying the "parent" env as you've described seems reasonable to me, for values not in the override.
For example.. modifying env['ENV']['PATH'] in a builder's generate method called via an override..
So I'd say yes, separate issue from delete, but also that it's (likely generally) ok.

For delete.. more complicated, but probably same answer.
The builder never needs to know that it's using an OverrideEnvironment().
So, I'd say our delete behavior is what I'd expect.

Separating the behavior from whether a builder should delete from the passed env.
Remember it could be modified in non-builder logic in the SConscripts.

If you did a delete on a dict and then it wasn't really deleted, that'd be surprising.. (not the good kind).
If you did a delete on a dict and it only deleted the override value, but kept the base env value, likewise bad surprise.

That said, I did some experiments a while back with locking an Environment() with the ideal of pre-subst'ing all values which could be. Didn't get around to completing that experiment, but I can see real value in locking an env, and/or being able to lock some values in the env from modification, and indeed there's already a mechanism for such for SOURCE, TARGET, etc..

[mwichmann]

For delete.. more complicated, but probably same answer. The builder never needs to know that it's using an OverrideEnvironment(). So, I'd say our delete behavior is what I'd expect.

From the point of view of the override, that's fine. That was the "interpretation question": should it look like it's gone if you called delete, or do you just uncover the previous layer like in ChainMap. I have absolutely no problem if the former is how we want it to be.

From the point of view of the base env, though: it doesn't feel right to me that a variable would vanish from that env because somebody did something to an override. That's why I suggested it's possible to make it look like it's gone when seen through the lens of the override without actually removing it from the base.

Separating the behavior from whether a builder should delete from the passed env. Remember it could be modified in non-builder logic in the SConscripts.

I think it's not so awkward if people do things during the linear flow through the sconscripts in that phase. You get what you get, and you can trace how things change and debug what breaks without horrible trouble. I'm less happy about routines that are called later doing things to an env, that seems a recipe for introducing breaks that nearly impossible to track down.

[bdbaddog]

I think when we look at an OverrideEnvironment() we need to just ignore the fact that it's an override and that the behavior should be the same as if we were operating on a regular Environment(), because unless your logic specifically checks for such, you'd not know.

It's rarely the case that someone explicitly creates an OverrideEnvironment() and then passes it along, in that case, I think it's safe to pass the burden on to the black magic practicing developer, rather than add all kinds of safety checks in the code impacting performance. In almost no circumstances is doing this the right thing to do, just env.Clone() and use that.

Have we seen such "in the wild", or are we just speculating on the mess of doing delete on an OverrideEnvironment?

Perhaps it's time to create a AntiPatterns wiki page (or in the UsersGuide?) .. as in.. you can do these things with SCons, but we advise against it because tracking down any issues with your build may be significantly more difficult..?

[mwichmann]

You've mentioned antipatterns before, and it's a good idea. Just don't relish sitting down and writing it from scratch... maybe if there's just a place to jot down ideas until there's enough to be worth polishing up into a page?

It's rarely the case that someone explicitly creates an OverrideEnvironment()

Well, for grins, Builder.py and Executor.py both do that! But yes, unlikely in user case, we don't document the Override factory function, presumably intentionally.

[bdbaddog]

someone in that context = not a core SCons developer for internal code..