Fix possible variable shadow in create_new_client_event

[H-Shay]

This PR renames a variable in create_new_client_event which potentially clashes with a same-named parameter of the function. It seems unlikely that they would clash but I figured it should be fixed just in case as the fix is easy.

[clokep]

This could happen if for_batch=True, state_event_ids is not-None.

So I guess the question is -- is it valid input if state_event_ids and state_map to both be not-None?

I think this might be a question for @MadLittleMods

[MadLittleMods]

I really wish we could add a lint for variable shadowing (parameter re-assigning) problem. Previously discussed at #10439 (comment)

The mypy issue still exists: python/mypy#11076

[MadLittleMods]

It looks like the state_map function parameter was added more recently in #13487 which caused the collision. Whereas the state_map variable was in place since #12083 and was only used as a temporary variable within itself at the time so this variable shadowing case seems legit.

(just pulled out of #12083)

synapse/synapse/handlers/message.py

Lines 972 to 976 in cd93056

	state_map = {(e.type, e.state_key): e.event_id for e in state_events}
	# Actually strip down and only use the necessary auth events
	auth_event_ids = self._event_auth_handler.compute_auth_events(
	event=temp_event,
	current_state_ids=state_map,

For reference and confusingly, all of the for_batch and state_map parameter stuff isn't for MSC2716 /batch_send (it's all new from the room creation batch event creation). It really feels like #13487 added some duplication of existing logic that could be covered by state_event_ids/auth_event_ids but I haven't delved into all of that code.

---

This could happen if for_batch=True, state_event_ids is not-None.

So I guess the question is -- is it valid input if state_event_ids and state_map to both be not-None?

I think this might be a question for @MadLittleMods

-- @clokep, #14575 (review)

As mentioned, since none of the stuff I've worked on for MSC2716 uses these parameters, I don't think there is a case where they interact unless @H-Shay has refactored some of that code and is probably the one to better answer.

[clokep]

Ah, my misunderstanding! Sorry about that. I assumed it all had to do with MSC2716 due to the "batch" comment in it. 😢

[MadLittleMods]

It really feels like #13487 added some duplication of existing logic that could be covered by state_event_ids/auth_event_ids but I haven't delved into all of that code.

@H-Shay Can you create a follow-up issue to track this?

And a separate one for "If we could somehow differentiate our internal batched event creating from MSC2716 that would be fabulous..." (#14575 (review))

[H-Shay]

Can you create a follow-up issue to track this?

Sure thing, I have created #14689 for the naming issue and #14688 for the possible logic duplication in create_new_client_event.

[clokep]

If we could somehow differentiate our internal batched event creating from MSC2716 that would be fabulous...

[H-Shay]

Yes this shadow was my fault, and while I didn't think there would be any overlap between the code paths, I thought it cleaner/clearer to future readers if I eliminated the possibility of confusion.