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

[red-knot] support fstring expressions #13511

Merged
merged 12 commits into from
Sep 27, 2024

Conversation

Slyces
Copy link
Contributor

@Slyces Slyces commented Sep 25, 2024

Summary

Implement inference for f-string, contributes to #12701.

First Implementation

When looking at the way mypy handles things, I noticed the following:

  • No variables (e.g. f"hello") ⇒ LiteralString
  • Any variable (e.g. f"number {1}") ⇒ str

My first commit (1ba5d0f) implements exactly this logic, except that we deal with string literals just like infer_string_literal_expression (if below MAX_STRING_LITERAL_SIZE, show Literal["exact string"])

Second Implementation

My second commit (90326ce) pushes things a bit further to handle cases where the expression within the f-string are all literal values (string representation known at static time).

Here's an example of when this could happen in code:

BASE_URL = "https://httpbin.org"
VERSION = "v1"
endpoint = f"{BASE_URL}/{VERSION}/post"  # Literal["https://httpbin.org/v1/post"]

As this can be sightly more costly (additional allocations), I don't know if we want this feature.

Test Plan

  • Added a test fstring_expression covering all cases I can think of

@Slyces Slyces changed the title Feat/support fstring expressions support fstring expressions Sep 25, 2024
@Slyces Slyces force-pushed the feat/support-fstring-expressions branch from 0039716 to 8ec8dab Compare September 25, 2024 11:54
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Oh wow nice and thanks for explaining how mypy handles this.

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Sep 25, 2024
Copy link
Contributor

github-actions bot commented Sep 25, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@Slyces Slyces force-pushed the feat/support-fstring-expressions branch 2 times, most recently from 3ee3fcb to 05f15bd Compare September 25, 2024 12:58
@AlexWaygood AlexWaygood changed the title support fstring expressions [red-knot] support fstring expressions Sep 25, 2024
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

This is great, thank you! I think there are still a couple things to address before landing.

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
@Slyces
Copy link
Contributor Author

Slyces commented Sep 25, 2024

I implemented the methods Type::repr to use in f-strings inference.

While doing so, I figured out a couple things:

  • String replacement in f-strings is actually using str (see here)
  • The str method uses repr as a fallback
  • There is a rabbit-hole of the format builtin (see here) to deal with things like f"total ${number:01}" that are out of scope with this PR

@AlexWaygood
Copy link
Member

String replacement in f-strings is actually using str (see here)

Ah! Very good point. I forgot about this -- str() is used unless !r is given as the format specifier, in which case repr is used :-)

@JelleZijlstra
Copy link
Contributor

String replacement in f-strings is actually using str

It actually uses __format__ (unless you use the !s modifier). However, for most objects, this is the same.

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/display.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
Current implementation (based on mypy behaviour):
- evaluate any fstring with expressions as `builtin.str`
- evaluate static fstrings (could be regular strings) as `Literal[..]`
  if it fits the max size or `LiteralString` (same as regular strings)
…sions

Resolves f-strings made entirely of literals (bool, int, str) at compile
time (not supported by mypy).

Example: 'f"{True}"' -> 'True'
Implement the methods `Type::str` and `Type::repr` for some basic types
and use it when infering the type of an f-string expression.

Ensure we always infer sub-expressions in an f-string expression, even
when we're done figuring out its type.
@Slyces Slyces force-pushed the feat/support-fstring-expressions branch from c9dfb96 to 04ed1b5 Compare September 26, 2024 12:15
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

This is very close, thanks for sticking with it! Just a couple small comments yet.

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
@Slyces Slyces requested a review from carljm September 27, 2024 12:40
crates/red_knot_python_semantic/src/types/display.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
@carljm carljm merged commit 1639488 into astral-sh:main Sep 27, 2024
20 checks passed
@Slyces Slyces deleted the feat/support-fstring-expressions branch September 27, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants