Re: [PATCH] kunit: tool: fix type annotation for IO

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 16, 2023 at 2:03 PM Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
>
> On Thu, 2023-03-16 at 10:58 -0700, Daniel Latypov wrote:
> > Yikes.
> > I either forgot or never knew about --strict, thanks for pointing it out.
> >
> > Looking into it, I don't think it's worth using for kunit.py, details below.
>
> Oh, no argument, whatever works for you. I don't really even remember
> why I/we settled on mypy vs. pytypes, I don't remember really doing any
> comparison.
>
> 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.

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.

>
> > Hmm, the untyped stuff is not too important since we also use pytype,
> > which is a lot smarter in that it infers types where not specified.
>
> Hmm, mypy normally does a bunch of inference too, but yeah might be less
> smart here, or maybe --strict just insists on more precision.

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.

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.

>
> > The other two though warrant some attention
> > * attr-defined: we don't care about this. We have a directive to
> > squash this "error" for pytype. Ugh, do we need another one for
> > pytype?
>
> You mean mypy? I guess yes, you'd need "# type: ignore". But funnily, I

Ah yes, I meant mypy.

> 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.

>
>
> > * assignment: not actually a real error, it points to our type
> > annotations being a bit sloppy
>
> Not sure what you meant by this, but maybe I just don't see it with my
> version of mypy.
>
> >   tools/testing/kunit/kunit.py:459: error: Incompatible types in
> > assignment (expression has type "List[str]", variable has type
> > "TextIO")  [assignment]
> >
> > So it doesn't like that we assign a list[str] or a IO[str] to `kunit_output`.
> > But we're passing it in as Iterable[str], which both work as.
> >
> > Even explicitly annotating it as an Iterable[str] doesn't make it happy [1].
> > I'll ignore this, then.
>
> 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.

Thanks!

Note: we'd been avoiding the `var: T = 42` syntax to try and be more
compatible with old versions of python.
But since we formally added a check that it's running python>3.7, I
guess we can switch to that syntax now too.

>
> >         else:
> >                 with open(cli_args.file, 'r', errors='backslashreplace') as f:
> > -                       kunit_output = f.read().splitlines()
> > +                       kunit_output = f.read().splitlines()  #type[Iterable[str]]
> >
>
> which I think is because it assigned the type as Iterable[str] after the
> first one, and then was happy since splitlines() is Iterable[str] after
> all. So maybe pytypes is smarter about finding the best common type.
>
>
>
> Anyway ... I don't really care so much. I just noticed this, and had it
> been any more complex I'd probably have gone and just ignored kunit from
> my project.
>
> Oh, another unrelated thing: it kind of bothered me that even with
> kunit_parser.py you automatically get some output on stdout, it felt
> like kunit_parser.py was or should be more of a library, but in the end
> I don't really care so much since I needed to write a JSON file for our
> internal system and could just suppress the stdout entirely :)

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

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.

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.

Daniel




[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux