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

Accept all 3.3.x and 3.4.x Ruby versions for Prism.parse #3077

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

eregon
Copy link
Member

@eregon eregon commented Sep 17, 2024

As discussed with @mame

@eregon eregon force-pushed the accept-all-3.3-and-3.4-versions branch 2 times, most recently from a483c58 to b0914c2 Compare September 17, 2024 21:06
@eregon
Copy link
Member Author

eregon commented Sep 18, 2024

Mmh, assert_raise ArgumentError.new("invalid version: latest2") do is not handled when tests are run within CRuby: https://github.com/ruby/prism/actions/runs/10911102608/job/30283047716?pr=3077

    1) Error:
  Prism::VersionTest#test_syntax_versions:
  Test::Unit::ProxyError: class or module required
      /home/runner/work/prism/prism/ruby/ruby/test/prism/version_test.rb:24:in 'Prism::VersionTest#test_syntax_versions'

Unfortunate, but should be easy to work around. Maybe a bug in CRuby's test unit harness since it's incompatible with the test-unit gem there?

@eregon eregon force-pushed the accept-all-3.3-and-3.4-versions branch from b0914c2 to b372540 Compare September 18, 2024 10:59
Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

I'm reluctant to accept all 3.3.x patterns as opposed to having a specified list. It seems odd to me that we would accept 3.3.a. This also doesn't allow for the possibility of changes within patch versions, which has happened in the past. I think I would prefer to check each version that is supposed explicitly.

@eregon
Copy link
Member Author

eregon commented Sep 18, 2024

It seems odd to me that we would accept 3.3.a.

I could validate that it's a number after the . I suppose, right.

This also doesn't allow for the possibility of changes within patch versions, which has happened in the past.

It does, nothing prevents to change this code and handle an hypothetical 3.3.7 which would have some difference in syntax. Of course one would need to update Prism to get that different behavior, but one would need to update Prism anyway in such a rare case.

It's also extremely unlikely to have syntax changes within patch versions. Even more so for 3.3 and 3.4.
If they are they would most likely be bug fixes, and then that wouldn't be so different than a bug fix in Prism.

I think I would prefer to check each version that is supposed explicitly.

But then that will always be outdated for every CRuby release that comes out for some time (until the next Prism release, and force people to update Ruby & Prism at the same time), isn't it?
That doesn't seem practical at all.
For example when 3.3.5 will come out, it's extremely likely there are no syntax changes, but if we have an explicit list then there won't be a Prism release which accepts that version, and so anything using Prism.parse(version: RUBY_VERSION) would stop working if people specify e.g. - uses: setup-ruby: with: ruby-version: 3.3.
So for example if IRB used Prism it would stop working in the irb shipped with Ruby in the release, or fail just when bumping the version (annoying CRuby release maintainers), not a good solution/experience, right?

Let me know, but I think we should merge this in the interest of users and to make Prism.parse(version: RUBY_VERSION) work.

@kddnewton
Copy link
Collaborator

Okay, I can go with that logic. I think I would still like the validation that for 3.3.x that x is a number. Could you add that? Then I think this is good to merge.

* Otherwise it is invalid e.g. to call strlen() to the result,
  or to assume the argument was a string.
* All callers are already checking for nil before.
@eregon eregon force-pushed the accept-all-3.3-and-3.4-versions branch from b372540 to e00d23a Compare September 20, 2024 20:35
@eregon eregon force-pushed the accept-all-3.3-and-3.4-versions branch from e00d23a to a4fcd53 Compare September 20, 2024 20:36
@eregon
Copy link
Member Author

eregon commented Sep 20, 2024

Done!

*/
static const char *
check_string(VALUE value) {
// If the value is nil, then we don't need to do anything.
if (NIL_P(value)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious why this line is going away?

Copy link
Member Author

@eregon eregon Sep 24, 2024

Choose a reason for hiding this comment

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

Because all callers are already checking for nil before, and returning NULL is dangerous e.g. if later strlen() or so is called on it, or if RSTRING_LEN(ruby_str) is used after check_string(ruby_str) (the concrete case here).
It's explained in the commit message of 8197be8

Comment on lines +33 to +36
# Not supported version syntax
assert_raise ArgumentError do
Prism.parse("1 + 1", version: "3.3")
end
Copy link
Member

Choose a reason for hiding this comment

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

Why not supported?
Won't 3.3 be expected as the latest of 3.3.x, or are you considering syntax changes across teeny versions?
IIRC, we have no such syntax changes (probably except for critical bugs, and in these years at least).

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant mostly "not supported at the moment/yet", only x.y.z for now.

are you considering syntax changes across teeny versions?

Yes, at least the intention for now is to try to be careful about that.
So Prism matches exactly what the CRuby compiler accepts when using Prism.parse(code, version: RUBY_VERSION), no more, no less.

IIRC there were a couple syntax changes in teeny versions a while ago.
OTOH https://github.com/whitequark/parser/tree/master/lib/parser only has x.y versions, so probably those are small changes.
I am not sure whether such changes can matter or not in practice, I suppose it depends on the tool.
For IRB for example it seems best to be exactly the same including "bugs" so IRB and ruby -e are equivalent.

I'd be OK to treat 3.3 as 3.3.LAST or so, but it feels better to do separately and it needs @kddnewton's opinion on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I can support that, but let's do it in a follow-up PR.

@kddnewton kddnewton merged commit 00ec2bf into ruby:main Sep 24, 2024
55 checks passed
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