Skip to content

Conversation

@amcn
Copy link
Contributor

@amcn amcn commented Sep 16, 2024

We always pass the string value of the mode to determine_args, which causes the check on the mode argument inside determine_args to always evaluate to false.

Fix this by passing the mode itself, not its value.

Discovered this while investigating #13610. I noticed that determine_args is always called with a string as its mode argument, causing the condition at line 275 to always evaluate to false:

https://github.com/amcn/meson/blob/2b80d4cca174916a4b6d0c47c54ad56795741bb6/mesonbuild/interpreter/compiler.py#L266-L280

We always pass the string value of the mode to determine_args, which
causes the check on the mode argument inside determine_args to always
evaluate to false.

Fix this by passing the mode itself, not its value.
@amcn amcn requested a review from dcbaker as a code owner September 16, 2024 23:38
@amcn
Copy link
Contributor Author

amcn commented Sep 16, 2024

Pushing this up to CI in order to get a sense of its test impact. Also the commit message could use some clarification work but I'm not sure what would be best there.

Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

I wonder why mypy isn't catching this

@dcbaker dcbaker added this to the 1.5.2 milestone Sep 17, 2024
@dcbaker dcbaker merged commit 9cb9ad8 into mesonbuild:master Sep 17, 2024
@amcn
Copy link
Contributor Author

amcn commented Sep 17, 2024

I wonder why mypy isn't catching this

It's definitely strange. I wonder if mypy is able to account for the indirection introduced by this code-path's use of functools.partial in its analysis.

Update: support was recently added to mypy for functools.partial: python/mypy#16939

@eli-schwartz
Copy link
Member

The support for functools.partial also has huge gaps and flags all sorts of completely correct code as problematic. :)

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.

3 participants