Skip to content

bpo-40631: Disallow star targets with parentheses around #24019

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

Closed
wants to merge 1 commit into from

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Dec 31, 2020

@pablogsal
Copy link
Member Author

I am not completely happy about this approach but is late here and I cannot see immediately a better way without refactoring the whole star_targets.

Copy link
Member

@lysnikolaou lysnikolaou left a comment

Choose a reason for hiding this comment

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

Let's not break the rule that error messages only get generated on the second pass. I already got a refactoring for star_targets in mind, so I could take it up.

@pablogsal
Copy link
Member Author

Let's not break the rule that error messages only get generated on the second pass. I already got a refactoring for star_targets in mind, so I could take it up.

The base of the problem here is that if we don't immediately fail here the parsing continues and some other rule in the targets succeed. In particular it allows to recurse into the rule that accept a star target surrounded by parentheses. In other words: if we don't explicitly fail there is another rule that will succeed, as opposed to the parsing failing.

@lysnikolaou
Copy link
Member

Let's not break the rule that error messages only get generated on the second pass. I already got a refactoring for star_targets in mind, so I could take it up.

The base of the problem here is that if we don't immediately fail here the parsing continues and some other rule in the targets succeed. In particular it allows to recurse into the rule that accept a star target surrounded by parentheses. In other words: if we don't explicitly fail there is another rule that will succeed, as opposed to the parsing failing.

Let me quickly open a PR to show what I've got so far, which seems to be working.

@lysnikolaou
Copy link
Member

Here it is: #24027.

@pablogsal
Copy link
Member Author

Here it is: #24027.

Nice! I like that approach better. it's up to you if you want to move the tests in my PR to yours or change the grammar file here to use your approach :)

@lysnikolaou
Copy link
Member

Cool cool. I've moved all your tests to that PR. I'll let you close this.

@pablogsal pablogsal closed this Dec 31, 2020
@pablogsal pablogsal deleted the bpo-40631 branch May 19, 2021 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants