attribute-defined-outside-init while class has __setattr__()

[alonbl]

Bug description

Hello,

I am sorry if this was reported before, I search for a solution or hint but I could not find any, I do see that __getattr__() was addressed, I could not find __seetattr__() reference.

When developing a dynamic class that delegate attributes based on external metadata, the attributes are not defined in __init__() but redirected by __getattr__() and __setattr__().

See this example:

class Test:
    def __getattr__(self, attr):
        if attr == "a":
            print(f"get {attr}")
            return "value"
        raise AttributeError()

    def __setattr__(self, attr, value):
        if attr == "a":
            print(f"set {attr}={value}")

t = Test()
print(t.a)
t.a = "value"

In these cases when __setattr__() is available, there should be an option to declare the class in such a way so that the user of the class will not get lint errors of attribute-defined-outside-init as the class handles all its attributes dynamically.

Is there existing solution?

Thanks,

Configuration

No response

Command used

pylint example.py

Pylint output

************* Module example
example.py:14:0: W0201: Attribute 'a' defined outside __init__ (attribute-defined-outside-init)

Expected behavior

When a class has __setattr__() method, the attribute-defined-outside-init warning should be disabled.

Pylint version

pylint 3.2.5
astroid 3.2.2
Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0]

OS / Environment

DISTRIB_DESCRIPTION="Ubuntu 22.04.4 LTS"

Additional dependencies

No response

[DanielNoord]

I think the warning is in the spirit of a linter. What you're doing is technically possible but it is a linter's job to warn you about dangerous patterns that are not recommended. I think many will agree that this is a dangerous pattern that should be avoided or explicitly be enabled by disabling the linter in place.

[alonbl]

Hello @DanielNoord ,

Thank you for you addressing this, before you class this or finalize this as Question. and forget all about it.

You may call this dangerous or not recommended, I will tell you about the use case... I have a device which has a set of registers which I are get/set and exposed as class members by metadata definition, there is no point in holding these as members as the member are delegated to a device. These members are created based on metadata that is fed to the class, hence the __getattr__()/__setattr__() are useful and fit the use case.

The problem is not with the subjective view of the linter, the problem is that the author of the class should specify that "I've checked this code and it is OK", while as far as I understand in the current implementation, the USER of the class which is plain user which knows nothing of the implementation is affected by the internal implementation of the class he is using, so that he should be aware of the internal implementation of the class he uses.

A possible solution would be to annotate the class with some pylint: disable=xxx so that caller will not be alerted.

BTW: if you have a cleaner method to implement the use case, I would love to hear.

Thanks,

[DanielNoord]

I understand your use case but do have some questions. How is the user of the class supposed to know that a is "safe" to access?

Based on a simple from library import Device how can a user know that Device().a is safe to access?

[alonbl]

Hello @DanielNoord,

This exactly why I opened this bug, as it seems like pylint does have a special case for __getattr__() but not for __setattr__().

For example:

class X:  # pylint: disable=too-few-public-methods
    pass

class Y:  # pylint: disable=too-few-public-methods
    def __getattr__(self, attr):
        return 0

x = X()
y = Y()

print(x.a)
print(y.a)

Produces:

************* Module a
a.py:11:6: E1101: Instance of 'X' has no 'a' member (no-member)

I may ask you the same, how can I know that Device().a is safe to access? this is already supported... Y has no warning because it supports __getattr__(). What is not supported is Device().a = 5, this is why I think it is a bug as behavior is inconsistent (get is supported, set is not).

I thought of adding fake keys into __dict__ based on the metadata during construction, however it looks like pylint does not monitor the __dict__ attribute. Maybe there is a method for dynamic proxy to declare its attributes which I am not aware of?

class Test:
    def __init__(self):
        # self.a = 0
        self.__dict__["a"] = 0

    def __getattr__(self, attr):
        if attr == "a":
            print(f"get {attr}")
            return "value"
        raise AttributeError()

    def __setattr__(self, attr, value):
        if attr == "a":
            print(f"set {attr}={value}")

t = Test()
print(t.a)
t.a = "value"

Regards,

[DanielNoord]

Thanks for you persistence @alonbl. I misunderstood your initial question.

https://github.com/pylint-dev/pylint/blob/c6da47a14faa150dd129172fb7894b437bc9c7a3/pylint/checkers/classes/class_checker.py#L1148

The code doesn't immediately show that we're special casing __getattr__. I guess it is the unintentional result of some other check but I haven't checked. But I agree, we should make this behave consistently!

[jacobtylerwalls]

@DanielNoord I still don't see the issue here, the provided example switched to discussing no-member.

[DanielNoord]

I don't think so. The assignment to t.a on line 15 causes a attribute-defined-outside-init even though the access of a on line 14 doesn't. Thus, we "correctly" allow accessing arbitrary attributes if __getattr__ is defined, but we don't allow arbitrary setting of attributes when __setattr__ is defined.

[jacobtylerwalls]

I don't think so. The assignment to t.a on line 15 causes a attribute-defined-outside-init even though the access of a on line 14 doesn't.

Access is not assignment, so it's not the subject matter for attribute-defined-outside-init. Or are you suggesting to compare the two messages against a holistic principle?

I don't think it's fair to compare no-member and attribute-defined-outside-init to each other. One is an error; the other is a warning. It makes sense that the higher severity error would be more cautious about false positives, whereas the code style warning would be more considered with adhering to patterns.

