"Non-overlapping container check" type error in BulkPushRuleEvaluator

[anoadragon453]

Running mypy with strict equality checking (via mypy --strict-equality) we get the following error:

$ mypy --strict-equality
synapse/push/bulk_push_rule_evaluator.py:380: error: Non-overlapping container check (element type: "str", container item type: "Dict[Any, Any]")  [comparison-overlap]

It points to this piece of code:

synapse/synapse/push/bulk_push_rule_evaluator.py

Lines 379 to 382 in 2d0ba3f

	actions = evaluator.run(rules, uid, display_name)
	if "notify" in actions:
	# Push rules say we should notify the user of this event
	actions_by_user[uid] = actions

The complaint is that actions is defined as a Collection[dict]:

synapse/stubs/synapse/synapse_rust/push.pyi

Lines 45 to 50 in 2d0ba3f

	def run(
	self,
	push_rules: FilteredPushRules,
	user_id: Optional[str],
	display_name: Optional[str],
	) -> Collection[dict]: ...

thus if "notify" in actions: makes no sense, hence mypy's complaint.

---

The code that actually produces the data structure is in Rust, and returns a vector of Actions (which is an Enum):

synapse/rust/src/push/evaluator.rs

Lines 90 to 103 in 2d0ba3f

	/// Run the evaluator with the given push rules, for the given user ID and
	/// display name of the user.
	///
	/// Passing in None will skip evaluating rules matching user ID and display
	/// name.
	///
	/// Returns the set of actions, if any, that match (filtering out any
	/// `dont_notify` actions).
	pub fn run(
	&self,
	push_rules: &FilteredPushRules,
	user_id: Option<&str>,
	display_name: Option<&str>,
	) -> Vec<Action> {

synapse/rust/src/push/mod.rs

Lines 163 to 172 in 2d0ba3f

	#[derive(Debug, Clone, PartialEq, Eq)]
	pub enum Action {
	DontNotify,
	Notify,
	Coalesce,
	SetTweak(SetTweak),
	// An unrecognized custom action.
	Unknown(Value),
	}

Following the definition of Action, we see that this should result in a vector of str when sent to python:

synapse/rust/src/push/mod.rs

Lines 208 to 221 in 2d0ba3f

	impl Serialize for Action {
	fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
	where
	S: serde::Serializer,
	{
	match self {
	Action::DontNotify => serializer.serialize_str("dont_notify"),
	Action::Notify => serializer.serialize_str("notify"),
	Action::Coalesce => serializer.serialize_str("coalesce"),
	Action::SetTweak(tweak) => tweak.serialize(serializer),
	Action::Unknown(value) => value.serialize(serializer),
	}
	}
	}

Assuming my Rust->Python foo is sound (please double-check!), my conclusion here is that the type stub is incorrect, and that we should instead have a Collection[str] when we reach Python.

I don't believe this has any user-visible impact. Fixing this would just reduce the number of mypy errors (and potentially prevent future bugs!).

[DMRobertson]

please double-check!

my reading of

             Action::SetTweak(tweak) => tweak.serialize(serializer), 
             Action::Unknown(value) => value.serialize(serializer), 

is that tweak will get serialised as a JSON object

synapse/rust/src/push/mod.rs

Lines 183 to 195 in 2d0ba3f

	/// The body of a `SetTweak` push action.
	#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]
	pub struct SetTweak {
	set_tweak: Cow<'static, str>,
	#[serde(skip_serializing_if = "Option::is_none")]
	value: Option<TweakValue>,
	// This picks up any other fields that may have been added by clients.
	// These get added when we convert the `Action` to a python object.
	#[serde(flatten)]
	other_keys: Value,
	}

and that Unknown(value) is a placeholder for an arbitrary JSON value.

[anoadragon453]

@DMRobertson Oh I see. Perhaps what we're looking for is a Union[dict, str] then?

[DMRobertson]

@DMRobertson Oh I see. Perhaps what we're looking for is a Union[dict, str] then?

That seems consistent with

synapse/synapse/push/bulk_push_rule_evaluator.py

Line 306 in 2d0ba3f

actions_by_user: Dict[str, Collection[Union[Mapping, str]]] = {}

, which is the only place we actually use actions directly AFAICS