IndexTargetNode should always have ATTRIBUTE_WRITE #2118
Conversation
|
@kddnewton I only wonder if you should not use LATEST and just speculatively assume 3.4? It is possible MRI will pick another version number and 3.4 is not out for a year but it seems weird from a process perspective to mark LATEST on 3.4 syntax features. When 3.4 does come out you will have to global replace those to 3.4 and then use LATEST for 3.5. |
Because this is a user-facing change, we also need to deal with the fact that CRuby 3.3.0 was just released. In order to support workflows that want to parse exactly as CRuby parses in a specific version, this PR introduces a new option to the options struct that is "version". This allows you to specify that you want "3.3.0" parsing. I'm not sure if this is the correct solution. Another solution is to just fork and keep around the old branch for security patches. Or we could keep around a copy of the source files within this repository as another directory and only update when necessary. There are a lot of potential solutions here. Because this change is so small and the check for it is so minimal, I've decided to go with this enum. If this ends up entirely cluttering the codebase with version checks, we'll come up with another solution. But for now this works, so we're going to go in this direction for a bit until we determine it's no longer working.
kddnewton
force-pushed
the
index-target-node-attr-write
branch
from
8c3ee60 to
d8c7e6b
Compare
|
@enebo yeah great point. I just updated it |
| if (parser->version != PM_OPTIONS_VERSION_CRUBY_3_3_0) { | ||
| flags |= PM_CALL_NODE_FLAGS_ATTRIBUTE_WRITE; | ||
| } |
There was a problem hiding this comment.
@andrykonchin and I are looking at which version we'd set.
We do want the flag to be set here.
But with the current code that means we'd need to use PM_OPTIONS_VERSION_LATEST.
OTOH we do not want it handling from 3.4 (#2124).
And using PM_OPTIONS_VERSION_LATEST would enable it.
So how to solve this?
Maybe we need PM_OPTIONS_VERSION_CRUBY_3_3 (without _0) to mean 3.3 syntax with the latest fixes?
Alternatively, is it worth hiding this flag for Prism versions newer than the one shipped with 3.3.0? Maybe not?
It feels like this flag should not hurt anything, and if someone wants exactly the version of Prism as shipped in 3.3.0 then they can just specify that Prism release in their Gemfile.
In short adding PM_CALL_NODE_FLAGS_ATTRIBUTE_WRITE is not really a syntax change or different parsing or anything, it's just extra information or from a POV a bug fix for a missing flag.
There was a problem hiding this comment.
@kddnewton WDYT?
Feel free to extract this to an issue/discussion if you prefer to discuss it somewhere else.
EDIT: I moved it to #2163
There was a problem hiding this comment.
Maybe the fundamental question is whether this change would go in CRuby 3.3.1. I think it should as a bug fix.
Fixes #2102
Because this is a user-facing change, we also need to deal with the fact that CRuby 3.3.0 was just released.
In order to support workflows that want to parse exactly as CRuby parses in a specific version, this PR introduces a new option to the options struct that is "version". This allows you to specify that you want "3.3.0" parsing.
I'm not sure if this is the correct solution. Another solution is to just fork and keep around the old branch for security patches. Or we could keep around a copy of the source files within this repository as another directory and only update when necessary. There are a lot of potential solutions here.
Because this change is so small and the check for it is so minimal, I've decided to go with this enum. If this ends up entirely cluttering the codebase with version checks, we'll come up with another solution. But for now this works, so we're going to go in this direction for a bit until we determine it's no longer working.