Fix --strict-equality for iteratively visited code

[tyralla]

Fixes#19328
Fixes#20294

The logic is very similar to what we did to report different revealed types that were discovered in multiple iteration steps in one line. I think this fix is the last one needed before I can implement #19256.

[tyralla]

The primer results are a little mysterious. I could simplify to:

foryin [1.0]:
ifyisnotNoneory!="None": # E: Non-overlapping equality check (left operand type: "float", right operand type: "Literal['None']")
...

This obvious error is only reported with my change, but I do not know why current master misses it and therefore have no idea what in this PR contributes to detecting it.

For whatever reason this happens, I will add a corresponding test case.

[tyralla]

Ah, I got it. It is because optuna enables --strict-equality but not unreachable. y is not None is always true, so that the y != "None" mismatch is not reported.

I could invest some time so that

foryin [1.0]:
ifyisnotNoneory!="None":
...

and

y=1.0ifyisnotNoneory!="None":
...

would be handled identically again (no --strict-equality warnings). However, this is a good example that failing to raise warnings in unreachable code can be confusing, and there is already a PR that tries to improve the situation (#18707). Hence, maybe no further action is required here.

[tyralla]

I revisited the problem and added two lines of code that reset the old behaviour for the discussed problem and make some sense to me. The problem was that "nearer" error watchers could not filter non-overlap reports anymore. Now, the collection mechanism of IterationErrorWatcher only applies if no nearer error watcher has _filter activated. I think this is the place where the now again respected error watcher is created:

mypy/mypy/checkexpr.py

Lines 5982 to 5999 in 5a78607

	defanalyze_cond_branch(
	self,
	map: dict[Expression, Type] |None,
	node: Expression,
	context: Type|None,
	allow_none_return: bool=False,
	suppress_unreachable_errors: bool=True,
	) ->Type:
	withself.chk.binder.frame_context(can_skip=True, fall_through=0):
	ifmapisNone:
	# We still need to type check node, in case we want to
	# process it for isinstance checks later. Since the branch was
	# determined to be unreachable, any errors should be suppressed.
	withself.msg.filter_errors(filter_errors=suppress_unreachable_errors):
	self.accept(node, type_context=context, allow_none_return=allow_none_return)
	returnUninhabitedType()
	self.chk.push_type_map(map)
	returnself.accept(node, type_context=context, allow_none_return=allow_none_return)

[tyralla]

@ilevkivskyi: I don't want to overstretch your willingness to help, but could you also take a look at this one? This PR aligns closely with what we began in #19324 and would enable me to implement reachability checking for functions with constrained type variables.

[sterliakov]

Could you move this logic into the watcher itself? This access to private followed by special-casing on watcher type is a bit painful to read, IMO this would be clearer as

forwatcherinself.errors.get_watchers():
ifwatcher.store_nonoverlapping_types(ctx, kind, left, right):
return

Where store_nonoverlapping_types (not the best name) is a no-op in ErrorWatcher, overridden with this block in IterationErrorWatcher

[sterliakov]

Ough, you really need both break and return, sorry.

[tyralla]

No need to excuse. I am obviously using things a little differently from how they were originally intended. Hence, I highly appreciate any thoughts on improving readability.

[sterliakov]

Maybe NamedTuple or at least TypeAlias? I'm personally lost in brackets here

[tyralla]

In my opinion, TypeAlias doesn't help much. But using NamedTuple (here and in the similar cases above) would definitely increase readability. I will adjust it if there are no performance concerns.

[tyralla]

Sorry for the slow response: I just introduced the NamedTuple NonOverlapErrorInfo.

It would be good to refactor the other members of IterationDependentErrors as well, but maybe it would distract other reviewers, so I leave this for a separate PR.

[sterliakov]

Sorry for a big delay, LG! Left a couple of nits, but none of them are deal-breakers for me

[sterliakov]

selected is a strict subset of nonoverlaps, right? How big is it usually, relative to nonoverlaps size? If it normally includes a relatively small portion of those, it might be faster to swap these checks (iterate over selected and do left, right = nonoverlaps[candidate] for each).

[tyralla]

selected is only a subset of each nonoverlaps item if the relevant lines are reachable in all iterations. So, I am not really sure about the potential benefits of such a switch. However, maybe performance isn't overly critical here, since the loop's body is only executed if there is, in fact, an overlap issue to report?

[ilevkivskyi]

LG, but I am a bit worried about performance. Did you make any measurements with misc/perf_compare.py?

[ilevkivskyi]

I think a regular class will be faster when compiled with mypyc.

[tyralla]

Okay. Much boilerplate code (we also need __eq__ and __hash__), but no problem, of course. Done.

[ilevkivskyi]

Please don't do this, the effect on readability is ~0, while it may potentially affect compiled performance.

[tyralla]

+0.5 % readability IMO!

Also changed.

[ilevkivskyi]

(oh, you also need to update tests to use new union syntax)

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

[tyralla]

(oh, you also need to update tests to use new union syntax)

Also done.

LG, but I am a bit worried about performance. Did you make any measurements with misc/perf_compare.py?

Sorry, I should have done this already. I now performed two runs:

master 7.990s (0.0%) | stdev 0.315s
fix_strict_quality_in_loops 7.876s (-1.4%) | stdev 0.283s
Total time taken by the whole benchmarking program (including any setup): 11 minutes, 4 seconds

master 7.894s (0.0%) | stdev 0.415s
fix_strict_quality_in_loops 7.931s (+0.5%) | stdev 0.459s
Total time taken by the whole benchmarking program (including any setup): 11 minutes, 5 seconds

[tyralla]

@ilevkivskyi and @sterliakov : Thanks for the reviews!