GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-131798: Narrow the return type of `_FORMAT_SIMPLE` and `_FORMAT_WITH_SPEC` to `str` by NekoAsakura · Pull Request #146639 · python/cpython

Conversation

@NekoAsakura

@NekoAsakura NekoAsakura commented Mar 30, 2026 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Better uop coverage in the JIT optimizer #131798

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@NekoAsakura
pythonGH-131798: Narrow the return type of _FORMAT_SIMPLE and `_FOR…
c1b9a83
…MAT_WITH_SPEC` to `str`
@NekoAsakura
NekoAsakura requested review from Fidget-Spinner, markshannon, savannahostrowski and tomasr8 as code owners March 30, 2026 17:43
@bedevere-app bedevere-app Bot mentioned this pull request Mar 30, 2026
Better uop coverage in the JIT optimizer #131798
Open
@bedevere-app bedevere-app Bot added the awaiting review label Mar 30, 2026
eendebakpt
eendebakpt reviewed Mar 30, 2026
View reviewed changes
Comment thread Python/optimizer_bytecodes.c
res = sym_new_type(ctx, &PyUnicode_Type);
}

op(_FORMAT_WITH_SPEC, (value, fmt_spec -- res)) {

@eendebakpt eendebakpt Mar 30, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This can return a subclass, e.g.

class my(str):
    def __format__(self, spec):
        return self
m=my('aap')
type(f'{m}')

Does the sym_new_type(ctx, &PyUnicode_Type); not guarantee we have an exact type?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@NekoAsakura NekoAsakura Apr 1, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good catch! I oversimplified this. Narrowing type for format isn't as straightforward as I thought.

That said, I'm curious why FORMAT_SIMPLE needs to preserve str subclasses. Had a look through the git history it's been this way since 2015, and seems not a deliberate type-preservation contract.

Current behaviour is already inconsistent:

class my(str):
    def __format__(self, spec):
        return self

m = my('aap')
type('{}'.format(m))   # <class 'my'>
type('x{}'.format(m))  # <class 'str'>
type(f'{m}')           # <class 'my'>
type(f'x{m}')          # <class 'str'>

Multi-piece concatenation already strips it (both _PyUnicodeWriter in str.format and BUILD_STRING in f-strings create exact str).

If FORMAT_SIMPLE normalised to exact str, this inconsistency goes away and type narrowing becomes unconditionally correct. Would that be a reasonable change, or is there a reason to preserve str subclass identity through formatting that I'm missing?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Fidget-Spinner Fidget-Spinner Apr 1, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Huh that's kinda scary. The semantics look a little strange, so maybe we should leave this alone for now?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@NekoAsakura NekoAsakura Apr 1, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fair enough. I just noticed the inconsistency and thought it was worth raising.

For the narrowing itself: we could do conditional narrowing for input types where we can prove __format__ returns exact str (e.g. int, float). But honestly it feels not pythonic to hardcode a list of "safe" types for a uop that's less then 0.1%. I'd prefer just closing this PR unless you think it's worth keeping with that approach?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Fidget-Spinner Fidget-Spinner Apr 1, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Actually we already have something like that called sym_is_safe_type, you can use that

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@NekoAsakura NekoAsakura Apr 1, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I couldn't find sym_is_safe_type anywhere. The closest thing I can see is sym_is_safe_const, but that checks for known constant values rather than types. Have I overlooked something?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Fidget-Spinner Fidget-Spinner Apr 1, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oh yeah sorry that's the one! Just factor out the type checks in _Py_uop_sym_is_safe_const into another function and use that in this case, I think that's fine!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@NekoAsakura NekoAsakura Apr 1, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done. How does it look now?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@NekoAsakura
pythongh-131798: Narrow the return type of _FORMAT_SIMPLE and `_FOR…
ed1204e
…MAT_WITH_SPEC` to str for built-in types
Fidget-Spinner
Fidget-Spinner reviewed Apr 1, 2026
View reviewed changes
Comment thread Python/optimizer_symbols.c Outdated
Comment on lines +278 to +283
return (typ == &PyLong_Type) ||
(typ == &PyUnicode_Type) ||
(typ == &PyFloat_Type) ||
(typ == &_PyNone_Type) ||
(typ == &PyBool_Type) ||
(typ == &PyFrozenDict_Type);

@Fidget-Spinner Fidget-Spinner Apr 1, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oops, I meant to factor this out into a common function, sorry!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@NekoAsakura NekoAsakura Apr 1, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You mean factoring safe types into a static helper? No problem.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@NekoAsakura
pythongh-131798: Narrow the return type of _FORMAT_SIMPLE and `_FOR…
d09a7cc
…MAT_WITH_SPEC` to str for built-in types
@Fidget-Spinner Fidget-Spinner added the skip news label Apr 1, 2026
@Fidget-Spinner

Fidget-Spinner commented Apr 3, 2026

Copy link
Copy Markdown
Member

Going to close and reopen this PR to re-trigger CI. Sorry!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner Fidget-Spinner closed this Apr 3, 2026
@Fidget-Spinner Fidget-Spinner reopened this Apr 3, 2026
Fidget-Spinner and others added 3 commits April 3, 2026 23:23
@Fidget-Spinner
Merge branch 'main' into format-type-narrowing
3dea0df
@NekoAsakura
Merge remote-tracking branch 'upstream/main' into format-type-narrowing
253f55b
# Conflicts:
#	Python/optimizer_symbols.c
@NekoAsakura
Merge branch 'main' into format-type-narrowing
0efc0b5
markshannon
markshannon reviewed May 1, 2026
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good overall. The is_safe_builtin_type function needs fixing up though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/optimizer_symbols.c Outdated
}

