Variables instance bug

[bdbaddog]

This issue was originally created at: 2009-07-04 00:35:03. This issue was reported by: simpleton. simpleton said at 2009-07-04 00:35:03

I was browsing the Scons souce, and found this in Variables/__init__.py:

        # create the singleton instance
        if is_global:
            self=Variables.instance
 
            if not Variables.instance:
                Variables.instance=self

It seems to me that the line 'self=Variables.instance' is a bug.

gregnoel said at 2009-12-09 11:26:28

Bug party triage. Remove all uses of self.instance.

[mwichmann]

The usual SCons-y mess here. The internal signature of the Variables function is:

def Variables(files=None, args=ARGUMENTS):

The documented signature is:

Variables([files, [args]])

The Variables class initializer, meanwhile, is defined this way:

def __init__(self, files=None, args=None, is_global=True):

It I read that right, it means you could only instantiate a Variables if you directly called to it, passing is_global=False, you can't do that through the published API (the Variables function).

So do I read the @gregnoel comment as: get rid of the singleton-forcing stuff, and potentially make it possible for there to be more than one Variables object? If not, then the docs should possibly receive an update to show it's a singleton, the way is mentioned for DefaultEnvironment.

And, yes, the way this is written is broken, it doesn't create a singleton. I think the author was expecting to be able to morph self this way but it doesn't do what's expected, so we're not implementing a singleton now.

[bdbaddog]

It looks like Variables.instance is a class variable and thus is a singleton. Am I reading that wrong?

[mwichmann]

It turns out... yes - my first glance was also to think "oh, it's a singleton". By the time you're in __init__, a new instance has already been created. You then set self in __init__ to the one saved, which presumably intends to morph the new instance into the old one, but it's actually just assigning to the local self and nothing is then done with that. So it's similar to a common pattern, but not doing it right. Unless I'm reading it completely wrong... I can easily make it into one, but I'm not sure there's any benefit to it.

[mwichmann]

Swung back and checked - it's definitely producing different instances each time you instantiate Variables. It's dead simple to fix - it's a small stanza in __new__ instead of in __init__, but it doesn't really seem to matter, since we're already not getting a singleton and stuff still seems to be working. This sort of jig would do it:

    def __new__(cls):
        if not hasattr(cls, '_instance'):
            cls._instance = super().__new__(cls)
        return cls._instance

The bugparty note seemed to indicate a decision to just forget it: "Remove all uses of self.instance."

[mwichmann]

Forgot to link this to a PR since what started as a single PR (which did have a link in the commntary) was split into a series - the broken singelton-ish behavior was removed in PR #4533.