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

Fix #4564: indent closes implicit object #4570

Merged

Conversation

helixbass
Copy link
Collaborator

Fixes #4564

The breaking example from that issue was:

for key of fn k: v
  a

The general case is when an implicit object is followed by an indent, eg this also breaks:

if k: v
  1

The solution was to make implicit objects act more like implicit calls as far as how they behave when they see an INDENT. Basically the exception to them behaving the same is when the INDENT follows the : of an object key (at which point clearly an INDENT should not close the implicit object). Otherwise they should behave the same and this allows us to close multiple open implicit calls and/or implicit objects when we see an INDENT (like in the first example)

To make implicit objects act more like implicit calls, we need to push CONTROLs (ie suppressors of INDENT-closes-implicits behavior) onto the stack not just when we're (immediately) inside an implicit call (ie inImplicitCall()) but also when we're (immediately) inside an implicit object (ie inImplicitObject())

This led to a slightly cleaner solution to handle things like a: for x in y then x than what @xixixao did in #3328. Basically if we see a FOR right after a :, we explicitly push a CONTROL (rather than having a special case represented by @insideForDeclaration that thwarts attempted endImplicitObject()s, that's what CONTROL is there to do). So I was able to get rid of all the @insideForDeclaration stuff from that PR

@GeoffreyBooth if this is deemed potentially-breaking I can retarget it to 2

@GeoffreyBooth
Copy link
Collaborator

After #4568 I’m wondering if all fixes like this should just go on 2. I don’t want to keep getting surprised by unexpected breaking changes.

@GeoffreyBooth
Copy link
Collaborator

Assuming all the tests still pass, this looks good to me. I think we should probably target 2 though. I think we should put a stop to bugfixes for 1.x unless the bug in question is really egregious.

@helixbass helixbass changed the base branch from master to 2 June 13, 2017 21:20
@helixbass
Copy link
Collaborator Author

@GeoffreyBooth sure, retargeted to 2

@xixixao
Copy link
Contributor

xixixao commented Jun 14, 2017

But this is a pretty bad bug in 1 no?

@GeoffreyBooth
Copy link
Collaborator

But this is a pretty bad bug in 1 no?

If it throws a compiler error, then by definition it isn’t. No one is relying on this bug for their current output, so there’s no 1.x code that needs this fix.

@GeoffreyBooth GeoffreyBooth requested a review from lydell June 14, 2017 17:11
Copy link
Collaborator

@lydell lydell left a comment

Choose a reason for hiding this comment

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

Nothing stands out in the code. If the tests passes and @helixbass has done this then I guess it’s fine.

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.

4 participants