paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.com/
1.94k stars 710 forks source link

Remove `sp_std` #2101

Open koute opened 1 year ago

koute commented 1 year ago

Historically having the sp_std crate might have had benefits, but nowadays I think there isn't much point in keeping it as the only thing it's doing is reexporting a few thing from core/alloc and being confusing to newcomers. We can get rid of it and just use core and alloc directly like everyone else in the Rust ecosystem.

For example:

-use sp_std::{marker::PhantomData, vec, vec::Vec};
+extern crate alloc;
+use core::marker::PhantomData;
+use alloc::{vec, vec::Vec};

This works in both std and no_std contexts.

koute commented 1 year ago

And just for reference, there are currently 1525 sp_std references in the whole repository. Here's a hacky (and untested!) script that I quickly whipped up in 10 minutes which cuts down 1525 sp_std references into 397, where 328 of the remaining ones are sp_std::prelude::* imports, so they probably can also be easily automated away:

#!/usr/bin/ruby

require 'json'
require 'set'

ALLOC = Set.new [
    "borrow",
    "boxed",
    "collections",
    "format!",
    "rc",
    "vec",
    "vec!"
]

CORE = Set.new [
    "any",
    "cell",
    "cmp",
    "convert",
    "default",
    "fmt",
    "hash",
    "iter",
    "marker",
    "mem",
    "num",
    "ops",
    "result",
    "slice",
    "str",
    "sync",
    "time"
]

def classify mod
    if CORE.include? mod
       "core"
    elsif ALLOC.include? mod
        "alloc"
    else
        "sp_std"
    end
end

paths = `rg -t rust sp_std --json`.each_line.map { |line| (JSON.parse(line)["data"]["path"] || {})["text"] }.select { |p| p != nil }.sort.uniq
paths.each do |path|
    old_data = File.read(path)
    new_data = old_data.gsub(/sp_std::([a-z_\!]+)(::|;|\[)/) do
        "#{classify $1}::#{$1}#{$2}"
    end.gsub(/use sp_std::\{([^\};]+)\};/) do
        per_crate = {}
        $1.strip.split(",").each do |path|
            crate = classify(path.strip.split("::")[0])
            per_crate[crate] ||= []
            per_crate[crate] << path.strip
        end
        out = ""
        per_crate.each do |crate, list|
            if list.length == 1
                out << "use #{crate}::#{list[0]};\n"
            else
                out << "use #{crate}::\{#{list.join(", ")}\};\n"
            end
        end
        out.strip
    end

    next if old_data == new_data
    File.write(path, new_data)
end
davxy commented 1 year ago

Then we need to do this explicit extern crate alloc in every primitive crate which uses Vec (maybe all)?

I don't have a strong opinion, but maybe the sp-std facade is a bit less verbose and convenient.

EDIT: just for reference. We are not alone https://github.com/arkworks-rs/std

burdges commented 1 year ago

I've no opinion here but..

Arkworks' ark-std exists largely to provide core::io, which their serialization uses. I'd kinda expect rust gains core::io eventually, but afaik no real progress exists towards this yet. Arguably, scale should be similarly designed around core::io, not its own Input/Output traits, although one difference exists in error propagation, which made sense back when native runtimes existed.

kianenigma commented 10 months ago

Seeing people have worked on this I would move forward and assist with merging them, but in general have mixed feeling about whether this is easier for esp. Junior Rust devs.

athei commented 9 months ago

I am in favour of that. One less special thing that people need to wrap their head around when using the substrate code base. To me this crate was always an annoyance.