[mypyc] Refactor: reuse format string parser #10894
Conversation
mypyc/irbuild/specialize.py
Outdated
| # The empty Context and MessageBuilder for parse_format_value(). | ||
| # They wouldn't be used since the code has passed the type-checking. | ||
| EMPTY_CONTEXT = Context() | ||
| EMPTY_MSG = MessageBuilder(Errors(), {'': MypyFile([], [])}) |
There was a problem hiding this comment.
Since this is mutable, it would be better to create this on each translate_str_format call. We don't want to risk accumulating state here.
Why do we need the '': MypyFile(...) entry in the dictionary? What happens if we leave it out? If we actually need it, please add a comment explaining why.
There was a problem hiding this comment.
Can I mark EMPTY_MSG as Final?
There was a problem hiding this comment.
It doesn't help -- see my other comment.
mypyc/irbuild/specialize.py
Outdated
| if not can_optimize_format(format_str): | ||
| return None | ||
| specifiers = parse_format_value(format_str, EMPTY_CONTEXT, EMPTY_MSG) | ||
| assert specifiers is not None |
There was a problem hiding this comment.
This may not be safe to do. For example, what if there is a malformed format string and the error has been # type: ignored? I think that it would be better to fall back to the default slow path but still generate IR if we can't parse the format string. What do you think?
mypyc/irbuild/specialize.py
Outdated
| # The empty Context and MessageBuilder for parse_format_value(). | ||
| # They wouldn't be used since the code has passed the type-checking. | ||
| EMPTY_CONTEXT: Final = Context() | ||
| EMPTY_MSG: Final = MessageBuilder(Errors(), {}) |
There was a problem hiding this comment.
Making EMPTY_MSG final doesn't help with the issue I mentioned earlier. Errors() is mutable, so it's now shared mutable state, which we want to usually avoid. Please create MessageBuilder and Errors within translate_str_format to avoid the global state.
[mypyc] Refactor: reuse format string parser (python#10894)
Description
parse_conversion_specifiersandparse_format_valuefrommypy.checkstrformatcan be reused when compiling string formatting.can_optimize_formatandsplit_bracesare useless now.Adding a
start_posas an attribute ofConversionSpecifiercan not only help parse literals but also help generate useful error messages in the future.Test Plan