You can still use dynamic __setattr__ inside __init__ if you want to and thus avoid the warning. If you don't want to do that, that means you need to disable the rule. A team that uses __setattr__ implementations to perform side effects like the provided example might still want to prohibit uses outside __init__ and would find a behavior change here unwelcome.

The code doesn't immediately show that we're special casing __getattr__. I guess it is the unintentional result of some other check but I haven't checked. But I agree, we should make this behave consistently!

We bail out of no-member immediately if there is a __getattr__ implementation:

https://github.com/pylint-dev/pylint/blob/a48cd4c6a872b6565bc58030b74585812f327f36/pylint/checkers/typecheck.py#L440

To me, that doesn't have any bearing on what other checks should do.

I'm a works-as-designed on this one. pylint is not smarter than you -- we need to encourage users to be more confident to turn off less-than-error-level messages they don't like.

[jacobtylerwalls]

8300 also asks for an exception from attribute-defined-outside-init in a similar edge case.

[DanielNoord]

I don't really understand what no-member has to do this with issue?

I understand the confusion of the reporter that attribute-defined-outside-init isn't raised for when __getattr__ is implemented but it is when __setattr__ is. Personally I would not want either of the exceptions and would raise in both cases, but having this difference does seem counter-intuitive. Couldn't we fix that without touching no-member?

[jacobtylerwalls]

I understand the confusion of the reporter that attribute-defined-outside-init isn't raised for when getattr is implemented but it is when setattr is.

But attribute-defined-outside-init has nothing to do with getting attributes, only setting them. Getting attributes in whatever way, with dot access or getattr is as irrelevant to this message as try/except or while loops. From the message definition: "Used when an instance attribute is defined outside the init method."

Personally I would not want either of the exceptions and would raise in both cases, but having this difference does seem counter-intuitive.

There isn't a difference. We always raise when an attribute is defined outside __init__.

I don't really understand what no-member has to do this with issue?

Neither do I. The only reason we started talking about __getattr__ is because we switched to talking about no-member.

[DanielNoord]

To me the implementation of __setattr__ seems to indicate that user considers all attributes to be defined already. This is also sort of how my mental model works: there is not really a definition outside of the __init__ since the combination of __setattr_ and __getattr__ makes it so that all attributes will return a value and are therefore defined.

That said, I do consider this an edge case and already pointed out in the thread that this is probably a pattern a linter should discourage anyway. I'm fine with closing as won't fix.

[jacobtylerwalls]

Closing as works as designed. Builtin __setattr__ also accepts arbitrary input, and this is the message that is supposed to discourage that kind of arbitrary input.

[jacobtylerwalls]

Thanks for raising the issue. It was good to have an opportunity to consider the situation in greater detail 👍🏻

[alonbl]

Hello,

Thank you for the discussion, I am unsure I fully understand why there is a difference between get and set, but let's assume we want to discourage both.

Let's call the use case "dynamic proxy classes", these classes are built out of metadata obtained locally or remotely to simplify the developer interface. The use case is valid in dynamic classes such as python, it is also valid in bytecode based such as Java.

The problem we discuss here is that [as far as I know], the user of these classes should disable the lint error in every usage of the proxy, it makes the usage of dynamic proxy classes uncomfortable to the user, even if he state, yes, I understand, this is a dynamic proxy, I am aware of this.

What I would suggest is to consider one of the two:

Add somekind of pylint hint on these classes so that we can declare at one place that a class is using the pattern.

class Proxy: # pylint: hint:dyanmic-proxy
    ...

Or, I am unsure that this is possible, pylint to read the __dict__ member for properties hint.

class Proxy:
    def __init__(self):
        self.__dict__["property"] = None

What do you think?

[DanielNoord]

Although in theory this makes sense I'm not sure if it fits in pylint. "dynamic proxy classes" is exactly the kind of pattern that Python allows but which (in my opinion) a Python linter should discourage and make harder. Speaking from my own perspective: I would use a dynamic proxy while testing out a prototype but before committing the code and passing CI I would turn them into standard classes.

[alonbl]

Hi,

I respect your opinion, however, it is not always possible to use concrete classes, the data of what is available may reside outside of the program scope, transmitted at runtime as metadata.

I appreciate that you are trying to make it harder, however, my suggestions above do not contradict this, either allow to disable the warning, as you allow to every other warning, the difference here is where you put the statement (the interface not the usage).

In the current implementation, most people will workaround by disabling these warning globally which is counter productive, as we do not detect real issues in concrete classes, or use functions such as xxx.set_property(name: str, value: Any) -> None xxx.get_property(name: str) -> Any which is counter productive as well, as you achieved nothing in preventing people of implementing dynamic proxies. I guess most people will just disable the warning globally.

If you wish to make this harder and do not allow any way to disable the warning, I do challenge you guys to disable the __getattr__() handling as legit, so people will be notified in all cases of dynamic proxies... And then wait for issues to be reported to see how much usable it is.

Thanks,

[DanielNoord]

I think a set_property method would make sense in your case. It means there is one place that knows about the dynamic nature of the class (and you only need to disable the lint in one place). By putting that knowledge in one place you don't leak the dynamic nature of the class to all other places, which I think is a better pattern.