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: crystal next compatibility #407

Merged
merged 1 commit into from
Oct 9, 2023
Merged

fix: crystal next compatibility #407

merged 1 commit into from
Oct 9, 2023

Conversation

veelenga
Copy link
Member

@veelenga veelenga commented Oct 6, 2023

@veelenga veelenga requested a review from Sija October 6, 2023 15:52
@veelenga veelenga marked this pull request as draft October 6, 2023 15:52
@veelenga veelenga marked this pull request as ready for review October 6, 2023 16:04
Copy link
Member

@Sija Sija left a comment

Choose a reason for hiding this comment

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

Lint/UselessAssign rule is using transformed?, just fyi

@veelenga
Copy link
Member Author

veelenga commented Oct 7, 2023

Lint/UselessAssign rule is using transformed?, just fyi

Exactly, I left it for BC. It is not needed anymore for Crystal master and above.

@veelenga veelenga requested a review from Sija October 7, 2023 18:49
@Sija
Copy link
Member

Sija commented Oct 7, 2023

@veelenga Could you please explain what do you mean by BC in this particular case? And why it's not longer needed?

@veelenga
Copy link
Member Author

veelenga commented Oct 9, 2023

@Sija, sure.

In this PR, we've introduced support for Crystal's master version and subsequent versions. The term "BC" refers to the backward compatibility of Crystal (1.9.x) and earlier versions.

We implemented the transformed? method in the past to determine if a node represents an assignment transformed by a compiler. To illustrate, in Crystal 1.9.x, the following code:

collection.each do |(a, b)|
  puts b
end

is transformed before the AST phase to:

collection.each do |__arg0|
  a = __arg0[0]
  b = __arg0[1]
  puts(b)
end

This transformation leads to false positives since the assignment to the variable a becomes redundant. So we had to handle that case "manually" via #transformed? method.

However, in the Crystal master, the block mentioned above remains unaltered, and the resulting AST mirrors the original code:

collection.each do |(a, b)|
  puts(b)
end

Consequently, for the Crystal master and later versions, the transformed? method becomes unnecessary. However, I suggest retaining this method until the next release for the sake of backward compatibility. This ensures that users using both Ameba's master and Crystal 1.9.x will still not encounter these false positives.

@Sija
Copy link
Member

Sija commented Oct 9, 2023

@veelenga Thanks for the thorough explanation ❤️ It all makes sense (re-reading referenced PR helped too 😅), let's have this shipped then! :shipit:

@Sija Sija added this to the next milestone Oct 9, 2023
@Sija Sija merged commit c953822 into master Oct 9, 2023
4 checks passed
@Sija Sija deleted the crystal-next-compatibility branch October 9, 2023 21:47
@Sija Sija modified the milestones: next, 1.6.0 Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing spec on crystal master
2 participants