Reject true && not true
#3585
Reject true && not true
#3585
Conversation
| true && not (true) | ||
| ^ expected a `(` after `not` | ||
| ^ unexpected '(', expecting end-of-input | ||
|
|
There was a problem hiding this comment.
I wanted to add a test to confirm that
true && not
true
be rejected, but I am not sure how to write the test.
There was a problem hiding this comment.
Perhaps we can write something like this in test/prism/errors/command_calls_31.txt:
true && not
true
^~~~ expected a `(` after `not`
^~~~ unexpected 'true', expecting end-of-input
|
|
||
| true && not (true) | ||
| ^ expected a `(` after `not` | ||
| ^ unexpected '(', expecting end-of-input |
There was a problem hiding this comment.
This error message is a bit weird, but I am not sure how to fix it.
|
This looks correct!! I can fix the error messages next Monday |
|
@kddnewton Just a gentle follow-up on the PR I sent. I believe this fix is somewhat time-sensitive. Since the bug can lead to invalid Ruby code being written, it would be great to get it resolved before it spreads further. Ideally, it could also be backported to Ruby 3.4.5. Please let me know if there's anything I can do to help move it forward. Thanks so much! |
|
@kddnewton Second follow-up on the PR. Would appreciate it if you could take a look when you have time. cc/ @tenderlove @eileencodes |
|
How about following changes? Perhaps the remaining concerns (#3585 (comment) and #3585 (comment)) have been addressed. |
|
I've added the above commits. If they don't match your intentions, please feel free to revert them without any concern. |
|
@kddnewton Please feel free to let me know if there is anything blocking the merge of this PR so I can help you. |
|
@kddnewton @tenderlove @eileencodes Hey, we really need to get this merged soon. I’d like to have it backported to the upcoming Ruby 3.4 series. Let me repeat: this is a very serious bug. It allows invalid code to be parsed, and once people start writing such code, fixing it becomes much harder later on. Also, I’m increasingly concerned that bug reports for Prism are being left unaddressed. If maintenance doesn't improve, I think we'll seriously need to switching the default parser back to parse.y. cc/ @paracycle |
|
@kddnewton @tenderlove @eileencodes @paracycle Ruby 3.4.5 has already been released. That's very unfortunate. This reinforces my growing concern that Prism is effectively unmaintained. If this isn't resolved in time for Ruby 3.4.6, or if any other serious signs of it being unmaintained appear, I'll propose removing Prism from the Ruby interpreter. |
https://bugs.ruby-lang.org/issues/21337 Early backport of ruby/prism#3585 due to its bug severity, but missed 3.4.5 release.
A command-call-like `not true` must be rejected after `&&` and `||`. https://bugs.ruby-lang.org/issues/21337 ---- Early backport of ruby/prism#3585 due to its bug severity, but missed 3.4.5 release.
I think that's an exaggeration based on this specific case, e.g., 22 open 1007 closed at https://github.com/ruby/prism/issues is what I would call very well maintained. I think it would be good to clarify in which situations it's OK for CRuby committers to merge changes to Prism themselves. |
|
Well, I myself have two PRs that attempt to fix some sort of bug with prism:
They have been open without any feedback for a few months now unfortunatly. I'm not a C dev and also not experienced with parsers in general but wanted to help out a bit. Previously I focused on the translation layer and the time to merge there has been phenomenal. @kddnewton github says he is on paternity leave, so I do not expect immediate action from him in any way. The PRs from me also do not address anything serious, so personally those are not a big deal to me. It was just nice to do something a bit different for a change. What I do want to say in regards to the maintenance though is in regards to this:
I think it would be incredibly unfortunate to lose prism. It has seen widespread adoption in the ecosystem and other parsers that used to be around have stopped development, like the parser gem. If there is no adequate replacement, this leaves many projects in an awkward spot. I can't tell about alternative ruby implementations like truffleruby/jruby/natalie, but I presume they don't carry around their old parser implementation anymore now that they use prism too. Projects like rails/ruby-lsp/solargraph/rubocop and others also adapted prism so I would say prism has been a huge success thus far. I would like to see anything to keep prism going, and more direct involvement from other cruby devs would certainly help with that. |
|
Sorry friends, still getting the hang of 3 kids while on leave here. I will get this reviewed now. |
|
Please don't yank prism because I went on paternity leave. It is still maintained, I promise. |
A command-call-like `not true` must be rejected after `&&` and `||`. https://bugs.ruby-lang.org/issues/21337
kddnewton
force-pushed
the
no-command-call-not-after-ampersand-ampersand
branch
from
5da77ea to
d9151b8
Compare
|
First of all, thank you for merging it! I'm aware that @kddnewton is currently on paternity leave and is no longer maintaining Prism full-time. It is a fact that several Prism-related bugs, including a serious one like heap-buffer-overflow, have been left unaddressed for some time, and I am growing increasingly concerned about this trend. https://bugs.ruby-lang.org/issues/21461 Unfortunately, there are several components in Ruby that are not actively maintained, but the parser, arguably one of the most critical parts of a language implementation, should not be one of them. One of Prism's original goal was to provide a more maintainable alternative to parse.y. The code itself of parse.y is never clean, but it has been actively maintained for decades (largely thanks to @nobu). |
A command-call-like
not truemust be rejected after&&and||.https://bugs.ruby-lang.org/issues/21337