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

Improve the "Argument must be a mapping" error message #12222

Merged
merged 5 commits into from
Mar 2, 2022

Conversation

purna135
Copy link
Contributor

Description

Fixes #12070

Improved the error message "error: Argument after ** must be a mapping" when there is a mismatch types

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

Could you add a test case where this makes a difference? I'd grep for existing test cases that use this message, and add another one that would be different without this PR.

@purna135
Copy link
Contributor Author

Hi @JelleZijlstra, I'm completely new to this, so I'm a little perplexed right now.
I made the following changes to the check-kwargs.test file:

[case testInvalidTypeForKeywordVarArg]
from typing import Dict
from typing import Any, Optional
def f(**kwargs: 'A') -> None: pass
d = None # type: Dict[A, A]
f(**d)         # E: Keywords must be strings
f(**A())       # E: Argument after ** must be a mapping, not "A"
class A: pass
kwargs: Optional[Any]
f(**kwargs)    # E: Argument after ** must be a mapping, not "Union[Any, None]"
[builtins fixtures/dict.pyi]

But running the pytest -n0 -k 'testInvalidTypeForKeywordVarArg' gives the below output

E   AssertionError: Unexpected type checker output (C:\Users\Purna\Downloads\OpenSource\mypy\test-data\unit\check-kwargs.test, line 352)
------------------------------------------------------------------- Captured stderr call -------------------------------------------------------------------
Expected:
  main:5: error: Keywords must be strings
  main:6: error: Argument after ** must be a mapping, not "A"
  main:9: error: Argument after ** must be a mapping, not "Union[Any, None]" (diff)
Actual:
  main:5: error: Keywords must be strings
  main:6: error: Argument after ** must be a mapping, not "A"

================================================================= short test summary info ==================================================================
FAILED mypy/test/testcheck.py::TypeCheckSuite::check-kwargs.test::testInvalidTypeForKeywordVarArg

Please guide how to solve this, Thanks

@JelleZijlstra
Copy link
Member

You may have to turn on strict optional mode in the test case (put # flags: --strict-optional at the top of the case).

@purna135
Copy link
Contributor Author

Thanks @JelleZijlstra, this time I got some error : )
First of all there was a Incompatible types due to d = None # type: Dict[A, A], so I removed this check temporarily to check my test.

Below is the error, what I got before this PR
Test:

[case testInvalidTypeForKeywordVarArg]
# flags: --strict-optional
from typing import Dict
from typing import Any, Optional
def f(**kwargs: 'A') -> None: pass
f(**A())       # E: Argument after ** must be a mapping, not "A"
class A: pass
kwargs: Optional[Any]
f(**kwargs)    # E: Argument after ** must be a mapping, not "Union[Any, None]"
[builtins fixtures/dict.pyi]

Output:

Expected:
  main:5: error: Argument after ** must be a mapping, not "A"
  main:8: error: Argument after ** must be a mapping, not "Union[Any, None]" (diff)
Actual:
  main:5: error: Argument after ** must be a mapping, not "A"
  main:8: error: Argument after ** must be a mapping (diff)

Alignment of first line difference:
  E: ...** must be a mapping, not "Union[Any, None]"
  A: ...** must be a mapping

After this PR, below is the output so I have to replace Union[Any, None] with Optional[Any]

Expected:
  main:5: error: Argument after ** must be a mapping, not "A"
  main:8: error: Argument after ** must be a mapping, not "Union[Any, None]" (diff)
Actual:
  main:5: error: Argument after ** must be a mapping, not "A"
  main:8: error: Argument after ** must be a mapping, not "Optional[Any]" (diff)

Alignment of first line difference:
  E: ...** must be a mapping, not "Union[Any, None]"
  A: ...** must be a mapping, not "Optional[Any]"
                                   ^

Shoul I replace the message with

self.fail(
      'Argument after ** has incompatible type {}, expected "Mapping[str, Any]"'.format(format_type(typ)),
      context, code=codes.ARG_TYPE)

@purna135
Copy link
Contributor Author

and one more question, should I remove # flags: --strict-optional from test while commiting the changes ?

@purna135
Copy link
Contributor Author

Hello, I'm waiting for your response; could you please direct me in the right direction?

@JelleZijlstra
Copy link
Member

Hi, that's a lot of questions.

For Union[Any, None], just change the test output to whatever variant makes it pass. We recently merged a PR that I think affects the output here.

I'm not sure why you are suggesting a message with Mapping[str, Any] in it. I feel like that would make the message harder to read for users.

And no, the --strict-optional should stay, because otherwise the tests will fail.

@purna135
Copy link
Contributor Author

Thanks @JelleZijlstra

--strict-optional causes the test to fail for below case with the message main:4: error: Incompatible types in assignment (expression has type "None", variable has type "Dict[A, A]")

def f(**kwargs: 'A') -> None: pass
d = None # type: Dict[A, A]
f(**d)         # E: Keywords must be strings

I am pushing the changes please have a look.

@@ -350,12 +350,15 @@ class A: pass
[builtins fixtures/dict.pyi]

[case testInvalidTypeForKeywordVarArg]
from typing import Dict
# flags: --strict-optional
from typing import Dict, Any, Optional
def f(**kwargs: 'A') -> None: pass
d = None # type: Dict[A, A]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
d = None # type: Dict[A, A]
d = {} # type: Dict[A, A]

Does this work?

@purna135
Copy link
Contributor Author

Yes now it's passing all the tests, Thanks : )

@JelleZijlstra JelleZijlstra self-assigned this Mar 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit 26bfc63 into python:master Mar 2, 2022
@JelleZijlstra
Copy link
Member

Thanks for your contribution!

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.

Improve the "Argument must be a mapping" error message
2 participants