[Red-Knot] String Annotations

[Slyces]

Hi, I am opening a discussion as I am not sure this belongs in a github issue.

As I worked on future annotations in red-knot a short while ago, I noticed that there is no support for string annotations yet. I did locate a couple // TODO related to this.

I might be interested in coding this feature at some point, however, I am not sure on how to proceed.

I know that given the string, I can use a the python parser to produce AST nodes and probably infer the type of a string annotation. However, this leads to problems in the current architecture as those AST nodes are not known for the SemanticIndex and symbols table.

Could we consider solving this by parsing StringAnnotations directly on the first run, making them into maybe a new variant? Or is there a way I don't see yet to achieve this by keeping those nodes as StringLiteral but parsing at inference time?

[MichaReiser]

Yeah, what you outline is correct. Stringified annotations are a pain, and we don't have a good solution yet to support them well.

• Ruff parses the stringified annotations on demand. However, that comes with its own challenges: For example that all node offsets are off. This is more of a problem for Red Knot where we use offsets to uniquely identify nodes...
• We could extend our AST and parse out the annotations. We have to consider that we want to use the same AST for the formatter so that type checking and formatting a file doesn't require parsing it twice. That means the AST needs to contain enough information for the formatter and type checker. The relevant information for the formatter is understanding the implicit concatenated string parts. So these should be retained in a representation.... However, it's valid to write "Lit" "ereal[10]" and it should be parsed as a single expression for type checking... We have to explore how an AST could support both use cases (e.g., introduce a new expression variant and store the string offsets separately?)

Whenever possible, I prefer to solve this in the parser using a new parser mode (so that we don't need to rewrite ruff). But it does require design on how to make this fit into the AST.

I'm not sure if I remember it correctly but I believe @dhruvmanila wanted to look into this after this PTO but we can also support you designing and implementing this change if you're interested.

CC: @carljm

[MichaReiser]

I'm confused by this, because I mentioned already that the type checker will also need to be able to get access to the full original string literal in some cases.

Sorry I missed that

We also need to handle partially stringified annotations, e.g. SomeGenericType["ForwardReference"]. So whatever handling we add here doesn't just apply to annotations that are strings at the top level, but to any string found within an annotation.

Considering that, handling it in the parser seems less appealing to me because the result is that some string annotations are handled in the parser while others still require special handling in more higher level crates.

Does the parser ever emit diagnostics?

The parser always emits a diagnostic when it recovers.

[carljm]

Considering that, handling it in the parser seems less appealing to me because the result is that some string annotations are handled in the parser while others still require special handling in more higher level crates.

I would assume that if we handle it in the parser, the parser would track recursively when it is within an annotation, and provide this handling for all strings found within an annotation.

I agree that if we can't (or don't want to) do that, then we shouldn't do this in the parser at all.

[MichaReiser]

I would assume that if we handle it in the parser, the parser would track recursively when it is within an annotation, and provide this handling for all strings found within an annotation.

The issue I see is that some strings are indeed just strings and there's no good way for the parser to know which string is a string and which isn't. It leads to very obscure results if you want the recovery logic described here

If this string is in annotation position, and we are parsing stringified annotations in the parser and providing an AST for the string contents to consumers of the parser, then in this case we should still provide an AST with an invalid-syntax node

because the parser then tries to parse the inner string of Literal["TestString with spaces"] as an expression, where it probably has to parse out a tuple or three identifiers (TestString, with spaces) as the "closest" valid expression, which is very far-fetched and unnecessary.

[carljm]

If we don't want to ever speculatively parse a string in an annotation that probably is (but might in the end not be) a stringified annotation, then I agree we can't do this in the parser.

It will also mean some difficulty for red-knot, since a lot of our architecture currently assumes that we have the full AST up-front, and we can number nodes, semantic index, etc. But I suspect we can handle this by treating each stringified annotation as almost a separate "file", but with some additional prepopulated builtin/global names, or something along those lines. Name lookup in stringified annotations is going to be weird and special-cased anyway (e.g. see the issue @Viicos mentioned below.)

[MichaReiser]

It will also mean some difficulty for red-knot, since a lot of our architecture currently assumes that we have the full AST up-front, and we can number nodes,

Possibly yes, unless we can also number the parsed out type annotations when we number the main AST.

[Viicos]

On a slightly related note, is red knot going to resolve forward annotations the same way it does for "normal" annotations? Because of a backward compatibility behavior of typing.get_type_hints, module scope is given priority for forward annotations (see microsoft/pyright#8958).

[carljm]

We haven't discussed this explicitly yet, but I suspect we will have to match the behavior of typing.get_type_hints and pyright here.

[Viicos]

Makes sense, thanks for the answer (and anyway hopefully string annotations will be a thing of the past at some point with PEP 649)

[carljm]

We do need to implement support for stringified annotations, which means we should have an open issue for it. So I am going to close this discussion for now and open an issue for implementing stringified annotations, with summaries of some of the things discussed here, and a reference back to this discussion.

[carljm]

#13796