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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

[codspeed-hq]

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

⚠️ 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 fix: avoid name conflicts (option 2) use __get__ directly to create signal instances #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 a`SignalGroup` 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](https://github.com/pyapp-kit/psygnal/pull/260/files#diff-8218e71615e3cb1b30f78bd2143ca77661925d39178dd9ccad62dc0f62206c98R125-R127) -- Reply to this email directly or view it on GitHub: #260 (comment) 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 a`SignalGroup` 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](https://github.com/pyapp-kit/psygnal/pull/260/files#diff-8218e71615e3cb1b30f78bd2143ca77661925d39178dd9ccad62dc0f62206c98R125-R127) > > > >-- >Reply to this email directly or view it on GitHub: >#260 (comment) >You are receiving this because you authored the thread. > >Message ID: ***@***.***>