Name conflict in SignalGroup

[getzze]

• psygnal version: 0.9.5
• Python version: 3.11

Description

I created an Evented Dataclass with private field names in conflict with the attributes of the SignalInstance class. This resulted in an AttributeError. The bug comes from a name conflicts inside SignalGroup.

I tried an easy solution to avoid name conflict, by allowing to set a suffix for the signal names.

What I Did

from typing import ClassVar
from attrs import define, field
from psygnal import SignalGroupDescriptor, Signal, SignalGroup

# Evented Dataclass error
@define
class Test:
    events: ClassVar[SignalGroupDescriptor] = SignalGroupDescriptor()

    _name: str = field(default="name")
#    _args_queue: int = field(default=1)
#    _instance: int = field(default=1)
    age: int = field(default=1)

m = Test()
m.events  # -> AttributeError: 'str' object has no attribute 'connect'

# SignalGroup error
class Events(SignalGroup):
    sig1 = Signal(int)
    _name = Signal(int)

events = Events()  # ->  AttributeError: 'str' object has no attribute 'connect'

# With this PR
@define
class Test:
    events: ClassVar[SignalGroupDescriptor] = SignalGroupDescriptor(signal_suffix="_changed")

    _name: str = field(default="name")
    _args_queue: int = field(default=1)
    _instance: int = field(default=1)
    age: int = field(default=1)

m = Test()
@m.events.connect
def on_any_change(info: EmissionInfo):
    field_name = info.signal.name.removesuffix("_changed")
    print(f"signal {info.signal.name!r} emitted because the field {field_name!r} changed: {info.args}")

m._name = "new" 
# >> signal '_name_changed' emitted because the field '_name' changed: ('new', 'name')'_name_changed' changed to ('new', 'name')

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (59472ca) 99.89% compared to head (7f118e8) 99.89%. Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #260 +/- ## ======================================= Coverage 99.89% 99.89% ======================================= Files 22 22 Lines 1908 1912 +4 ======================================= + Hits 1906 1910 +4 Misses 2 2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[codspeed-hq[bot]]

CodSpeed Performance Report

Merging #260 will degrade performances by 35.67%

_{Comparing getzze:name-conflicts (7f118e8) with main (d3d558d)}

Summary

⚡ 1 improvements ❌ 1 regressions ✅ 64 untouched benchmarks

:warning: Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	getzze:name-conflicts	Change
❌	test_evented_setattr	38.4 µs	59.7 µs	-35.67%
⚡	test_emit_time[partial_method-2]	135.8 µs	121.9 µs	+11.39%

[tlambert03]

yeah, this is definitely a big problem, with no currently open issue. So thanks a lot for opening this. I will have a look soon!

[getzze]

I tried modifying SignalGroup to store the SignalInstance's in a signal_instances dict instead but I didn't manage to make it work, also self.events.field was not working anymore.

So I made this minimal change instead.

[getzze]

Ok, now we make sure that the signal variable in _setattr_and_emit_ is really a SignalInstance and not a str or whatever if we catch an attribute of SignalInstance whose name conflicts with the field name.

I also added another keyword argument to convert private fields to alias or skip them, so it solves #261.

[getzze]

I reverted the stuff about private fields as it is not related to the problem of this PR

[tlambert03]

once again, thanks so much for opening this and taking the time to consider solutions. I had some time to look at it.

So, just to state explicitly what seems to be the two-three possible solutions for name conflicts:

