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.

Sign up for GitHub

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

Jump to bottom

Support type alias statements in simple statement positions #8916

Merged
merged 2 commits into from
Nov 30, 2023
Merged

Support type alias statements in simple statement positions #8916

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01e0119
Support type alias statements in simple statement positions
charliermarsh Nov 30, 2023
5f5a1ce
Expand tests
charliermarsh Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/ruff_python_parser/src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,6 +822,10 @@ type X \
[T] = T
type X[T] \
= T

# simple statements
type X = int; type X = str; type X = type
class X: type X = int
"#;
insta::assert_debug_snapshot!(parse_suite(source, "<test>").unwrap());
}
Expand Down
94 changes: 94 additions & 0 deletions ...ython_parser/src/snapshots/ruff_python_parser__parser__tests__parse_type_declaration.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,4 +849,98 @@ expression: "parse_suite(source, \"<test>\").unwrap()"
),
},
),
TypeAlias(
StmtTypeAlias {
range: 590..602,
name: Name(
ExprName {
range: 595..596,
id: "X",
ctx: Store,
},
),
type_params: None,
value: Name(
ExprName {
range: 599..602,
id: "int",
ctx: Load,
},
),
},
),
TypeAlias(
StmtTypeAlias {
range: 604..616,
name: Name(
ExprName {
range: 609..610,
id: "X",
ctx: Store,
},
),
type_params: None,
value: Name(
ExprName {
range: 613..616,
id: "str",
ctx: Load,
},
),
},
),
TypeAlias(
StmtTypeAlias {
range: 618..631,
name: Name(
ExprName {
range: 623..624,
id: "X",
ctx: Store,
},
),
type_params: None,
value: Name(
ExprName {
range: 627..631,
id: "type",
ctx: Load,
},
),
},
),
ClassDef(
StmtClassDef {
range: 632..653,
decorator_list: [],
name: Identifier {
id: "X",
range: 638..639,
},
type_params: None,
arguments: None,
body: [
TypeAlias(
StmtTypeAlias {
range: 641..653,
name: Name(
ExprName {
range: 646..647,
id: "X",
ctx: Store,
},
),
type_params: None,
value: Name(
ExprName {
range: 650..653,
id: "int",
ctx: Load,
},
),
},
),
],
},
),
]
94 changes: 78 additions & 16 deletions crates/ruff_python_parser/src/soft_keywords.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::{lexer::LexResult, token::Tok, Mode};
use itertools::{Itertools, MultiPeek};

use crate::{lexer::LexResult, token::Tok, Mode};

/// An [`Iterator`] that transforms a token stream to accommodate soft keywords (namely, `match`
/// `case`, and `type`).
///
Expand All @@ -21,7 +22,7 @@ where
I: Iterator<Item = LexResult>,
{
underlying: MultiPeek<I>,
start_of_line: bool,
position: Position,
}

impl<I> SoftKeywordTransformer<I>
Expand All @@ -31,7 +32,11 @@ where
pub fn new(lexer: I, mode: Mode) -> Self {
Self {
underlying: lexer.multipeek(), // spell-checker:ignore multipeek
start_of_line: !matches!(mode, Mode::Expression),
position: if matches!(mode, Mode::Expression) {
Position::Other
} else {
Position::Statement
},
}
}
}
Expand Down Expand Up @@ -59,7 +64,7 @@ where
// (This is to avoid treating `match` or `case` as identifiers when annotated with
// type hints.) type hints.)
Tok::Match | Tok::Case => {
if self.start_of_line {
if matches!(self.position, Position::Statement) {
let mut nesting = 0;
let mut first = true;
let mut seen_colon = false;
Expand Down Expand Up @@ -93,7 +98,10 @@ where
// 2. The type token is immediately followed by a name token.
// 3. The name token is eventually followed by an equality token.
Tok::Type => {
if self.start_of_line {
if matches!(
self.position,
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to your change: It's somewhat unfortunate that it's necessary to track whether we're at the start of the line, because we already know this inside of the Lexer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely. Perhaps we could move this into the lexer...

Position::Statement | Position::SimpleStatement
) {
let mut is_type_alias = false;
if let Some(Ok((tok, _))) = self.underlying.peek() {
if matches!(
Expand Down Expand Up @@ -132,18 +140,56 @@ where
}
}

self.start_of_line = next.as_ref().is_some_and(|lex_result| {
lex_result.as_ref().is_ok_and(|(tok, _)| {
if matches!(tok, Tok::NonLogicalNewline | Tok::Comment { .. }) {
return self.start_of_line;
// Update the position, to track whether we're at the start of a logical line.
if let Some(lex_result) = next.as_ref() {
if let Ok((tok, _)) = lex_result.as_ref() {
match tok {
Tok::NonLogicalNewline | Tok::Comment { .. } => {
// Nothing to do.
}
Tok::StartModule | Tok::Newline | Tok::Indent | Tok::Dedent => {
self.position = Position::Statement;
}
// If we see a semicolon, assume we're at the start of a simple statement, as in:
// ```python
// type X = int; type Y = float
// ```
Tok::Semi => {
self.position = Position::SimpleStatement;
}
// If we see a colon, and we're not in a nested context, assume we're at the
// start of a simple statement, as in:
// ```python
// class Class: type X = int
// ```
Tok::Colon if matches!(self.position, Position::Other) => {
self.position = Position::SimpleStatement;
Copy link
Member

Choose a reason for hiding this comment

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

Just looking at the colon itself might not be sufficient because colons are also used to separate type annotations, slice indices, etc). I'm not sure if this is an issue here, but let's add a few more tests, e.g. a: type X = int (which should not be valid)

Copy link
Member Author

Choose a reason for hiding this comment

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

In the logical line rules, we have some more extensive logic to ensure that the colon follows a statement keyword (like class). I could add that here...

}
Tok::Lpar | Tok::Lsqb | Tok::Lbrace => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You could implement Eq on Position so that you can use == instead of matches

self.position = if let Position::Nested(depth) = self.position {
Position::Nested(depth.saturating_add(1))
} else {
Position::Nested(1)
};
}
Tok::Rpar | Tok::Rsqb | Tok::Rbrace => {
self.position = if let Position::Nested(depth) = self.position {
let depth = depth.saturating_sub(1);
if depth > 0 {
Position::Nested(depth)
} else {
Position::Other
}
} else {
Position::Other
};
}
_ => {
self.position = Position::Other;
}
}

matches!(
tok,
Tok::StartModule | Tok::Newline | Tok::Indent | Tok::Dedent
)
})
});
}
}

next
}
Expand All @@ -161,3 +207,19 @@ fn soft_to_name(tok: &Tok) -> Tok {
name: name.to_owned(),
}
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum Position {
/// The lexer is at the start of a logical line, i.e., the start of a simple or compound statement.
Statement,
/// The lexer is at the start of a simple statement, e.g., a statement following a semicolon
/// or colon, as in:
/// ```python
/// class Class: type X = int
/// ```
SimpleStatement,
/// The lexer is within brackets, with the given bracket nesting depth.
Nested(u32),
/// The lexer is some other location.
Other,
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This and Nested could be combined, since this is just Nested with a depth of 0. Alternatively, Nested could be NonZeroU32. But both felt clunkier in practice. Open to changing, though.

Loading