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 21590 - assignment inside assert accepted for -checkaction=context #12165
Conversation
Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "stable + dmd#12165" |
c1376e0
to
ec55ef7
Compare
This doesn't detect other kind of assignments, such as |
ec55ef7
to
1261bcf
Compare
Fixed. But there is no error for those expressions at the moment (regardless of |
1261bcf
to
125736b
Compare
Copy |
It does, but it also breaks a lot of code. Boolean conversion for assignments is rejected because it's most likely a typo ( |
125736b
to
8946b1d
Compare
The rewrite introduced a temporary which hid the assignment inside of `assert(...)` and hence prevented the error. The fix is to omit the additional temporary for `AssignExp` and use the assigned variable directly. The temporary is unecessary anyways and the following semantic analysis will raise an appropriate error.
8946b1d
to
f9dc70c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The rewrite introduced a temporary which hid the assignment inside of
assert(...)
and hence prevented the error.The fix is to omit the additional temporary for
(Bin)AssignExp
and usethe assigned variable directly. The temporary is unecessary anyways
and the following semantic analysis will raise an appropriate error.