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

f-string parser: Check comments in f-strings #157

Open
pablogsal opened this issue Oct 9, 2021 · 9 comments
Open

f-string parser: Check comments in f-strings #157

pablogsal opened this issue Oct 9, 2021 · 9 comments

Comments

@pablogsal
Copy link

pablogsal commented Oct 9, 2021

In test_fstring the test test_comments fails. We need to check if is due to regular tokenization problems and the test needs updating or we need to fix something regarding comments in f-strings

@pablogsal pablogsal changed the title f-string parser: f-string parser: Check comments in f-strings Oct 9, 2021
@FFY00
Copy link

FFY00 commented Oct 16, 2021

Old:

$ ./python -c 'f"{)#}"'
  File "<string>", line 1
    f"{)#}"
           ^
SyntaxError: f-string: unmatched ')'

New:

$ ./python -c 'f"{)#}"'
  File "<string>", line 1
    f"{)#}"
       ^
SyntaxError: closing parenthesis ')' does not match opening parenthesis '{'

I don't think the new error is good, and seems like a regression, right?

@pablogsal
Copy link
Author

pablogsal commented Oct 16, 2021

Yeah, ideally we should preserve the old error message: the new expression inside should be like a new parse completely regarding matchings.

@isidentical
Copy link
Collaborator

We already handle the comments inside the f-strings (it just raises an error), so this seems more of an error message discussion. I'd say the new error message is no worse than the old one, since it sort of makes sense. You probably wouldn't put # inside an expression, so the closing paranthesis is probably meant for the {.

@lysnikolaou
Copy link
Member

I think that this would be solved by moving the parenstack into the mode struct which gets pushed/popped for every new tokenizer mode. In general, I think a bigger discussion is needed here for what stays in the top-level tok_state and what needs to be moved into _tokenizer_mode.

@pablogsal
Copy link
Author

a bigger discussion is needed here for what stays in the top-level tok_state and what needs to be moved into _tokenizer_mode.

We can probably delay this since at the end this is about the error message and that doesn't necessarily need to be preserved

@pablogsal
Copy link
Author

pablogsal commented Dec 16, 2022

There is an actual problem here as we are allowing comments inside any f-string (multi-lined or not). We may want to change the PEP or the implementation.

CC: @isidentical @lysnikolaou what do you think?

@isidentical
Copy link
Collaborator

There is an actual problem here as we are allowing comments inside any f-string (multi-lined or not).

Can you give an example @pablogsal? If someone tries to use a comment inside an f-string expression, than anything after the comment (including the quote character itself) is considered invalid so I don't understand how it is possible to have a valid single-line f-string that also has a comment.

>>> f"{ 1 + 2 # comment }"
  File "<stdin>", line 1
    f"{ 1 + 2 # comment }"
              ^
SyntaxError: f-string expression part cannot include '#'

@pablogsal
Copy link
Author

pablogsal commented Dec 16, 2022

Ah, I forgot to specify how we are allowing them:

>>> f"{ 1 + 2 = # comment }"
  File "<stdin>", line 1
    f"{ 1 + 2 = # comment }"
                            ^
SyntaxError: f-string: expecting '}'

In this case, no error message is being triggered and the comment works. Before we were getting this:

>>> f"{ 1 + 2 = # comment }"
  File "<stdin>", line 1
    f"{ 1 + 2 = # comment }"
                ^
SyntaxError: f-string expression part cannot include '#'

I am using the fstring-grammar-rebased-after-sprint branch.

@pablogsal
Copy link
Author

Hummmmm, I apologize, but I think I was using the wrong branch for some reason. I am checking again and seems that this doesn't happen in the last fstring-grammar-rebased-after-sprint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants