Skip to content

Enable "jump to def" feature on patterns #134216

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

Merged
merged 4 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/librustdoc/html/highlight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ enum Class {
Ident(Span),
Lifetime,
PreludeTy(Span),
PreludeVal,
PreludeVal(Span),
QuestionMark,
Decoration(&'static str),
}
Expand Down Expand Up @@ -385,7 +385,7 @@ impl Class {
Class::Ident(_) => "",
Class::Lifetime => "lifetime",
Class::PreludeTy(_) => "prelude-ty",
Class::PreludeVal => "prelude-val",
Class::PreludeVal(_) => "prelude-val",
Class::QuestionMark => "question-mark",
Class::Decoration(kind) => kind,
}
Expand All @@ -395,7 +395,11 @@ impl Class {
/// a "span" (a tuple representing `(lo, hi)` equivalent of `Span`).
fn get_span(self) -> Option<Span> {
match self {
Self::Ident(sp) | Self::Self_(sp) | Self::Macro(sp) | Self::PreludeTy(sp) => Some(sp),
Self::Ident(sp)
| Self::Self_(sp)
| Self::Macro(sp)
| Self::PreludeTy(sp)
| Self::PreludeVal(sp) => Some(sp),
Self::Comment
| Self::DocComment
| Self::Attribute
Expand All @@ -406,7 +410,6 @@ impl Class {
| Self::Number
| Self::Bool
| Self::Lifetime
| Self::PreludeVal
| Self::QuestionMark
| Self::Decoration(_) => None,
}
Expand Down Expand Up @@ -851,7 +854,9 @@ impl<'src> Classifier<'src> {
TokenKind::Ident => match get_real_ident_class(text, false) {
None => match text {
"Option" | "Result" => Class::PreludeTy(self.new_span(before, text)),
"Some" | "None" | "Ok" | "Err" => Class::PreludeVal,
"Some" | "None" | "Ok" | "Err" => {
Class::PreludeVal(self.new_span(before, text))
}
// "union" is a weak keyword and is only considered as a keyword when declaring
// a union type.
"union" if self.check_if_is_union_keyword() => Class::KeyWord,
Expand Down
33 changes: 29 additions & 4 deletions src/librustdoc/html/render/span_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_hir::intravisit::{self, Visitor};
use rustc_hir::{ExprKind, HirId, Item, ItemKind, Mod, Node};
use rustc_hir::{ExprKind, HirId, Item, ItemKind, Mod, Node, Pat, PatKind, QPath};
use rustc_middle::hir::nested_filter;
use rustc_middle::ty::TyCtxt;
use rustc_span::hygiene::MacroKind;
Expand Down Expand Up @@ -170,7 +170,7 @@ impl SpanMapVisitor<'_> {
true
}

fn handle_call(&mut self, hir_id: HirId, expr_hir_id: Option<HirId>, span: Span) {
fn infer_id(&mut self, hir_id: HirId, expr_hir_id: Option<HirId>, span: Span) {
let hir = self.tcx.hir();
let body_id = hir.enclosing_body_owner(hir_id);
// FIXME: this is showing error messages for parts of the code that are not
Expand All @@ -189,6 +189,27 @@ impl SpanMapVisitor<'_> {
self.matches.insert(span, link);
}
}

fn handle_pat(&mut self, p: &Pat<'_>) {
Copy link
Member

@fmease fmease Dec 12, 2024

Choose a reason for hiding this comment

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

This function very closely mirrors intravisit::walk_pat. The only difference is the QPath handling. The more general and cleaner solution would be to explicitly implement visit_qself. This would automatically handle paths in expression, pattern and type contexts.

For example, rn we don't linkify Self::Var (ExprKind::Path(qpath)):

impl Trait for Ty { fn f() { let _ = Self::Var; } }
pub enum Ty { Var }
pub trait Trait { fn f(); }

Moreover, I presume we don't linkify Self::Assoc in impl ... for ... { type Ty = Self::Assoc; } (TyKind::Path(qpath)) but I can't check, I'm commuting atm.

However, this would require this "maybe_typeck_results/qpath_res " setup I mentioned on Zulip. Furthermore, it would support fully-qualified paths <Ty as TraitRef>::assoc which we don't on master.

Anyway, if you don't want to do that in this PR, I can send a follup-up PR that impls that.

My 2¢ – won't say more rn, I'm still commuting lol.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was not in hurry for review you know. 😅 (But thanks!)

So do you want me to implement visit_qself in this PR or in a follow-up?

Copy link
Member

Choose a reason for hiding this comment

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

Let me get back home, then I'm going to answer that xD

match p.kind {
PatKind::Binding(_, _, _, Some(p)) => self.handle_pat(p),
PatKind::Struct(qpath, _, _)
| PatKind::TupleStruct(qpath, _, _)
| PatKind::Path(qpath) => match qpath {
QPath::TypeRelative(_, path) if matches!(path.res, Res::Err) => {
self.infer_id(path.hir_id, Some(p.hir_id), qpath.span());
}
QPath::Resolved(_, path) => self.handle_path(path),
_ => {}
},
PatKind::Or(pats) => {
for pat in pats {
self.handle_pat(pat);
}
}
_ => {}
}
}
}

impl<'tcx> Visitor<'tcx> for SpanMapVisitor<'tcx> {
Expand All @@ -206,6 +227,10 @@ impl<'tcx> Visitor<'tcx> for SpanMapVisitor<'tcx> {
intravisit::walk_path(self, path);
}

fn visit_pat(&mut self, p: &Pat<'tcx>) {
self.handle_pat(p);
}

fn visit_mod(&mut self, m: &'tcx Mod<'tcx>, span: Span, id: HirId) {
// To make the difference between "mod foo {}" and "mod foo;". In case we "import" another
// file, we want to link to it. Otherwise no need to create a link.
Expand All @@ -228,9 +253,9 @@ impl<'tcx> Visitor<'tcx> for SpanMapVisitor<'tcx> {
fn visit_expr(&mut self, expr: &'tcx rustc_hir::Expr<'tcx>) {
match expr.kind {
ExprKind::MethodCall(segment, ..) => {
self.handle_call(segment.hir_id, Some(expr.hir_id), segment.ident.span)
self.infer_id(segment.hir_id, Some(expr.hir_id), segment.ident.span)
}
ExprKind::Call(call, ..) => self.handle_call(call.hir_id, None, call.span),
ExprKind::Call(call, ..) => self.infer_id(call.hir_id, None, call.span),
_ => {
if self.handle_macro(expr.span) {
// We don't want to go deeper into the macro.
Expand Down
52 changes: 52 additions & 0 deletions tests/rustdoc/jump-to-def-pats.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// This test ensures that patterns also get a link generated.

//@ compile-flags: -Zunstable-options --generate-link-to-definition

#![crate_name = "foo"]

//@ has 'src/foo/jump-to-def-pats.rs.html'

use std::fmt;

pub enum MyEnum<T, E> {
Ok(T),
Err(E),
Some(T),
None,
}

pub enum X {
A,
}

pub fn foo() -> Result<(), ()> {
// FIXME: would be nice to be able to check both the class and the href at the same time so
// we could check the text as well...
//@ has - '//a[@class="prelude-val"]/@href' '{{channel}}/core/result/enum.Result.html#variant.Ok'
//@ has - '//a[@href="{{channel}}/core/result/enum.Result.html#variant.Ok"]' 'Ok'
Ok(())
}

impl<T, E> fmt::Display for MyEnum<T, E> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
//@ has - '//a[@href="#12"]' 'Self::Ok'
Self::Ok(_) => f.write_str("MyEnum::Ok"),
//@ has - '//a[@href="#13"]' 'MyEnum::Err'
MyEnum::Err(_) => f.write_str("MyEnum::Err"),
//@ has - '//a[@href="#14"]' 'Self::Some'
Self::Some(_) => f.write_str("MyEnum::Some"),
//@ has - '//a[@href="#15"]' 'Self::None'
Self::None => f.write_str("MyEnum::None"),
}
}
}

impl X {
fn p(&self) -> &str {
match self {
//@ has - '//a[@href="#19"]' 'Self::A'
Self::A => "X::A",
}
}
}
Loading