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

Align spinoso-string#make_binary to be more alike the String#b #1824

Merged
merged 4 commits into from
Apr 7, 2022

Conversation

b-n
Copy link
Member

@b-n b-n commented Apr 6, 2022

Fixes #1720

As talked about in the issue, the implementation in spinoso-string can be a little confusing when looking at how it is implemented in artichoke-backend. Specifically, spinoso-string is changing the encoding, however String#b is actually cloning the String before actually changing, so we might as well just do this directly and return a new value.

Copy link
Member

@lopopolo lopopolo left a comment

Choose a reason for hiding this comment

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

Looks good, I'd love to see one additional assert in the doc tests now that we're cloning the bytes

spinoso-string/src/lib.rs Show resolved Hide resolved
@b-n
Copy link
Member Author

b-n commented Apr 6, 2022

@lopopolo I believe it's unrelated to this specific change, but am I correct in assuming we can't actually check anything with relation to encoding on the String ruby class? https://github.com/artichoke/artichoke/blob/trunk/artichoke-backend/src/extn/core/string/string.rb#L514

Specifically, trying to test this above change like so:

artichoke 0.1.0-pre.0 (2022-04-02 revision 5484) [x86_64-unknown-linux-gnu]
[rustc 1.58.1 (db9d1b20b 2022-01-20) on x86_64-unknown-linux-gnu]
>>> "a".b.encoding
=> #<Encoding:UTF-8>

^-- not what I was expecting, and dug into String#encode and found that statement.

@lopopolo
Copy link
Member

lopopolo commented Apr 6, 2022

Yea Encoding as a Ruby object isn't exposed from Artichoke right now. The comments in the Ruby stubs appear to be outdated tho.

Artichoke does have an Encoding class, but it is essentially a stub to get Ruby spec to run. It so not wired up to the String internals.

I should file some tickets.

@b-n b-n marked this pull request as ready for review April 6, 2022 19:15
@b-n b-n requested a review from lopopolo April 6, 2022 19:16
@b-n
Copy link
Member Author

b-n commented Apr 6, 2022

Would you believe that Documentation Workflow error run can't be reproduced locally?

➜  artichoke git:(make-binary-to-binary) rustup -Vv    
rustup 1.24.3 (ce5817a94 2021-05-31)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.58.1 (db9d1b20b 2022-01-20)`
➜  artichoke git:(make-binary-to-binary) rustc -Vv 
rustc 1.58.1 (db9d1b20b 2022-01-20)
binary: rustc
commit-hash: db9d1b20bba1968c1ec1fc49616d4742c1725b4b
commit-date: 2022-01-20
host: x86_64-unknown-linux-gnu
release: 1.58.1
LLVM version: 13.0.0
➜  artichoke git:(make-binary-to-binary) cargo version --verbose       
cargo 1.58.0 (f01b232bc 2022-01-19)
release: 1.58.0
commit-hash: f01b232bc7f4d94f0c4603930a5a96277715eb8c
commit-date: 2022-01-19
host: x86_64-unknown-linux-gnu
libgit2: 1.3.0 (sys:0.13.23 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:OpenSSL/1.1.1l)
os: Ubuntu 20.04 (focal) [64-bit]

And cargo +nightly doc --workspace appears to "just work". I see you had some failure of this recently, any tips?

@lopopolo
Copy link
Member

lopopolo commented Apr 6, 2022

Oh sorry I ran into this in #1753 too. The current nightly rust doc has an ICE in spinoso-array. It's fixed in master. I'll run the workflow again tonight when the next nightly is out.

rust-lang/rust#95712 (comment)

@lopopolo
Copy link
Member

lopopolo commented Apr 6, 2022

And cargo +nightly doc --workspace appears to "just work". I see you had some failure of this recently, any tips?

In case you are curious about this and how Artichoke uses nightly Rust, this command uses whatever version of nightly you happen to have installed. The breakage is only on the current nightly though, which you can get by running rustup update nightly.

rake doc and rake fmt:rust will automatically pull down a nightly if you don't have one, but they won't update it.

rake doc uses nightly rust because docs.rs renders docs with nightly and Artichoke makes use of some docsrs attributes which are only available on nightly like feature callouts:

// Enable feature callouts in generated documentation:
// https://doc.rust-lang.org/beta/unstable-book/language-features/doc-cfg.html
//
// This approach is borrowed from tokio.
#![cfg_attr(docsrs, feature(doc_cfg))]
#![cfg_attr(docsrs, feature(doc_alias))]

rake fmt:rust uses nightly because this rustdoc config, which does intellij style import grouping, is nightly only:

artichoke/rustfmt.toml

Lines 14 to 15 in 19ff1ef

# nightly only option
group_imports = "StdExternalCrate"

In CI, Artichoke uses nightly for the fuzz workflows because:

  • cargo-fuzz requires nightly.
  • Using sanitizers like LeakSan requires passing an unstable -Z sanitizer=leak flag to rustc.

In CI, Artichoke uses nightly for the rustdoc workflow and for cargo fmt for the same reasons the Rakefile does.

spinoso-string/src/lib.rs Outdated Show resolved Hide resolved
@lopopolo
Copy link
Member

lopopolo commented Apr 7, 2022

new nightly is out, I rebuilt the docs workflow and build is green. we're ready to merge

@lopopolo lopopolo merged commit 987e378 into artichoke:trunk Apr 7, 2022
@lopopolo lopopolo added A-ruby-core Area: Ruby Core types. A-spec Area: ruby/spec infrastructure and completeness. labels Apr 7, 2022
@b-n b-n deleted the make-binary-to-binary branch April 8, 2022 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ruby-core Area: Ruby Core types. A-spec Area: ruby/spec infrastructure and completeness.
Development

Successfully merging this pull request may close these issues.

String::make_binary should clone instead of mutate; rename to String::to_binary
2 participants