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

Parse triple quoted string annotations as if parenthesized #15387

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

Glyphack
Copy link
Contributor

@Glyphack Glyphack commented Jan 9, 2025

Summary

Resolves #9467

Parse quoted annotations as if the string content is inside parenthesis.
With this logic x and y in this example are equal:

y: """
   int |
   str
"""

z: """(
    int |
    str
)
"""

Also this rule only applies to triple quotes(link).

This PR is based on the comments on the issue.

I did one extra change, since we don't want any indentation tokens I am setting the State::Other as the initial state of the Lexer.

Remaining work:

  • Add a test case for red-knot.
  • Add more tests.

Test Plan

Added a test which previously failed because quoted annotation contained indentation.
Added an mdtest for red-knot.
Updated previous test.

@Glyphack Glyphack force-pushed the stringized-annotations branch from b33cf05 to efbd25e Compare January 9, 2025 23:43
Copy link
Contributor

github-actions bot commented Jan 9, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

warning: Detected debug build without --no-cache.
error: Failed to read examples/Assistants_API_overview_python.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: expected `,` or `]` at line 197 column 8

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to read examples/Assistants_API_overview_python.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: expected `,` or `]` at line 197 column 8

@Glyphack Glyphack force-pushed the stringized-annotations branch 2 times, most recently from 88460a5 to be3115f Compare January 13, 2025 22:12
@Glyphack Glyphack marked this pull request as ready for review January 13, 2025 22:39
@Glyphack Glyphack changed the title Parse Quoted annotations as if parenthesized Parse triple quoted annotations as if parenthesized Jan 13, 2025
@Glyphack Glyphack changed the title Parse triple quoted annotations as if parenthesized Parse triple quoted string annotations as if parenthesized Jan 13, 2025
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.

Thank you!! The red-knot test and changes look fine to me, but that's a small part of this PR. Would rather have @dhruvmanila take a look at the parser/ruff side.

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.

Thank you. Would you mind adding a test where an expression is incorrectly parenthesized?

def f(
	a: """
		int | 
str)
"""
)

or

def f(
	a: """
		int) | 
str
"""
)

I'm curious how it recovers in those cases and if we need to do something more than just setting nesting = 1

@@ -599,6 +641,8 @@ pub enum Mode {
/// [System shell access]: https://ipython.readthedocs.io/en/stable/interactive/reference.html#system-shell-access
/// [Automatic parentheses and quotes]: https://ipython.readthedocs.io/en/stable/interactive/reference.html#automatic-parentheses-and-quotes
Ipython,

ParenthesizedExpression,
Copy link
Member

Choose a reason for hiding this comment

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

I'd move it next to Expression and let's add a comment explaining what this mode is.

@MichaReiser MichaReiser added the parser Related to the parser label Jan 14, 2025
@Glyphack Glyphack force-pushed the stringized-annotations branch from 93125a2 to b3cf440 Compare January 14, 2025 18:21
@Glyphack
Copy link
Contributor Author

@MichaReiser I added the test cases you suggested and the tests pass(see this commit)
But I think that is because the parser is recovering.

So I fixed the nesting check to check for nesting value of 1 when mode is ParenthesizedExpression. This way the lexer will detect the problem that nesting is one level lower than started as well.

crates/ruff_python_parser/src/lexer.rs Outdated Show resolved Hide resolved
crates/ruff_python_parser/src/lib.rs Outdated Show resolved Hide resolved
parsed.try_into_expression().unwrap().into_result()
}

pub fn parse_as_string_annotation(
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about parse_string_annotation or parse_string_annotation_content? Additionally, we should provide documentation similar to what you've done in parse_parenthesized_expression_range as this is a public function and the crate is being used by multiple projects.

Copy link
Contributor Author

@Glyphack Glyphack Jan 15, 2025

Choose a reason for hiding this comment

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

I agree, renamed to parse_string_annotation and added documentation.

Comment on lines 28 to 29
# single quotes are not implicitly parenthesized
invalid: "\n int"
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the same test case using triple-quoted string which will be useful as a documentation?

# but, using the same annotation inside a triple-quoted string is valid
valid: """\n int"""

Copy link
Contributor Author

@Glyphack Glyphack Jan 15, 2025

Choose a reason for hiding this comment

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

Actually I made a mistake here, invalid: "\n int" is in a python file. So this string will not actually have a newline character but \ and n as characters. So also """"\n int""" fails.
Is there a way to actually create a type annotation that contains newline inside a python file without using triple quotes?

https://pyright-play.net/?code=JYOwbghgNsAmBcACARKgOiRoAurkCh9RIYEVUiRcKg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this right now. Since I could not figure out how to do it correctly.

Comment on lines 32 to 38
invalid2: """
int |
str)
"""

invalid3: """
int) |
str
"""
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some test cases for unclosed parenthesis similar to how these? It would also be useful to provide test cases with multiple parentheses.

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.

Thanks. This looks great.

crates/ruff_python_parser/src/lexer.rs Outdated Show resolved Hide resolved
@Glyphack Glyphack requested a review from dhruvmanila January 15, 2025 20:44
@Glyphack Glyphack force-pushed the stringized-annotations branch from cf342b5 to a74d41e Compare January 15, 2025 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Related to the parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quoted annotations should be parsed as if parenthesized
4 participants