Skip to content

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Oct 18, 2016

Previously the fast parser would crash ("assert False" in stringify_name()). Now it forces an error.

I didn't see a decent way to report an error from fastparser.py, so I set the metaclass name to "<error>" and check for that in semanal.py which will report a decent error.

Note that the old parser still reports a syntax error on the '(', I think that's fine. Also there doesn't seem to be a corresponding issue with Python 2.

Fixes #2273.

@gvanrossum
Copy link
Member Author

@JukkaL would there be a better way to report this as an error?

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 19, 2016

Raise an exception and catch it at the top-level parse function? We already have TypeCommentParseError, and if there's a parse error, we give up and don't return the AST. This error is probably very rare, so losing errors from the following passes seems like a non-issue.

@gvanrossum
Copy link
Member Author

I'm not so sure that it's okay to treat this as a parse error. This is existing non-annotation code and people may have a good reason for it. We should produce an error explaining that we can't process the metaclass, and ignore it. So I don't think an exception would be better than what I have.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 20, 2016

Yeah, that's a good point. A better fix would be to make ClassDef.metaclass a node instead of a string.

@gvanrossum
Copy link
Member Author

I thought I had responded here but don't see my response.

I tried making it a node but the refactor was too complex given the simple problem. Can we just merge this? It's a crash bug (with the new parser).

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 24, 2016

Agreed, this isn't worth a big refactor.

@JukkaL JukkaL merged commit 6938e7b into master Oct 24, 2016
@gvanrossum gvanrossum deleted the dyn-metaclass branch October 24, 2016 17:05
@gvanrossum
Copy link
Member Author

gvanrossum commented Oct 25, 2016 via email

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 25, 2016

Were your comments delayed by several days? Odd.

@gvanrossum
Copy link
Member Author

Yeah, looks I replied by email and the email only showed up an hour ago. Weird.

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

Successfully merging this pull request may close these issues.

2 participants