Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add py.typed to elftools #507

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Add py.typed to elftools #507

merged 2 commits into from
Oct 20, 2023

Conversation

fzakaria
Copy link
Contributor

fixes #506

@sevaa
Copy link
Contributor

sevaa commented Sep 26, 2023

What's the deal with double quotes?

@fzakaria
Copy link
Contributor Author

ugh -- i hit save in my vscode and it just applied black formatting.
Sorry --

Let me revert and just apply the line.

Copy link
Owner

@eliben eliben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me what the point of this PR is. It needs additional justification.
It seems like every few months a new configuration file is added -- we already have two setup files and a pyproject file. What additional capability does py.typed provide?

# All packages and sub-packages must be listed here
packages=[
"elftools",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what all these changes have to do with your proposed addition, can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you looking at an older commit?
I had some formatting done which I have removed in the latest commit.

@fzakaria
Copy link
Contributor Author

@eliben the linked issue has a link to the PEP https://peps.python.org/pep-0561/
In order for type information to be picked up a "py.typed" file must be present at the top-level package.

@fzakaria fzakaria requested a review from eliben October 16, 2023 20:55
@eliben
Copy link
Owner

eliben commented Oct 20, 2023

@fzakaria yes I have seen the PEP.

What concrete benefit does this provide for elftools at this time? What will you be able to do with elftools once this file is in the tree that you could not have done before?

@fzakaria
Copy link
Contributor Author

Mypy or pyright (static type checkers) fail to infer the types of pyelftools because the file is not present.

It will let me use my static type checker with this library.
At the moment I have to # typed: ignore the import

@eliben eliben merged commit d9ea5f1 into eliben:master Oct 20, 2023
3 checks passed
@JonathonReinhart
Copy link

I think this PR misses the forest (PEP 561) for the trees.

See #533.

As noted there, the purpose of the py.typed marker is to inform static type checkers that a package containing said file includes type information (in the package), either inline (as part of the .py source) or as stubs (separate .pyi files). But elftools currently does neither of those.

Philosophically speaking, if the mere act of adding py.typed to a package (with no type information) accomplishes only good, then every package should have py.typed, in which case tools would not have to check for it, and thus it would be meaningless.

Example

Let's consider the following consumer code:

 1  from elftools.elf.elffile import ELFFile
 2  
 3  def foo() -> None:
 4      elf = ELFFile()
 5       reveal_type(elf)
 6  
 7      for sec in elf.iter_sections():
 8          reveal_type(sec)
 9  
10      elf.nonexistent_method()

Before this PR, when running mpy this file, one would encounter the following error (line 1)

$ mypy foo.py
foo.py:1: error: Skipping analyzing "elftools.elf.elffile": module is installed, but missing library stubs or py.typed marker  [import-untyped]
foo.py:1: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
foo.py:5: note: Revealed type is "Any"
foo.py:8: note: Revealed type is "Any"
Found 1 error in 1 file (checked 1 source file)

After this PR, one does not encounter that error, but rather some actual errors with the consumer code (lines 4, 10):

$ mypy foo.py
foo.py:4: error: Missing positional argument "stream" in call to "ELFFile"  [call-arg]
foo.py:5: note: Revealed type is "elftools.elf.elffile.ELFFile"
foo.py:8: note: Revealed type is "Any"
foo.py:10: error: "ELFFile" has no attribute "nonexistent_method"  [attr-defined]
Found 2 errors in 1 file (checked 1 source file)

So I will admit that this PR does add some basic benefit: mypy now at least recognizes the existence of elftools classes, their methods, and their arguments. (To be honest, I don't understand why mypy doesn't always at least infer this basic level of information, even in light of [import-untyped].)

However, the usefulness stops at the first layer. On line 8, mypy does not know the type returned by iterating elf.iter_sections().

@fzakaria
Copy link
Contributor Author

So I will admit that this PR does add some basic benefit: mypy now at least recognizes the existence of elftools classes, their methods, and their arguments. (To be honest, I don't understand why mypy doesn't always at least infer this basic level of information, even in light of [import-untyped].)

That what I was thinking... seemed like a net-win. I found pyright to be a lot better at infering the types although I ran both on my codebase.

Having read the PEP and that static checkers try to infer types; it felt like it only accomplishes good....
py.typed seems to be only a signal to other developers that the main author is trying to add annotations; the fact it blocks tools feels like the bug.

Maybe the proper fix is in mypy/pyright...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add py.typed file to support typing
4 participants