flake8-type-checking icon indicating copy to clipboard operation
flake8-type-checking copied to clipboard

TC100 not needed for stub files

Open jonyscathe opened this issue 3 years ago • 15 comments

flake8-pyi has started reporting the following warning if from __future__ import annotations is in a stub file:

Y044 "from __future__ import annotations" has no effect in stub files.

This is because forward references are enabled by default.

It might be a good idea for TC100 to not be reported against stub files?

jonyscathe avatar Aug 03 '22 00:08 jonyscathe

Yes this sounds like a good idea if possible. I'll have to double check which state flake8 passes to the plugin; I fear it might not include a filename or extension, but hopefully I'm wrong! 🤞

In the meantime you should be able to get around this by adding this to your flake8 config:

per-file-ignores =
    *pyi:<error code>

sondrelg avatar Aug 03 '22 06:08 sondrelg

Yep - had already added that to my flake8 config. But thought it worth raising an issue anyway as I'm sure I'm not the only one using flake8-type-checking and flake8-pyi

jonyscathe avatar Aug 03 '22 06:08 jonyscathe

It is possible to do this. We can add the filename attribute to our plugin class to receive filenames from flake8. I don't have time to implement the feature currently, but will try to get to this at some point.

In the meantime PRs are welcome 🙂

sondrelg avatar Aug 04 '22 15:08 sondrelg

Actually, perhaps this is an easy fix. Do you think this would be as easy as disabling TC100 for .pyi files @jonyscathe?

sondrelg avatar Aug 04 '22 18:08 sondrelg

Would you mind providing a source documenting that forward references are enabled by default? Just so I can add it to the readme.

sondrelg avatar Aug 04 '22 21:08 sondrelg

Hmmmm. On the flake8-pyi page (https://github.com/PyCQA/flake8-pyi) Under Y044's description it states: from __future__ import annotations has no effect in stub files, as forward references in stubs are enabled by default.

But I don't think that is actually correct. I just think that from __future__ import annotations isn't generally needed within stubs as stubs aren't used at runtime and type checkers don't require forward references. So I think that in flake8-pyi they came to the right conclusion about from __future__ import annotations being an unnecessary import within a stub, but for the wrong reason :\

The mypy documentation does state that String literal types are never needed in # type: comments and stub files. in their docs. That isn't quite the same as saying that the future annotations import isn't required, but amounts to the same given that future annotations just does auto string literal-ification of annotations.

jonyscathe avatar Aug 04 '22 23:08 jonyscathe

I've now opened up a documentation query on flake8-pyi as I think their documentation is just incorrect on why future annotatiosn aren't needed in stubs.

jonyscathe avatar Aug 05 '22 00:08 jonyscathe

Ok, flake8-pyi guys referenced https://typing.readthedocs.io/en/latest/source/stubs.html#syntax which does state:

Stubs are treated as if from future import annotations is enabled. In particular, built-in generics, pipe union syntax (X | Y), and forward references can be used.

jonyscathe avatar Aug 05 '22 00:08 jonyscathe

So it sounds to me like there's nothing wrong with having a futures import in a stubs file, except it would do nothing in practice. I'm happy to turn off TC100 for stubs with this in mind 👍

Just one more thing; does it make sense to use any of the error codes in a stub file? Guarding imports in a stub file seems redundant if the stubs aren't evaluated at runtime anyways. Perhaps it would be better if this plugin just didn't handle .pyi files at all?

sondrelg avatar Aug 05 '22 08:08 sondrelg

@jonyscathe what do you think? Would disabling the plugin for all pyi files make sense?

sondrelg avatar Aug 26 '22 19:08 sondrelg

Sorry for the slow responses! Yes, I think disabling the plugin for pyi files probably does make sense. As you say, stubs aren't evaluated at runtime so it does make these checks a bit pointless within those files. But up to you. For now I have just globally ignored TC100 on pyi files so it isn't causing any issues for me.

jonyscathe avatar Aug 31 '22 00:08 jonyscathe

I think the implementation is simpler this way, and I can't think of a reason not to, so think I'll go with that and add a note in the readme 👍 Thanks for the confirmation!

sondrelg avatar Aug 31 '22 06:08 sondrelg

After considering this for a bit, I think the original idea of muting TC100 is slightly better. It seems unlikely, but TC101 ("Annotation does not need to be a string literal") could add value to a stub file. More importantly, muting TC100 seems like a bugfix while turning off stubs files is probably a breaking change :shrug:

Would you mind checking if the stub-files branch works for you @jonyscathe? You can install with pip install git+https://github.com/snok/flake8-type-checking@stub-files :slightly_smiling_face:

sondrelg avatar Sep 01 '22 21:09 sondrelg

Ok. So first thing... With flake8==5.0.4 and flake8-type-checking==2.1.2 I don't have any issues with TC100 coming up in stub files.... Took me a while to realise that I was on flake8==4.0.1 when I made this issue,

Now: With flake8==4.0.1 and flake8-type-checking==2.1.2 I do again have the TC100 pop up. With flake8==4.0.1 and flake8-type-checking@stub-files the TC100 warning no longer comes up.

So firstly, the stub-file branch does work. Secondly, for some reason with flake8==5.0.4 this is no longer needed.

I can't see anything in the flake8 release notes that would suggest .pyi files are being handled any differently. So I have no idea why the TC100 has stopped coming up in my .pyi file now that I have moved to flake8==5.0.4

jonyscathe avatar Sep 02 '22 07:09 jonyscathe

Haha, as long as the branch works I'm happy 😄 Thanks!

sondrelg avatar Sep 02 '22 08:09 sondrelg