static bool
is_safe_builtin_type(PyTypeObject *typ)

@markshannon markshannon May 1, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't like this name. "is safe" for what?

For how we are using this, I don't think frozendict and frozenset should be included as they can contain "unsafe" objects. You should be able to add bytes to the list.

Maybe is_atomic_builtin_type?

Also add a comment explaining that "atomic" means that its values can be evaluated without side effects as they aren't containers. Also that common functions on them are pure and return the expected type, like str(ob) returns a str, int(ob) returns an obj, etc.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

NekoAsakura reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@NekoAsakura NekoAsakura May 1, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks like frozendict and frozenset aren't safe for const folding either. I've pushed a test that captures the behaviour. I am not sure if there is a better fix than just dropping them. Do you want that addressed here, or track in a separate issue?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
NekoAsakura added 4 commits May 1, 2026 10:06
@NekoAsakura
address feedback
e1c0417
@NekoAsakura
Merge branch 'main' into format-type-narrowing
374b4af
@NekoAsakura
add constant folding test
3752820
@NekoAsakura
Revert "add constant folding test"
02c37ad
This reverts commit 3752820.
@read-the-docs-community

read-the-docs-community Bot commented May 3, 2026

Copy link
Copy Markdown

Documentation build overview

📚 cpython-previews | 🛠️ Build #32512649 | 📁 Comparing 02c37ad against main (c1940bc)

  🔍 Preview build  

40 files changed · ± 40 modified

± Modified

  • contents.html
  • genindex-B.html
  • genindex-C.html
  • genindex-E.html
  • genindex-H.html
  • genindex-I.html
  • genindex-L.html
  • genindex-O.html
  • genindex-P.html
  • genindex-R.html
  • and 30 more...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 90 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 10, 2026
@NekoAsakura NekoAsakura closed this Oct 31, 2026
@NekoAsakura
NekoAsakura deleted the format-type-narrowing branch November 5, 2026 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eendebakpt eendebakpt eendebakpt left review comments
@markshannon markshannon markshannon left review comments
@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments
@tomasr8 tomasr8 Awaiting requested review from tomasr8 tomasr8 is a code owner
@savannahostrowski savannahostrowski Awaiting requested review from savannahostrowski

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review skip news stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@NekoAsakura @Fidget-Spinner @eendebakpt @markshannon
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.