Small code improvements in collect_intra_doc_links.rs
#112806
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @jsha (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
kadiwa4
force-pushed
the
collect_intra_doc_links
branch
from
2a25d1f
to
e8d7ef5
Compare
|
Thanks for the PR, could you provide a description of the changes, and even maybe show how they would improve the existing state would be helpful. |
kadiwa4
commented
Jun 25, 2023
Comment on lines
-109
to
+115
| if kind == DefKind::Macro(MacroKind::Bang) { | ||
| return Suggestion::Macro; | ||
| } else if kind == DefKind::Fn || kind == DefKind::AssocFn { | ||
| return Suggestion::Function; | ||
| } else if kind == DefKind::Field { | ||
| return Suggestion::RemoveDisambiguator; | ||
| } | ||
|
|
||
| let prefix = match kind { | ||
| DefKind::Fn | DefKind::AssocFn => return Suggestion::Function, | ||
| DefKind::Field => return Suggestion::RemoveDisambiguator, | ||
| DefKind::Macro(MacroKind::Bang) => return Suggestion::Macro, |
There was a problem hiding this comment.
Move the if-else blocks into the match block right below
kadiwa4
commented
Jun 25, 2023
Comment on lines
-284
to
+291
| let variant_field_name = split | ||
| .next() | ||
| .map(|f| Symbol::intern(f)) | ||
| .expect("fold_item should ensure link is non-empty"); | ||
| let variant_name = | ||
| // we're not sure this is a variant at all, so use the full string | ||
| // If there's no second component, the link looks like `[path]`. | ||
| // So there's no partial res and we should say the whole link failed to resolve. | ||
| split.next().map(|f| Symbol::intern(f)).ok_or_else(no_res)?; | ||
| let path = split | ||
| .next() | ||
| // If there's no third component, we saw `[a::b]` before and it failed to resolve. | ||
| // So there's no partial res. | ||
| .ok_or_else(no_res)?; | ||
| let variant_field_name = Symbol::intern(split.next().unwrap()); | ||
| // We're not sure this is a variant at all, so use the full string. | ||
| // If there's no second component, the link looks like `[path]`. | ||
| // So there's no partial res and we should say the whole link failed to resolve. | ||
| let variant_name = Symbol::intern(split.next().ok_or_else(no_res)?); | ||
|
|
||
| // If there's no third component, we saw `[a::b]` before and it failed to resolve. | ||
| // So there's no partial res. | ||
| let path = split.next().ok_or_else(no_res)?; |
There was a problem hiding this comment.
Move the comments above the statement which they're describing (one reason that's better is because they previously prevented rustfmt from working for that statement) and remove the misleading .expect(…) string. Also get rid of a .map(…) call.
kadiwa4
commented
Jun 25, 2023
Comment on lines
437
to
449
| let mut split = path_str.rsplitn(2, "::"); | ||
| // NB: `split`'s first element is always defined, even if the delimiter was not present. | ||
| // NB: `item_str` could be empty when resolving in the root namespace (e.g. `::std`). | ||
| let item_str = split.next().unwrap(); | ||
| let item_name = Symbol::intern(item_str); | ||
| let path_root = split | ||
| .next() | ||
| // NB: `path_root` could be empty when resolving in the root namespace (e.g. `::std`). | ||
| let (path_root, item_str) = path_str.rsplit_once("::").ok_or_else(|| { | ||
| // If there's no `::`, it's not an associated item. | ||
| // So we can be sure that `rustc_resolve` was accurate when it said it wasn't resolved. | ||
| .ok_or_else(|| { | ||
| debug!("found no `::`, assuming {} was correctly not in scope", item_name); | ||
| UnresolvedPath { | ||
| item_id, | ||
| module_id, | ||
| partial_res: None, | ||
| unresolved: item_str.into(), | ||
| } | ||
| })?; | ||
| debug!("found no `::`, assuming {path_str} was correctly not in scope"); | ||
| UnresolvedPath { item_id, module_id, partial_res: None, unresolved: path_str.into() } | ||
| })?; | ||
| let item_name = Symbol::intern(item_str); |
There was a problem hiding this comment.
Replace the usage of .rsplitn(2, …) with rsplit_once(…) to make the code simpler and make it clear that this doesn't panic (the .unwrap() makes it look like it might panic).
I also changed the comment about one of the strings possibly being empty because it previously made no sense. If path_str is "::std", then path_root will be empty, not item_str.
kadiwa4
commented
Jun 25, 2023
Comment on lines
-460
to
+464
| match resolve_primitive(&path_root, TypeNS) | ||
| .or_else(|| self.resolve_path(&path_root, TypeNS, item_id, module_id)) | ||
| .and_then(|ty_res| { | ||
| let candidates = self | ||
| .resolve_associated_item(ty_res, item_name, ns, module_id) | ||
| match resolve_primitive(path_root, TypeNS) | ||
| .or_else(|| self.resolve_path(path_root, TypeNS, item_id, module_id)) | ||
| .map(|ty_res| { | ||
| self.resolve_associated_item(ty_res, item_name, ns, module_id) | ||
| .into_iter() | ||
| .map(|(res, def_id)| (res, Some(def_id))) | ||
| .collect::<Vec<_>>(); | ||
| if !candidates.is_empty() { Some(candidates) } else { None } | ||
| .collect::<Vec<_>>() | ||
| }) { | ||
| Some(r) => Ok(r), | ||
| None => { | ||
| Some(r) if !r.is_empty() => Ok(r), | ||
| _ => { |
There was a problem hiding this comment.
The line
if !candidates.is_empty() { Some(candidates) } else { None }inside of and_then is just a .filter(…) condition with extra steps and can be moved into the match arm below.
kadiwa4
commented
Jun 25, 2023
Comment on lines
-1243
to
+1256
| if candidates.len() > 1 && !ambiguity_error(self.cx, &diag, &key.path_str, &candidates) { | ||
| candidates = vec![candidates[0]]; | ||
| if let [candidate, _candidate2, ..] = *candidates | ||
| && !ambiguity_error(self.cx, &diag, &key.path_str, &candidates) | ||
| { | ||
| candidates = vec![candidate]; |
There was a problem hiding this comment.
Pattern matching instead of checking .len() and then doing bounds-checked indexing.
kadiwa4
commented
Jun 25, 2023
Comment on lines
-1734
to
+1797
| let Some((start, end)) = split(name) else { | ||
| // FIXME(jynelson): this might conflict with my `Self` fix in #76467 | ||
| let Some((start, end)) = name.rsplit_once("::") else { |
There was a problem hiding this comment.
The helper function split already exists in the standard library: rsplit_once behaves the same way.
kadiwa4
commented
Jun 25, 2023
Comment on lines
-1745
to
+1809
| if !v_res.is_empty() { | ||
| *partial_res = Some(full_res(tcx, v_res[0])); | ||
| if let Some(&res) = v_res.first() { | ||
| *partial_res = Some(full_res(tcx, res)); |
There was a problem hiding this comment.
Pattern matching instead of checking .is_empty() and then doing bounds-checked indexing.
|
☔ The latest upstream changes (presumably #114905) made this pull request unmergeable. Please resolve the merge conflicts. |
kadiwa4
force-pushed
the
collect_intra_doc_links
branch
from
e8d7ef5
to
1c5cef9
Compare
|
☔ The latest upstream changes (presumably #117875) made this pull request unmergeable. Please resolve the merge conflicts. |
kadiwa4
force-pushed
the
collect_intra_doc_links
branch
from
1c5cef9
to
d2af7da
Compare
|
r? notriddle @bors r+ |
bors
added
S-waiting-on-bors
fmease
added a commit
to fmease/rust
that referenced
this pull request
Jan 23, 2024
…=notriddle Small code improvements in `collect_intra_doc_links.rs` Makes some of the code more readable by shortening it, and removes some unnecessary bounds checks.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 23, 2024
Rollup of 12 pull requests Successful merges: - rust-lang#112806 (Small code improvements in `collect_intra_doc_links.rs`) - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#119766 (Split tait and impl trait in assoc items logic) - rust-lang#120062 (llvm: change data layout bug to an error and make it trigger more) - rust-lang#120099 (linker: Refactor library linking methods in `trait Linker`) - rust-lang#120139 (Do not normalize closure signature when building `FnOnce` shim) - rust-lang#120160 (Manually implement derived `NonZero` traits.) - rust-lang#120171 (Fix assume and assert in jump threading) - rust-lang#120183 (Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`) - rust-lang#120195 (add several resolution test cases) - rust-lang#120259 (Split Diagnostics for Uncommon Codepoints: Add List to Display Characters Involved) - rust-lang#120261 (Provide structured suggestion to use trait objects in some cases of `if` arm type divergence) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 23, 2024
Rollup of 9 pull requests Successful merges: - rust-lang#112806 (Small code improvements in `collect_intra_doc_links.rs`) - rust-lang#119766 (Split tait and impl trait in assoc items logic) - rust-lang#120139 (Do not normalize closure signature when building `FnOnce` shim) - rust-lang#120160 (Manually implement derived `NonZero` traits.) - rust-lang#120171 (Fix assume and assert in jump threading) - rust-lang#120183 (Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`) - rust-lang#120195 (add several resolution test cases) - rust-lang#120259 (Split Diagnostics for Uncommon Codepoints: Add List to Display Characters Involved) - rust-lang#120261 (Provide structured suggestion to use trait objects in some cases of `if` arm type divergence) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 24, 2024
Rollup merge of rust-lang#112806 - kadiwa4:collect_intra_doc_links, r=notriddle Small code improvements in `collect_intra_doc_links.rs` Makes some of the code more readable by shortening it, and removes some unnecessary bounds checks.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Makes some of the code more readable by shortening it, and removes some unnecessary bounds checks.