Skip to content

NoneType handling for str.format() with specifiers #18952

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.

Sign up for GitHub

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

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

NoneType handling for str.format() with specifiers #18952

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3fe1aa7
Check for None type in format call
VyZhu Apr 21, 2025
4180b55
Add check for alignment specifier on none
VyZhu Apr 21, 2025
49d8fbf
Fix alignment specifier check to be more dynamic
VyZhu Apr 21, 2025
c5d0b71
Move alignment check to special check function
VyZhu Apr 21, 2025
31542a1
Added unit tests to check for Error Message for None argument and als…
ChristinaTrinh Apr 21, 2025
30665c0
Merge pull request #1 from VallinZ/unitTest
ChristinaTrinh Apr 21, 2025
feda1d3
Attempt to recognize no __format__ functions must result in error
ChristinaTrinh Apr 21, 2025
5c4e701
Merge branch 'master' of https://github.com/VallinZ/mypy into unitTest
ChristinaTrinh Apr 21, 2025
2286f46
Deleted commented out code
ChristinaTrinh Apr 21, 2025
8b94be8
Remove print for test
VyZhu Apr 21, 2025
d4f70fb
Fix isinstance call with get_property_type
VyZhu Apr 21, 2025
e3d002e
Fix lint for alignment specifier on none
VyZhu Apr 21, 2025
c536cab
Merge pull request #3 from VallinZ/AddAlignmentSpecifierLogic
VallinZ Apr 22, 2025
0d329dd
Initial approach for distribute Tuple issue
VyZhu Apr 24, 2025
693486e
Format changed by Linter
ChristinaTrinh Apr 24, 2025
cfc6b17
Fix merge conflicts
ChristinaTrinh Apr 24, 2025
bcf28be
Attempt to fix max recursive depth for distr tuple
VyZhu Apr 24, 2025
066a0f3
Move code of distribute tuple
VyZhu Apr 24, 2025
c979af9
Extend check for format string specifiers and added more unit tests
ChristinaTrinh Apr 26, 2025
d46f562
Fix lint error
ChristinaTrinh Apr 26, 2025
ade0d1b
Clean up and add comments
VyZhu Apr 26, 2025
35cf276
Merge pull request #5 from VallinZ/AddAlignmentSpecifierLogic
ChristinaTrinh Apr 26, 2025
6949731
Change function name to match pattern
VyZhu Apr 28, 2025
442d8c5
Merge pull request #4 from VallinZ/DistributeTuple
ChristinaTrinh Apr 30, 2025
e5e2430
Revert "Attempt to fix distribute Tuple issue "
VallinZ Apr 30, 2025
daac179
Merge pull request #7 from VallinZ/revert-4-DistributeTuple
VallinZ Apr 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions mypy/checkstrformat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
AnyType,
Instance,
LiteralType,
NoneType,
TupleType,
Type,
TypeOfAny,
Expand Down Expand Up @@ -98,7 +99,6 @@ def compile_new_format_re(custom_spec: bool) -> Pattern[str]:

# Conversion (optional) is ! followed by one of letters for forced repr(), str(), or ascii().
conversion = r"(?P<conversion>![^:])?"

# Format specification (optional) follows its own mini-language:
if not custom_spec:
# Fill and align is valid for all builtin types.
Expand All @@ -113,7 +113,6 @@ def compile_new_format_re(custom_spec: bool) -> Pattern[str]:
else:
# Custom types can define their own form_spec using __format__().
format_spec = r"(?P<format_spec>:.*)?"

return re.compile(field + conversion + format_spec)


Expand Down Expand Up @@ -449,6 +448,24 @@ def perform_special_format_checks(
call,
code=codes.STRING_FORMATTING,
)
a_type = get_proper_type(actual_type)
if isinstance(a_type, NoneType):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be cleaner:

Suggested change
a_type = get_proper_type(actual_type)
if isinstance(a_type, NoneType):
if isinstance(get_proper_type(actual_type), NoneType):

# Perform type check of alignment specifiers on None
if spec.format_spec and any(c in spec.format_spec for c in "<>^"):
specifierIndex = -1
for i in range(len("<>^")):
if spec.format_spec[i] in "<>^":
specifierIndex = i
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic seems confused -- why are we mixing i (refers to position in <>^) with indexing into spec.format_spec?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be just

specifier_char = next(c for c in spec.format_spec if c in "<>^")

and then specifier_char instead of spec.format_spec[specifierIndex] (and next if is redundant, obviously, since you already check that such symbol exists).

Aside, please stick to snake_case naming convention - it's both used in this codebase and recommended by PEP8, only very old stdlib parts still have some camelCase identifiers.

if specifierIndex > -1:
self.msg.fail(
(
f"Alignment format specifier "
f'"{spec.format_spec[specifierIndex]}" '
f"is not supported for None"
),
call,
code=codes.STRING_FORMATTING,
)

def find_replacements_in_call(self, call: CallExpr, keys: list[str]) -> list[Expression]:
"""Find replacement expression for every specifier in str.format() call.
Expand Down
10 changes: 10 additions & 0 deletions test-data/unit/check-formatting.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,16 @@ b'%c' % (123)
-- ------------------


[case testFormatCallNoneAlignment]
'{:<1}'.format(None) # E: Alignment format specifier "<" is not supported for None
'{:>1}'.format(None) # E: Alignment format specifier ">" is not supported for None
'{:^1}'.format(None) # E: Alignment format specifier "^" is not supported for None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a few more tests - at least with f-strings, with conversion (e.g. "{!s:>5}".format(None) is allowed) and with dynamic spec (f"{None:{foo}}" is also valid, foo may be an empty string).

Copy link

@VallinZ VallinZ Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm looking at the case of

foo = "^"'
test2 = f"{None:{foo}}"

which python throws an error but the current implementation does not catch it.
I look into the callExp of this case, it return:

CallExpr:9(
  MemberExpr:9(
    StrExpr({:{}})
    format)
  Args(
    NameExpr(None [builtins.None])
    CallExpr:9(
      MemberExpr:9(
        StrExpr({:{}})
        format)
      Args(
        NameExpr(foo [mypy.LocalTest.test.foo])
        StrExpr()))))

Is there a way to see what value foo represent in MyPy? My understanding is that MyPy is a static tool, so it doesn't have the value of foo. So, technically, there is no way to check this case? So, would it be an option we just raise an warning that there might be an error rather saying that this is an error?

Copy link
Collaborator

Choose a reason for hiding this comment

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

would it be an option we just raise an warning

no, false positives should be avoided at all costs in mypy. If we can't statically prove that some code is invalid, mypy should be green.

There are Literal types and Instance.last_known_value field, representing the literal vale if we know it. However, we accept any dynamic specifiers everywhere else, so I think this should be deferred to a separate PR if you consider it important enough. For now I'd suggest just ignoring any dynamic spec components; formatting with dynamic alignment specifier should be a very rare case (IMO not worth supporting at all).


'{:<10}'.format('16') # OK
'{:>10}'.format('16') # OK
'{:^10}'.format('16') # OK
[builtins fixtures/primitives.pyi]

[case testFormatCallParseErrors]
'}'.format() # E: Invalid conversion specifier in format string: unexpected }
'{'.format() # E: Invalid conversion specifier in format string: unmatched {
Expand Down