[red-knot] Remove Type::None

[sharkdp]

Summary

Removes Type::None in favor of KnownClass::NoneType.to_instance(…).

closes #13670

Test Plan

Existing tests pass.

[codspeed-hq]

CodSpeed Performance Report

Merging #14024 will degrade performances by 4.32%

_{Comparing david/remove-type-none (c7d4bdb) with main (e302c2d)}

Summary

❌ 1 (👁 1) regressions
✅ 31 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	david/remove-type-none	Change
👁	red_knot_check_file[incremental]	4.4 ms	4.6 ms	-4.32%

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[AlexWaygood]

We should be able to remove this branch too once we understand that NoneType is a subclass of object (my MRO PR is coming soon!!)

[sharkdp]

Note that this is about NoneType <: NoneType. The X <: object case is handled further above.

I'm actually surprised that NoneType <: NoneType doesn't work without this. We have a other == self check in is_subclass_of on the ClassTypes. Is that not working correctly? Is it actually checking for equal classes or for some other identity. Like just comparing salsa IDs?

my MRO PR is coming soon!!

That will definitely help us remove some cases!

[carljm]

self == other is indeed just checking salsa IDs. But Salsa IDs are assigned by Salsa interning, which interns based on struct equality, so that should be the same thing as an Eq check.

It would be good to understand what's happening here. Let me know if you'd like another pair of eyes on it. Maybe it's related to the fact that there are multiple NoneType in typeshed? (The one in _typeshed and the one in types for more recent Python versions.)

Also, I think this branch should go into is_equivalent_to instead of is_subtype_of.

[carljm]

Looks great!

I wonder how much of the perf regression is due to the fact that currently I think every None ends up being a union of the None defined in _typeshed and the one defined intypes? This could mean we spend a lot more time in recursive union handling for things like is_subtype_of? If so, this should be largely mitigated by sys.version_info checking.

[carljm]

self == other is indeed just checking salsa IDs. But Salsa IDs are assigned by Salsa interning, which interns based on struct equality, so that should be the same thing as an Eq check.

It would be good to understand what's happening here. Let me know if you'd like another pair of eyes on it. Maybe it's related to the fact that there are multiple NoneType in typeshed? (The one in _typeshed and the one in types for more recent Python versions.)

Also, I think this branch should go into is_equivalent_to instead of is_subtype_of.

[MichaReiser]

I took a look at the perf regression

What I spotted is that this branch now spends 5.6% of total time inside infer_function_definition_statement where we only spent 1.6% before.

Expanding the tree shows that the difference comes from infer_decorators that isn't present in the profile for main. I suspect that it comes from

@lru_cache(maxsize=None)
def cached_tz(hour_str: str, minute_str: str, sign_str: str) -> timezone:
    sign = 1 if sign_str == "+" else -1
    return timezone(
        timedelta(
            hours=sign * int(hour_str),
            minutes=sign * int(minute_str),
        )
    )

Inside infer_decorators there's now a call to Type::none which starts semantic indexing and what not of `lru_cache.

I tried to debug if we now infer a more accurate type for lru_cache but both main and this PR infer Todo for

from functools import lru_cache
from typing import reveal_type

reveal_type(lru_cache(maxsize=None))

The main thing that's puzzling me.... Why is the lru_cache type not cached? Shouldn't this bail out early instead of calling into semantic_index?

Ohh... never mind. I looked at the warmup phase instead of the incremental path.

This is for the incremental part

The finding is the same... and, for some reason, KnownClass::none is re-executing semantic indexing of the functools module which it should not... Not sure why. We should look into this

incremental check_types this PR

incremental check_types main

[MichaReiser]

Looking at the salsa code. What this suggests is that we don't reuse the same salsa ids and, because of that have cache misses...

[MichaReiser]

I'm not sure what's happening and/or if this is a bug in the benchmark. I copied the tomllib and manually ran the red knot CLI in watch mode and the salsa events indicate that it uses the memoized values except for _re.py that changed.

[MichaReiser]

Okay, I might still have looked at the wrong profiles. I now created a PR to use a named closure for the incremental and warmup vs to avoid this in the future.

• incremental none
• incremental main

I haven't looked at it too closely because salsa's maybe changed after profiles are a bit of a pain but I strongly suspect that the difference mainly comes from that salsa now has to validate the ingredients for the module defining None? I suspect that addressing #13169 could help here as well.

[carljm]

the difference mainly comes from that salsa now has to validate the ingredients for the module defining None

Is this a speculation or is there evidence pointing to it? I couldn't really follow which comments here are based on looking at the wrong part of the profile :)

This could be, though. I was thinking we would already have a dependence on types module (because builtins imports it), but it's possible tomllib didn't actually resolve the type of any builtin depending on types module, so our lazy per-definition inference might have meant we previously didn't have to semantic-index types module.

If this is the cause of the regression, it's mostly just a weakness in our benchmark. Real-world codebases are likely to already rely on the types module in some way; adding a core typechecking dependency on it should not really be considered a real-world regression, if the regression just comes from "now we have to index that module."

I still think it's worth seeing whether Salsa-caching the resolution of the type of None makes a difference here. If the regression is entirely due to having all the new ingredients from the types module, then it won't.

[sharkdp]

Performance investigation

• The original version of this PR had a -4% performance regression on the "incremental" as well as the "cold" benchmark (results). The "cold" benchmark was probably just slightly below the warning/error threshold.
• Consequently, a rebase to main (after the merge of Give non-existent files a durability of at least Medium #14034 by @MichaReiser) did not help with improving performance. Both versions are still at -4% (results)
• I then tried the suggestion by @carljm and added explicit salsa caching for the lookup of NoneType in 503aa46. The CI run now passes, but the performance regression is still there with -3% for both versions (results). Not sure if the resolution of these benchmarks is really sub-1%? If so, this is a small improvement.
• I checked which modules were resolved when running red-knot on tomllib; On main, we do not load _typeshed; on this branch, we do (due to the explicit lookup of NoneType in typeshed). This alone could probably explain the increase in runtime.

This also explains why I didn't see a performance regression when comparing this branch with main on a larger codebase (black). Because we do load _typeshed even on main (for some reason) when running on black. Results:

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
./red_knot_main --current-directory /home/shark/black	113.7 ± 3.4	105.5	120.0	1.00 ± 0.05
./red_knot_remove_none --current-directory /home/shark/black	113.2 ± 4.3	107.3	121.1	1.00

After talking to @MichaReiser, I removed the explicit caching of the NoneType lookup again.

[AlexWaygood]

I've only skimmed, but this all LGTM, and I agree that a small perf "regression" here is fine given your analysis (and given the significant improvements to maintainability this gives us)