1. we give the user the option (and responsibility) of resolving name conflicts by adding a suffix field for them to control (this PR). In this case, it think that it will be important to emit warnings in cases where there are name conflicts that the user hasn't fixed on their own
2. we give priority to the signal (probably the expected behavior), such that signal names accessed on aSignalGroup instance always point to their corresponding SignalInstances, and shadow any matching attributes on the SignalGroup. (so, if you wanted to use _name or name, it would be just fine, but you would have to put in more effort to get at the shadowed attribute). (See #262)
3. Some combination of 1 and 2. Where priority is given to the signal name by default, but the user can also provide a mapping of field name to event name.

I'm curious to hear your thoughts on PR #262, which has a slight variant on the fix in this line of this PR

[getzze]

Nice! I think 3 would be best, it gives more flexibility. But the most important is 2, otherwise everything is totally messed up. I think the way Signal are attached to SignalGroup should be changed:

• the signals should be stored in a protected attribute, like __signal_instances, in SignalGroup instances.
• normal attributes inherited from SignalInstance stay in __dict__
• SignalGroup has 2 methods:
• get_signal to return the SignalInstance associated with the name.
• get_safe_attr to return the normal attribute from SignalInstance.
• we overwrite __getattr__ to look if the name was found in __signal_instances first, and return it. Otherwise return the normal attribute. Like this we can get the signals and the normal attributes safely, and self.events.x will return the signal also.

I don't know what you think about it.

On 2 February 2024 16:30:26 GMT+00:00, Talley Lambert @.***> wrote:

once again, thanks so much for opening this and taking the time to consider solutions. I had some time to look at it.

So, just to state explicitly what seems to be the two-three possible solutions for name conflicts:

1. we give the user the option (and responsibility) of resolving name conflicts by adding a suffix field for them to control (this PR). In this case, it think that it will be important to emit warnings in cases where there are name conflicts that the user hasn't fixed on their own
2. we give priority to the signal (probably the expected behavior), such that signal names accessed on aSignalGroup instance always point to their corresponding SignalInstances, and shadow any matching attributes on the SignalGroup. (so, if you wanted to use _name or name, it would be just fine, but you would have to put in more effort to get at the shadowed attribute). (See #262)
3. Some combination of 1 and 2. Where priority is given to the signal name by default, but the user can also provide a mapping of field name to event name.

I'm curious to hear your thoughts on PR #262, which has a slight variant on the fix in this line of this PR

-- Reply to this email directly or view it on GitHub: https://github.com/pyapp-kit/psygnal/pull/260#issuecomment-1924217669 You are receiving this because you authored the thread.

Message ID: @.***>

[getzze]

Maybe it was not very clear, but I also meant that in __init_subclasses the Signals should be deleted from the __dict__ of the class.

On 2 February 2024 19:36:42 GMT+01:00, getzze @.***> wrote:

Nice! I think 3 would be best, it gives more flexibility. But the most important is 2, otherwise everything is totally messed up. I think the way Signal are attached to SignalGroup should be changed:

• the signals should be stored in a protected attribute, like __signal_instances, in SignalGroup instances.
• normal attributes inherited from SignalInstance stay in __dict__
• SignalGroup has 2 methods:
• get_signal to return the SignalInstance associated with the name.
• get_safe_attr to return the normal attribute from SignalInstance.
• we overwrite __getattr__ to look if the name was found in __signal_instances first, and return it. Otherwise return the normal attribute. Like this we can get the signals and the normal attributes safely, and self.events.x will return the signal also.

I don't know what you think about it.

On 2 February 2024 16:30:26 GMT+00:00, Talley Lambert @.***> wrote:

once again, thanks so much for opening this and taking the time to consider solutions. I had some time to look at it.

So, just to state explicitly what seems to be the two-three possible solutions for name conflicts:

1. we give the user the option (and responsibility) of resolving name conflicts by adding a suffix field for them to control (this PR). In this case, it think that it will be important to emit warnings in cases where there are name conflicts that the user hasn't fixed on their own
2. we give priority to the signal (probably the expected behavior), such that signal names accessed on aSignalGroup instance always point to their corresponding SignalInstances, and shadow any matching attributes on the SignalGroup. (so, if you wanted to use _name or name, it would be just fine, but you would have to put in more effort to get at the shadowed attribute). (See #262)
3. Some combination of 1 and 2. Where priority is given to the signal name by default, but the user can also provide a mapping of field name to event name.

I'm curious to hear your thoughts on PR #262, which has a slight variant on the fix in this line of this PR

-- Reply to this email directly or view it on GitHub: https://github.com/pyapp-kit/psygnal/pull/260#issuecomment-1924217669 You are receiving this because you authored the thread.

Message ID: @.***>