Fully type check the gnome module#9704
Conversation
|
This pull request introduces 3 alerts when merging d459623 into 824e091 - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## master #9704 +/- ##
==========================================
- Coverage 67.75% 64.38% -3.37%
==========================================
Files 400 200 -200
Lines 85560 42701 -42859
Branches 18865 9321 -9544
==========================================
- Hits 57967 27491 -30476
+ Misses 23053 13005 -10048
+ Partials 4540 2205 -2335
Continue to review full report at Codecov.
|
dcbaker
force-pushed
the
submit/fully-type-check-the-gnome-module
branch
from
d459623 to
cca60f6
Compare
|
This pull request introduces 3 alerts when merging cca60f6 into 1e5d7f2 - view on LGTM.com new alerts:
|
dcbaker
force-pushed
the
submit/fully-type-check-the-gnome-module
branch
from
cca60f6 to
533da2f
Compare
|
This pull request introduces 3 alerts when merging 533da2f into 484a389 - view on LGTM.com new alerts:
|
dcbaker
force-pushed
the
submit/fully-type-check-the-gnome-module
branch
from
533da2f to
2268289
Compare
|
This pull request introduces 1 alert when merging 2268289 into 251d6f0 - view on LGTM.com new alerts:
|
dcbaker
force-pushed
the
submit/fully-type-check-the-gnome-module
branch
2 times, most recently
from
3947e87 to
aed1a72
Compare
|
Let's merge this immediately after 0.61.1? |
RunTargets and AliasTargets may depend on RunTargets, so annotate them that way. The Gnome module relies on this internally.
…ncludes_and_update_depends There is the problem of the annotations themselves, then there is the problem with depends being mutated. The mutation side effect is a problem in itself, but there's also the problem that we really want to use Sequence, which isn't mutable.
dcbaker
force-pushed
the
submit/fully-type-check-the-gnome-module
branch
from
aed1a72 to
9f0035c
Compare
This is hard to fix, and it's really doing something bad anyway. But we know it's right, so just tell mypy to not worry about it.
gnome points out that CustomTargets can be linked with, so we should allow that.
This is better as it avoids building unnecessary lists, and two fixes the typing issue from concatenating lists of different types.
Which is pretty much necessary to make anything involving unions of lists work
These don't return `Target`, they return `BuildTarget | CustomTarget | CustomTargetIndex`
_get_gir_target_deps The typing issues with these are tightly intertwined, so it didn't really make sense to solve them independently
Which is one incorrect type annotation, and a couple of instances of concatenating lists of unlike types. List being invariant is super annoying.
Since they don't use the instance or class state, they should be static methods.
dcbaker
force-pushed
the
submit/fully-type-check-the-gnome-module
branch
from
9f0035c to
26e87bc
Compare
| builddir = os.path.join(state.environment.get_build_dir(), state.subdir) | ||
|
|
||
| depends: T.List[T.Union['FileOrString', build.GeneratedTypes, build.Executable, build.SharedLibrary, build.StaticLibrary]] = [] | ||
| depends: T.List[T.Union['FileOrString', 'build.GeneratedTypes', build.BuildTarget]] = [] |
There was a problem hiding this comment.
How did this ever work? Weird. build.GeneratedTypes is a typechecking-only variable but it never produced undefined variable errors when running live?
There was a problem hiding this comment.
It seems that the answer to this is: https://www.python.org/dev/peps/pep-0526/#runtime-effects-of-type-annotations
A variable inside a function doesn't get stored in __annotations__ of the function, unlike class or module variables, and in fact isn't even evaluated...
3 participants
Finally, with these changes (and the ones in #9700), the gnome module has become type safe, and with it all callers of CustomTarget, so I can finally get to the work I actually wanted to, fixing the layering violation between the build and interpreter modules!