Fix bad intra-doc-link preprocessing #148169
Conversation
rustbot
added
S-waiting-on-review
| // IMPORTANT: To be kept in sync with the corresponding function in `rustc_resolve::rustdoc`. | ||
| // Namely, whenever this function successfully returns a `path_str` for a given input, the | ||
| // rustc counterpart *MUST* return the same result! |
| // certain link kinds cannot have their path be urls, | ||
| // so they should not be ignored, no matter how much they look like urls. | ||
| // e.g. [https://example.com/] is not a link to example.com. | ||
| // e.g. `[https://example.com/]` is not a link to example.com. |
There was a problem hiding this comment.
I think this is confusing to use markdown syntax in a remark about markdown syntax, as `[https://example.com/]` and [https://example.com/] are different things, and we are talking about the latter, not the former.
Maybe we can just use " quotes?
| ); | ||
|
|
||
| // [] is mostly likely not supposed to be a link | ||
| // `[]` is mostly likely not supposed to be a link |
There was a problem hiding this comment.
A bit less confusing than above, but I still am kinda meh on the idea of non-doc comments always using markdown syntax. Maybe there's a valid reason for it I'm unaware of but idk.
| /// Simplified version of the corresponding function in rustdoc. | ||
| /// If the rustdoc version returns a successful result, this function must return the same result. | ||
| /// Otherwise this function may return anything. | ||
| fn preprocess_link(link: &str) -> Box<str> { | ||
| // IMPORTANT: To be kept in sync with the corresponding function in rustdoc. | ||
| // Namely, whenever the rustdoc function returns a successful result for a | ||
| // given input, this function *MUST* return the same result! |
There was a problem hiding this comment.
Somewhere in here we should mention that path_str is the field of PreprocessingInfo this return value corresponds to.
| let link = link.split('#').next().unwrap(); | ||
| let link = link.trim(); | ||
| let link = link.rsplit('@').next().unwrap(); | ||
| let link = link.split_once('@').map_or(link, |(_, link)| link); |
There was a problem hiding this comment.
| let link = link.split_once('@').map_or(link, |(_, link)| link); | |
| let link = link.split_once('@').map_or(link, |(_, x)| x); |
nit: 3 different variables named link all being referenced on the same line might be a bit much.
There was a problem hiding this comment.
I probably won't name it x since I'm not a fan of non-descriptive identifiers but I'll probably name it right or suffix.
| strip_generics_from_path(link).unwrap_or_else(|_| link.into()) | ||
| let link = strip_generics_from_path(link).unwrap_or_else(|_| link.into()); | ||
| link |
There was a problem hiding this comment.
Leftover from printf dbg'ing, will revert
3 participants
How did #147981 happen?
/[a-zA-Z0-9_:<>, !*&;]/(we don't even emit lint broken-intra-doc-links for these).ignore_urllike && should_ignore_link(path_str)).preprocess_linkmust be kept in sync with a simplified counterpart in rustc. More specifically, whenever rustdoc's preprocessor returns a successful result then rustc's must yield the same result. Otherwise, rustc doesn't resolve the necessary links for rustdoc.preprocess_link. Namely, when presented with a link likestruct@Type@suffix, it didn't cut off the disambiguator if present (here:struct@). Instead itrsplit('@')which is incorrect if the "path" contains@itself (yieldingsuffixinstead ofType@suffixhere). Prior to PR #132748, a link like[`struct@Type@suffix`]was not considered a potential intra-doc link / worth querying rustc for. Now it is due to the backticks.Type@suffix(it only recordedsuffix(to beRes::Err)), we triggered an assertion we have in place to catch cases like this.Fixes #147981.
I didn't and still don't have the time to investigate if #132748 led to more rustc/rustdoc mismatches (after all, the PR makde rustdoc's
preprocess_linkreturnSome(Ok(_))in more cases). I've at least added another much needed "warning banner" & made the existing one more flashy.While this fixes a stable-to-beta regression, I don't think it's worth beta backporting, esp. since it's only P-medium and since the final 1.91 release steps commence today / the next days, so it would only be stressful to get it in on time. However, feel free to nominate.
r? @lolbinarycat