kunit: tool: fix type annotation for IO

From: Johannes Berg <johannes.berg@intel.com>

This should be IO[str], since we use it for printing strings.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 tools/testing/kunit/kunit_printer.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Thu, 2023-03-16 at 10:35 -0700, Daniel Latypov wrote:
> On Thu, Mar 16, 2023 at 12:42 AM Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > 
> > On Wed, 2023-03-15 at 23:02 -0700, Daniel Latypov wrote:
> > > This is a good catch, thanks.
> > > But we also have a few more bare generic types that warrant attention.
> > 
> > Oh, that might well be true. I was using kunit_parser in a script, and
> > that imports kunit_printer, and then tried to check *my* script for type
> > annotations with mypy. This led it to go check through the dependencies
> > too, and since it was just one small thing there I decided to just fix
> > it rather than figure out how to tell mypy that I don't care about those
> > dependencies :-)
> 
> There's --exclude='<regex>', if you ever end up needing to ignore other files.
> But yeah, we should try and make sure that mpyy is happy w/ kunit.py code.

Yes, but that has other complexities I think? Anyway, I didn't even
really read much into it since it was so easy.

> > Now for everything else? I didn't even look.
> 
> Oh, does mypy complain about this now?
> That'd be nice.
> 
> Hmm, I don't see it even after upgrading my local version.
> $ pip install --upgrade mypy pytype
> $ ../tools/testing/kunit/run_checks.py
> <no errors>
> # Checking if it doesn't report error but logs a warning:
> $ mypy ./tools/testing/kunit/*.py
> Success: no issues found in 9 source files

Oh, I use --strict, and

$ mypy --version
mypy 0.941

right now. Probably old.

And

$ mypy --strict tools/testing/kunit/*.py

has a _lot_ of complaints.

> How I found the rest is Google has a wrapper around pylint, which has
> a number of custom checks. "g-bare-generic" is one of them, which
> complains about these.
> 
> I don't think there's a publicly accessible version of those checks,
> even though the base pylintrc file is...

:)

> https://lore.kernel.org/linux-kselftest/20230316172900.755430-1-dlatypov@google.com
> 

Great, thanks!

johannes

On Thu, 2023-03-16 at 14:30 -0700, Daniel Latypov wrote:
> 
> 100% agree, just the concern is about trying to keep two different
> checkers happy when they might have conflicting demands.
> Pytype has been able to catch some relatively subtle errors, so I've
> prioritized it.

Sure. Maybe I'll just switch to pytype too ;-)


> But since commit b7e0b983ff13 ("kunit: tool: fix pre-existing python
> type annotation errors") onwards, I've been trying to fix up all the
> issues we've had with either.

OK.

> One footgun I ran into was that mypy seemed like it wasn't even
> touching functions if they didn't have an argument or the return type
> annotated.
> See 09641f7c7d8f ("kunit: tool: surface and address more typing issues")
> Maybe --strict is better.

It might not be, I have no idea.

> Pytype takes a lot longer and caught issues that normal `mypy` didn't.
> I haven't compared them w/ strict --strict. But given pytype is still
> a lot slower, I assume (hope) it's doing more work.

Hehe :-)

> > think both tools will insist that you place the comment on the same line
> > as the error, and that doesn't really work since you can only have one
> > comment on that line :-) However, it looks like pytype would also accept
> > "# type: ignore" according to the docs.
> 
> That's true, but `type: ignore` disables all type checking on the line.
> 
> In this case, I think it's moot given the whole line is just
>   sys.stdin.reconfigure(errors='backslashreplace')
> so I'd basically turned off all type-checking already.

Right, pytype looks a bit more controlled there.

> > I think you got that annotation wrong, at least as far as mypy is
> > concerned?
> > 
> > >                 sys.stdin.reconfigure(errors='backslashreplace')  #pytype: disable=attribute-error
> > > -               kunit_output = sys.stdin
> > > +               kunit_output = sys.stdin  # type[Iterable[str]]
> > 
> > Doing
> > 
> >  kunit_output: Iterable[str] = sys.stdin
> > 
> > actually fixes it for me, and you don't even need the second one:
> 
> D'oh, I forgot the ": "  Ugh, magic comments :)
> Doing "# type: Iterable[str]` on just the first one works for me.

Right, not sure I like the magic comments better than the inline type in
the code, but YMMV.

> Note: we'd been avoiding the `var: T = 42` syntax to try and be more
> compatible with old versions of python.

Well, OK, I gave up on non-controlled versions of python long ago. In my
case it's all running in a nix-shell environment so I control it very
precisely :-)

> Ack, yeah, the fact it dumps anything directly to stdout is annoying.
> 
> The argument is that
> * it wants to print in real time
> * it needs access to the tty to know if it should include colors or not

Right.

> This patch might be of interest to you:
> https://kunit-review.git.corp.google.com/c/linux/+/5730/1
> With that, you could pass in a kunit_printer.BufferPrinter to
> parse_tests() and keep stdout pristine.

Heh. Sounds like a useful patch but I can't see that link given the lack
of corp.google.com login :P

> I could split out the concurrency support from that patch and try to
> get submitted.
> There just wasn't motivation to do so since there was no use case for
> suppressing output yet.
> Given you're using it, that might be sufficient.

I honestly don't care much now. Basically decided that redirecting
stdout to /dev/null was sufficient, since anyway the real output I need
happens to a file. I might've designed it to print the JSON on stdout
instead if it wasn't getting all that output from kunit, but it really
doesn't matter now.

Anyway, thanks for the discussion! I've been playing with kunit only for
like 48 hours now, so we'll see where I can go from here :-)

johannes

On Thu, Mar 16, 2023 at 2:30 PM Daniel Latypov <dlatypov@google.com> wrote:
> > I do find the whole type-checking, if occasionally a bit painful, to be
> > of benefit though, at least for larger code bases (and by larger I think
> > I probably would say "more than a single file that you can keep in your
> > head entirely" :-)
>
> 100% agree, just the concern is about trying to keep two different
> checkers happy when they might have conflicting demands.
> Pytype has been able to catch some relatively subtle errors, so I've
> prioritized it.

Actually, I've gotten both pytype and `mypy --strict` happy now:
https://lore.kernel.org/linux-kselftest/20230316220638.983743-3-dlatypov@google.com/

That patch also updates kunit/run_checks.py so hopefully we can keep
`mypy --strict` happy going forward too.

Here's to safer python,
Daniel