Update lexer snapshots with token flags by dhruvmanila · Pull Request #11614 · astral-sh/ruff

dhruvmanila · 2024-05-30T08:27:08Z

Summary

This PR updates the lexer test snapshots to include the TokenFlags.

Test Plan

Verify the snapshots.

codspeed-hq · 2024-05-30T08:31:32Z

CodSpeed Performance Report

Merging #11614 will not alter performance

_{Comparing dhruv/lexer-tests (66953c5) with dhruv/parser-phase-2 (d55bfcc)}

Summary

✅ 30 untouched benchmarks

MichaReiser · 2024-05-30T08:31:39Z

crates/ruff_python_parser/src/snapshots/ruff_python_parser__lexer__tests__empty_fstrings.snap

+        FStringStart,
        11..13,
+        TokenFlags(
+            F_STRING,


what's the usage of the FStringFlag. Can't we tell that it is an FString by looking at the TokenKind?

Yeah, good point. Currently, it's only used inside the lexer to call either lex_fstring_start or lex_string method.

I think it's required because we represent regular string with no string kind flag set. And, prefix method would also not be able to differentiate because the flags themselves don't have access to the token kind.

Hmm okay. I would need to check out the code to better understand it. We could move the prefix method to the Token? And could we only keep it for FStringFlags? But it's probably not that important.

dhruvmanila · 2024-05-30T11:24:16Z

crates/ruff_python_parser/src/lexer.rs


-        // Keep the current flags in sync throughout the f-string context.
-        self.current_flags = fstring.flags();
-
        // Check if we're at the end of the f-string.
        if fstring.is_triple_quoted() {
            let quote_char = fstring.quote_char();
            if self.cursor.eat_char3(quote_char, quote_char, quote_char) {
+                self.current_flags = fstring.flags();
                return Some(TokenKind::FStringEnd);
            }
        } else if self.cursor.eat_char(fstring.quote_char()) {
+            self.current_flags = fstring.flags();
            return Some(TokenKind::FStringEnd);


Set the token flags only if we know that it's FStringEnd.

dhruvmanila · 2024-05-30T11:24:25Z

crates/ruff_python_parser/src/lexer.rs


+        self.current_flags = fstring.flags();
        Some(TokenKind::FStringMiddle)


Set the token flags only if we know that it's FStringMiddle.

dhruvmanila · 2024-05-30T11:35:45Z

(I'll look at the remaining test cases in a separate PR.)

MichaReiser · 2024-05-30T12:27:45Z

crates/ruff_python_parser/src/lexer.rs

+            let mut tuple = if matches!(self.value, TokenValue::None) {
+                tuple.field(&self.kind)
+            } else {
+                tuple.field(&self.value)
+            };
+            tuple = tuple.field(&self.range);


Nit: At least debug_map etc. mutate the state in place.

Suggested change

let mut tuple = if matches!(self.value, TokenValue::None) {

tuple.field(&self.kind)

} else {

tuple.field(&self.value)

};

tuple = tuple.field(&self.range);

if matches!(self.value, TokenValue::None) {

tuple.field(&self.kind);

} else {

tuple.field(&self.value);

};

tuple.field(&self.range);

Are you suggesting to use debug_map? I want to keep the tuple here to have minimal diff which makes it easier to verify the changes. We can explore other formats as a follow-up.

Oh no, I'm suggesting to not use let mut tuple = tuple.field because I think it might not be necessary (it's not necessary for debug_map)

MichaReiser · 2024-05-30T12:28:49Z

crates/ruff_python_parser/src/lexer.rs

+                tuple.field(&self.value)
+            };
+            tuple = tuple.field(&self.range);
+            if self.flags.is_empty() {


Nit: Maybe

if !self.flags.is_empty() { tuple.field(&self.flags); } tuple.finish()

MichaReiser · 2024-05-30T12:31:00Z

crates/ruff_python_parser/src/snapshots/ruff_python_parser__lexer__tests__empty_fstrings.snap

+        FStringStart,
        11..13,
+        TokenFlags(
+            F_STRING,


Hmm okay. I would need to check out the code to better understand it. We could move the prefix method to the Token? And could we only keep it for FStringFlags? But it's probably not that important.

## Summary This PR updates the lexer test snapshots to include the `TokenFlags`. ## Test Plan Verify the snapshots.

dhruvmanila added the testing Related to testing Ruff itself label May 30, 2024

MichaReiser reviewed May 30, 2024

View reviewed changes

dhruvmanila force-pushed the dhruv/compile branch from 68e781e to dc0ed20 Compare May 30, 2024 10:04

dhruvmanila force-pushed the dhruv/lexer-tests branch from 6b4c77f to 71113db Compare May 30, 2024 10:04

dhruvmanila force-pushed the dhruv/compile branch from dc0ed20 to 5f634e1 Compare May 30, 2024 10:22

dhruvmanila force-pushed the dhruv/lexer-tests branch from 71113db to 2d0d520 Compare May 30, 2024 10:34

dhruvmanila force-pushed the dhruv/compile branch from 5f634e1 to b422554 Compare May 30, 2024 10:52

Base automatically changed from dhruv/compile to dhruv/parser-phase-2 May 30, 2024 11:14

Update lexer tests with token flags

66953c5

dhruvmanila force-pushed the dhruv/lexer-tests branch from 2d0d520 to 66953c5 Compare May 30, 2024 11:14

dhruvmanila commented May 30, 2024

View reviewed changes

dhruvmanila marked this pull request as ready for review May 30, 2024 11:35

MichaReiser approved these changes May 30, 2024

View reviewed changes

dhruvmanila merged commit 400d074 into dhruv/parser-phase-2 May 30, 2024

dhruvmanila deleted the dhruv/lexer-tests branch May 30, 2024 15:11

dhruvmanila added a commit that referenced this pull request May 31, 2024

Update lexer snapshots with token flags (#11614)

3a7b9e2

## Summary This PR updates the lexer test snapshots to include the `TokenFlags`. ## Test Plan Verify the snapshots.

dhruvmanila added a commit that referenced this pull request Jun 3, 2024

Update lexer snapshots with token flags (#11614)

cffcdab

## Summary This PR updates the lexer test snapshots to include the `TokenFlags`. ## Test Plan Verify the snapshots.


		self.current_flags = fstring.flags();
		Some(TokenKind::FStringMiddle)

Conversation

dhruvmanila commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

codspeed-hq bot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #11614 will not alter performance

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvmanila commented May 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dhruvmanila commented May 30, 2024 •

edited

Loading

codspeed-hq bot commented May 30, 2024 •

edited

Loading