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

[Compatibility] Add alias String#dedup to String#-@ for Ruby v3.2 #3042

Merged
merged 3 commits into from
May 23, 2023

Conversation

itarato
Copy link
Collaborator

@itarato itarato commented May 9, 2023

Source: #3039

String#dedup has been added as an alias to String#-@. [Feature #18595]

@itarato itarato self-assigned this May 9, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label May 9, 2023
@itarato itarato added shopify OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Verified All contributors have signed the Oracle Contributor Agreement. labels May 9, 2023
@itarato itarato force-pushed the feature/PA-3039-string-dedup branch 2 times, most recently from 824ddfb to 5ccda89 Compare May 10, 2023 13:47
@itarato itarato marked this pull request as ready for review May 12, 2023 12:39
@andrykonchin
Copy link
Member

suggestion: String#-@ is handled in a special way at parsing time. It's inlined when is called on a String literal to avoid method call. It makes sense to treat the #dedup method in the same way.

Inlining is implemented here:

if (receiver instanceof StrParseNode &&
(methodName.equals("freeze") || methodName.equals("-@"))) {
final StrParseNode strNode = (StrParseNode) receiver;
final TruffleString tstring = strNode.getValue();
final ImmutableRubyString frozenString = language.getFrozenStringLiteral(tstring, strNode.encoding);
return addNewlineIfNeeded(node, withSourceSection(
sourceSection,
new FrozenStringLiteralNode(frozenString, FrozenStrings.METHOD)));
}

@itarato itarato force-pushed the feature/PA-3039-string-dedup branch 2 times, most recently from be0e5c4 to 2463891 Compare May 18, 2023 13:16
@itarato
Copy link
Collaborator Author

itarato commented May 18, 2023

suggestion: String#-@ is handled in a special way at parsing time. It's inlined when is called on a String literal to avoid method call. It makes sense to treat the #dedup method in the same way.

Inlining is implemented here:

if (receiver instanceof StrParseNode &&
(methodName.equals("freeze") || methodName.equals("-@"))) {
final StrParseNode strNode = (StrParseNode) receiver;
final TruffleString tstring = strNode.getValue();
final ImmutableRubyString frozenString = language.getFrozenStringLiteral(tstring, strNode.encoding);
return addNewlineIfNeeded(node, withSourceSection(
sourceSection,
new FrozenStringLiteralNode(frozenString, FrozenStrings.METHOD)));
}

Thanks for pointing this out. Added dedup there.

@itarato itarato requested a review from andrykonchin May 18, 2023 13:18
Copy link
Member

@andrykonchin andrykonchin left a comment

Choose a reason for hiding this comment

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

Thank you!

@andrykonchin
Copy link
Member

andrykonchin commented May 18, 2023

After inlining one test case fails:

1)
String#dedup interns the provided string if it is frozen FAILED
Expected "this string is unique and frozen 0.452[28](https://github.com/oracle/truffleruby/actions/runs/5014365700/jobs/8988815849?pr=3042#step:8:29)764636217345"
to be identical to "this string is unique and frozen 0.45228764636217345"
/home/runner/work/truffleruby/truffleruby/spec/ruby/core/string/shared/dedup.rb:54:in `block (3 levels) in <top (required)>'
/home/runner/work/truffleruby/truffleruby/spec/ruby/core/string/dedup_spec.rb:4:in `<top (required)>'

Right now it fails for String#-@ as well and is excluded (here spec/tags/core/string/uminus_tags.txt). Probably we should exclude it for dedup as well. You can use a command jt tag <path-to-spec-file> locally to create a similar tag-file for the String#dedup method.

@itarato itarato force-pushed the feature/PA-3039-string-dedup branch from 2463891 to 25dd774 Compare May 18, 2023 15:43
@itarato
Copy link
Collaborator Author

itarato commented May 18, 2023

After inlining one test case fails:

1)
String#dedup interns the provided string if it is frozen FAILED
Expected "this string is unique and frozen 0.452[28](https://github.com/oracle/truffleruby/actions/runs/5014365700/jobs/8988815849?pr=3042#step:8:29)764636217345"
to be identical to "this string is unique and frozen 0.45228764636217345"
/home/runner/work/truffleruby/truffleruby/spec/ruby/core/string/shared/dedup.rb:54:in `block (3 levels) in <top (required)>'
/home/runner/work/truffleruby/truffleruby/spec/ruby/core/string/dedup_spec.rb:4:in `<top (required)>'

Right now it fails for String#-@ as well and is excluded (here spec/tags/core/string/uminus_tags.txt). Probably we should exclude it for dedup as well. You can use a command jt tag <path-to-spec-file> locally to create a similar tag-file for the String#dedup method.

Thank you, just did that.

Copy link
Member

@andrykonchin andrykonchin left a comment

Choose a reason for hiding this comment

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

Thank you!

@andrykonchin andrykonchin added the in-ci The PR is being tested in CI. Do not push new commits. label May 19, 2023
@@ -31,6 +31,7 @@ codepoints
concat
count
crypt
dedup
Copy link
Member

Choose a reason for hiding this comment

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

FYI we shouldn't modify these files, they should reflect the MRI version we import.
But it's no big deal, let's leave as-is for this PR, it will overridden on 3.2 import.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is the right way to do this? Without this tests will complain that the method is not exposed?

Copy link
Member

@eregon eregon May 23, 2023

Choose a reason for hiding this comment

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

See https://github.com/oracle/truffleruby/blob/master/spec/truffle/methods_spec.rb, so you'd just use jt tag to mark as extra method, until the 3.2 import which would then regenerate those files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted, thanks!

@graalvmbot graalvmbot merged commit bfbc036 into oracle:master May 23, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement. shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants