Syntaxerror in variable unpacking 487#662
Syntaxerror in variable unpacking 487#662slozier merged 6 commits intoIronLanguages:masterfrom hackf5:syntaxerror_in_variable_unpacking_487
Conversation
…mplicit tuple unpacking.
slozier
left a comment
slozier
left a comment
There was a problem hiding this comment.
Here are my initial comments.
Did you have a chance to compare with my branch?
|
|
||
| if (left != null) { | ||
| Debug.Assert(left.Count > 0); | ||
| CheckNotAssignmentTargetOnly(right); |
There was a problem hiding this comment.
Not that I necessarily have an issue with doing this as an initial implementation, but CPython doesn't do these checks in the parser. For example, with CPython you can do x = ast.parse("a = 1,*b") without problem, with this implementation, ast.parse will throw. I should only fail when you try to compile the AST (e.g. compile(x, None, 'exec')).
If you want to be sneaky, with CPython you can print the AST, construct the same in IronPython (you can't use ast.parse since that uses the parser), compile and end up with a TypeError: Unexpected expr type: IronPython.Modules._ast+Starred.
As I mention, I am fine with not going down that rabbit hole for now since we're in the world of invalid Python code anyway. Although, we should definitely file an issue if we go ahead and merge this in.
| ret = new ListExpression(); | ||
| } else { | ||
| bool prevAllow = _allowIncomplete; | ||
| } |
There was a problem hiding this comment.
Just a note on style: } else {
If you're using VS then formatting the document should pick up the editorconfig and apply the correct style throughout.
There was a problem hiding this comment.
I've got VS and also ReSharper. My auto-format is currently doing too much as it is setup to match the StyleCop format, however closing an outer brace works.
I could take a look at formalizing the coding standard with some analyzers if you are interested?
There was a problem hiding this comment.
Is there an analyzer that picks up the rules from editorconfig? Some parts of the codebase (e.g. BZip2 and c#sqlite) have their own inconsistent style (although I haven't set up an editorconfig for those).
There was a problem hiding this comment.
No, but VS with ReSharper should pick it up.
| CheckNotAssignmentTargetOnly(right); | ||
|
|
||
| var target = left.ToArray(); | ||
| var target = left?.ToArray() ?? new [] { singleLeft }; |
There was a problem hiding this comment.
Looking at the method without context it wasn't clear that singleLeft is never null at this point. I wonder if it's worth adding Debug.Assert(singleLeft != null). Maybe also Debug.Assert(PeekToken(TokenKind.Assign)) at the top of the method?
|
@hackf5 Any other plans for this PR? Otherwise I'm happy with it and will merge it in. |
|
Thanks for the PR! |
|
You're welcome!
…On Mon, Nov 25, 2019 at 1:06 AM slozier ***@***.***> wrote:
Thanks for the PR!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#662?email_source=notifications&email_token=AJK7HWNDKIUANMEG5FYAWHTQVLJSFA5CNFSM4JFEGEOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFATBIA#issuecomment-557920416>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJK7HWILXHNVDHU7QEYF63DQVLJSFANCNFSM4JFEGEOA>
.
|
3 participants
Resolves #487
as mentioned in the chat I've tried to minimize the changes, however I chose to make a deoptimization in FinishAssignments so that it always allocates a list at the start. imo the improved readability and reduction in cyclomatic complexity outweigh the cost of the allocation. see what you think.
probably won't be able to make any updates for the next week and a half due to other commitments.