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

Bug: parallel assignment with a side effect. #2344

Closed
wants to merge 1 commit into from

Conversation

dblock
Copy link
Contributor

@dblock dblock commented Oct 25, 2015

Let me demonstrate:

type = [Hash]
type, value_type = type.class, type.first
=> [Array, Hash] 

after auto-correct

type = [Hash]
type = type.class
value_type = type.first
=> Undefined method `first` for Array:Class

I think a generic fix just changes the order of assignment to do right-to-left, but that will seem odd most of the time.

@dblock dblock force-pushed the parallel-assignment-order branch from ea248b4 to db30393 Compare October 25, 2015 19:44
@jonas054
Copy link
Collaborator

A generic change of order will not be enough. The problem is that parrallel assignment evaluates all expressions on the right hand side before doing all the assignments. So with right-to-left order, you'd still have the same problem with

type = [Hash]
value_type, type = type.first, type.class

We need to check if any variable used on the left hand side appears on the right hand side. If it does, we can either skip auto-correction (easy way) or change the order (hard way).

@dblock
Copy link
Contributor Author

dblock commented Oct 26, 2015

Oh, interesting, how would you figure out the right order without temporary variables every time?

I think skipping auto-correction is more viable.

@jonas054
Copy link
Collaborator

I agree. There are probably cases when there is no correct order without introducing temporary variables.

@alexdowad
Copy link
Contributor

There are definitely cases where there is no correct order. Here's one:

a, b = b, a

@alexdowad
Copy link
Contributor

Aha, I see that ParallelAssignment doesn't try to correct in that case.

@alexdowad
Copy link
Contributor

Just opened a PR which still needs a little work, but (when finished) will provide a robust fix to this problem.

Oh, interesting, how would you figure out the right order without temporary variables every time?

You can figure out the right order using a topological sort.

@bbatsov bbatsov closed this in 0b03365 Nov 4, 2015
bbatsov added a commit that referenced this pull request Nov 4, 2015
[Fix #2344] When correcting parallel assignment, reorder lines to avoid breaking code
dblock referenced this pull request in dblock/virtus Nov 17, 2015
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.

3 participants