Dogfood `str_split_once()` #79809

Eric-Arellano · 2020-12-07T21:31:42Z

Part of #74773.

Beyond increased clarity, this fixes some instances of a common confusion with how splitn(2) behaves: the first element will always be Some(), regardless of the delimiter, and even if the value is empty.

Given this code:

fn main() {
    let val = "...";
    let mut iter = val.splitn(2, '=');
    println!("Input: {:?}, first: {:?}, second: {:?}", val, iter.next(), iter.next());
}

We get:

Input: "no_delimiter", first: Some("no_delimiter"), second: None
Input: "k=v", first: Some("k"), second: Some("v")
Input: "=", first: Some(""), second: Some("")

Using str_split_once() makes more clear what happens when the delimiter is not found.

rust-highfive · 2020-12-07T21:31:45Z

Some changes occurred in intra-doc-links.

cc @jyn514

rust-highfive · 2020-12-07T21:31:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Eric-Arellano · 2020-12-07T21:34:36Z

compiler/rustc_mir/src/transform/coverage/debug.rs

@@ -148,23 +148,19 @@ impl DebugOptions {

        if let Ok(env_debug_options) = std::env::var(RUSTC_COVERAGE_DEBUG_OPTIONS) {
            for setting_str in env_debug_options.replace(" ", "").replace("-", "_").split(',') {
-                let mut setting = setting_str.splitn(2, '=');
-                match setting.next() {


The original match was considering more than necessary. setting.next() was always Some(). Only the value could be None.

I kept value modeled as an Option to avoid needing to change the related code like bool_option_val.

Eric-Arellano · 2020-12-07T21:35:51Z

compiler/rustc_session/src/config.rs

-        let mut parts = arg.splitn(2, '=');
-        let name = parts
-            .next()
-            .unwrap_or_else(|| early_error(error_format, "--extern value must not be empty"));


This case could never happen.

Eric-Arellano · 2020-12-07T21:36:39Z

compiler/rustc_session/src/config.rs

-        let (options, name) = match (first_part, second_part) {
-            (Some(opts), Some(name)) => (Some(opts), name),
-            (Some(name), None) => (None, name),
-            (None, None) => early_error(error_format, "--extern name must not be empty"),


This case could never happen, the first value is always Some.

Eric-Arellano · 2020-12-07T21:38:22Z

library/test/src/time.rs

+        let (warn_str, critical_str) = durations_str.split_once(',').unwrap_or_else(|| {
+            panic!(
+                "Duration variable {} expected to have 2 numbers separated by comma, but got {}",
+                env_var_name, durations_str
+            )
+        });


Note that this inverts the order of validation: now we first validate that the split works, and only then validate that they can be parsed as numbers.

Eric-Arellano · 2020-12-07T21:39:40Z

src/tools/linkchecker/main.rs

+            Some((url, fragment)) => (url, Some(fragment)),
+        };
+        // NB: the `splitn` always succeeds, even if the delimiter is not present.
+        let url = url.splitn(2, '?').next().unwrap();


Because we discard the second element, it's simpler to still use splitn().

src/tools/tidy/src/error_codes_check.rs

src/librustdoc/passes/collect_intra_doc_links.rs

* Add assertion value is defined. * Simplify comment. * Fix bad change in err message.

compiler/rustc_mir/src/transform/coverage/debug.rs

matklad · 2020-12-08T10:17:56Z

compiler/rustc_session/src/config.rs

-        let mut parts = arg.splitn(2, '=');
-        let name = parts
-            .next()
-            .unwrap_or_else(|| early_error(error_format, "--extern value must not be empty"));


matklad · 2020-12-08T10:21:18Z

compiler/rustc_session/src/config.rs

-                             one of dylib, framework, or static",
-                            s
-                        ),
-                    );


Just writing my reasoning down for posterity:

The previous version is more compact, so I am almost inclined to say that it was better. However, the new version doesn't have .unwrap(), and, given a couple of instances where have impossible cases in other parts of this PR, I think a tad more verbosity is a fine price to pay for making illegal states unrepresentable here.

That said, would flattening the two matches into one work better here?

match s.split_once('=') { Some(("dylib", name)) => () }

?

Ack that this is less compact, but I refactored to use the nested match because it reduces duplication and imo makes this code easier to grok, albeit longer. We no longer have to put name in multiple arms of the match, like we had previously:

match s.split_once('=') { Some(("dylib", name)) => (), Some(("framework", name)) => (), ... }

Instead, what do you think about adding impl TryFrom<String> for NativeLibKind? I'm happy to add that (although I might recommend saving that for a followup to keep this PR more focused).

Instead, what do you think about adding impl TryFrom for NativeLibKind?

Not TryFrom, but impl FromStr for NativeLibKind would work nicely. But I think the added benefit here is marginal, if any (there's drawback of more code churn), so I suggest leaving the code as is then!

matklad · 2020-12-08T10:26:52Z

src/librustdoc/config.rs

@@ -707,11 +704,9 @@ fn parse_extern_html_roots(
 ) -> Result<BTreeMap<String, String>, &'static str> {
    let mut externs = BTreeMap::new();
    for arg in &matches.opt_strs("extern-html-root-url") {
-        let mut parts = arg.splitn(2, '=');
-        let name = parts.next().ok_or("--extern-html-root-url must not be empty")?;


matklad · 2020-12-08T10:28:24Z

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -435,8 +435,10 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {

        // Try looking for methods and associated items.
        let mut split = path_str.rsplitn(2, "::");


should we use rsplit_once here?

I wanted to, but had a hard time getting this code working properly. My WIP ended up being more confusing imo, so I gave up.

Yeah everything here is a mess. I tried to clean it up in #76467 but it ended up not working and getting stuck for 3 months :( so I reverted it.

matklad · 2020-12-08T10:31:57Z

src/tools/tidy/src/ui_tests.rs

-                        .next()
-                        .unwrap();
+                    let testname =
+                        file_path.file_name().unwrap().to_str().unwrap().split_once('.').unwrap().0;


I wonder if this is just file_path.file_stem() ?

I tried it now, and it breaks things. From lines 12 - 17, there are some files with multiple .s in the name, like $testname.$revision.$mode.stderr, and we need to strip those. I'll add a comment.

src/tools/tidy/src/error_codes_check.rs

lcnr · 2020-12-08T10:40:46Z

r=me

@matklad just wrote some review comments so I will leave the final approval to them
r? @matklad

* Use a match statement. * Clarify why we can't use `file_stem()`. * Error if the `:` is missing for Tidy error codes, rather than no-oping.

matklad · 2020-12-10T12:52:25Z

LGTM now!

@bors r+

matklad · 2020-12-10T12:53:14Z

@bors r+

bors · 2020-12-10T12:54:04Z

📌 Commit 989edf4 has been approved by matklad

Dogfood `str_split_once()` Part of rust-lang#74773. Beyond increased clarity, this fixes some instances of a common confusion with how `splitn(2)` behaves: the first element will always be `Some()`, regardless of the delimiter, and even if the value is empty. Given this code: ```rust fn main() { let val = "..."; let mut iter = val.splitn(2, '='); println!("Input: {:?}, first: {:?}, second: {:?}", val, iter.next(), iter.next()); } ``` We get: ``` Input: "no_delimiter", first: Some("no_delimiter"), second: None Input: "k=v", first: Some("k"), second: Some("v") Input: "=", first: Some(""), second: Some("") ``` Using `str_split_once()` makes more clear what happens when the delimiter is not found.

bors · 2020-12-11T05:20:48Z

⌛ Testing commit 989edf4 with merge 7414a221dbcad5b0f38a58956eab1374e5f3296f...

tmandry · 2020-12-11T05:34:01Z

Rolled up
@bors retry

Rollup of 11 pull requests Successful merges: - rust-lang#77027 (Improve documentation for `std::{f32,f64}::mul_add`) - rust-lang#79375 (Make the kernel_copy tests more robust/concurrent.) - rust-lang#79639 (Add long explanation for E0212) - rust-lang#79698 (Add tracking issue template for library features.) - rust-lang#79809 (Dogfood `str_split_once()`) - rust-lang#79851 (Clarify the 'default is only allowed on...' error) - rust-lang#79858 (Update const-fn doc in unstable-book) - rust-lang#79860 (Clarify that String::split_at takes a byte index.) - rust-lang#79871 (Fix small typo in `wrapping_shl` documentation) - rust-lang#79896 (Make search results tab and help button focusable with keyboard) - rust-lang#79917 (Use Symbol for inline asm register class names) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bors · 2020-12-11T07:54:26Z

⌛ Testing commit 989edf4 with merge 65d053a...

matklad · 2020-12-11T09:28:34Z

@Eric-Arellano do you want to file a stabilization PR as well (see the instructions in the tracking issue)?

I am not sure if we should let this bake more or not, but I think it's fine to raise the issue with the libs team already.

jyn514 · 2020-12-12T20:39:18Z

src/librustdoc/passes/collect_intra_doc_links.rs

-        let (item_str, item_name) = split.next().map(|i| (i, Symbol::intern(i))).unwrap();
+        // NB: `split`'s first element is always defined, even if the delimiter was not present.
+        let item_str = split.next().unwrap();
+        assert!(!item_str.is_empty());


This caused an assertion failure on ::some_crate, I forgot :: means the root namespace. Sending a revert PR now.

https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Perf.20failing

Eric-Arellano added 5 commits December 7, 2020 12:48

Dogfood 'str_split_once() with compiler/

12db222

Dogfood 'str_split_once() with Tidy

d6baf38

Dogfood 'str_split_once() with linkchecker

7bd47bd

Dogfood 'str_split_once() with librustdoc

85e9ea0

Dogfood 'str_split_once()` in the std lib

d2de69d

rust-highfive assigned lcnr Dec 7, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 7, 2020

Eric-Arellano commented Dec 7, 2020

View reviewed changes

jyn514 added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Dec 7, 2020

Eric-Arellano mentioned this pull request Dec 7, 2020

Tracking Issue for str_split_once #74773

Closed

4 tasks

jyn514 reviewed Dec 7, 2020

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved

jyn514 added T-libs Relevant to the library team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 7, 2020

Eric-Arellano added 2 commits December 7, 2020 18:47

Review feedback for collect_intra_doc_links.rs

f68cc68

* Add assertion value is defined. * Simplify comment. * Fix bad change in err message.

Fix net.rs - rsplitn() returns a reverse iterator

a3174de

Eric-Arellano force-pushed the split-once branch from c03a8de to a3174de Compare December 8, 2020 01:47

matklad reviewed Dec 8, 2020

View reviewed changes

rust-highfive assigned matklad and unassigned lcnr Dec 8, 2020

Review feedback

989edf4

* Use a match statement. * Clarify why we can't use `file_stem()`. * Error if the `:` is missing for Tidy error codes, rather than no-oping.

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2020

Dylan-DPC-zz mentioned this pull request Dec 10, 2020

Rollup of 14 pull requests #79920

Closed

tmandry mentioned this pull request Dec 11, 2020

Rollup of 11 pull requests #79927

Merged

bors merged commit 17ec4b8 into rust-lang:master Dec 11, 2020

rustbot added this to the 1.50.0 milestone Dec 11, 2020

Eric-Arellano deleted the split-once branch December 11, 2020 16:21

jyn514 mentioned this pull request Dec 12, 2020

Make --self-profile work for rustdoc rust-lang/rustc-perf#806

Merged

jyn514 reviewed Dec 12, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dogfood `str_split_once()` #79809

Dogfood `str_split_once()` #79809

Eric-Arellano commented Dec 7, 2020

rust-highfive commented Dec 7, 2020

rust-highfive commented Dec 7, 2020

Eric-Arellano Dec 7, 2020

Eric-Arellano Dec 7, 2020

matklad Dec 8, 2020

Eric-Arellano Dec 7, 2020

Eric-Arellano Dec 7, 2020

Eric-Arellano Dec 7, 2020

matklad Dec 8, 2020

matklad Dec 8, 2020 •

edited

Loading

Eric-Arellano Dec 8, 2020

matklad Dec 10, 2020

matklad Dec 8, 2020

matklad Dec 8, 2020

Eric-Arellano Dec 8, 2020

jyn514 Dec 12, 2020

matklad Dec 8, 2020

Eric-Arellano Dec 8, 2020

lcnr commented Dec 8, 2020

matklad commented Dec 10, 2020 •

edited

Loading

matklad commented Dec 10, 2020

bors commented Dec 10, 2020

bors commented Dec 11, 2020

tmandry commented Dec 11, 2020

bors commented Dec 11, 2020

matklad commented Dec 11, 2020

jyn514 Dec 12, 2020

		@@ -435,8 +435,10 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {

		// Try looking for methods and associated items.
		let mut split = path_str.rsplitn(2, "::");

Dogfood str_split_once() #79809

Dogfood str_split_once() #79809

Conversation

Eric-Arellano commented Dec 7, 2020

rust-highfive commented Dec 7, 2020

rust-highfive commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr commented Dec 8, 2020

matklad commented Dec 10, 2020 • edited Loading

matklad commented Dec 10, 2020

bors commented Dec 10, 2020

bors commented Dec 11, 2020

tmandry commented Dec 11, 2020

bors commented Dec 11, 2020

matklad commented Dec 11, 2020

Choose a reason for hiding this comment

Dogfood `str_split_once()` #79809

Dogfood `str_split_once()` #79809

matklad Dec 8, 2020 •

edited

Loading

matklad commented Dec 10, 2020 •

edited

Loading