Skip to content

feat: Closure capture inlay hints #14742

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 3 commits into from
May 8, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 5 additions & 1 deletion crates/hir-ty/src/infer/closure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ impl HirPlace {
}

#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub(crate) enum CaptureKind {
pub enum CaptureKind {
ByRef(BorrowKind),
ByValue,
}
Expand All @@ -166,6 +166,10 @@ impl CapturedItem {
self.place.local
}

pub fn kind(&self) -> CaptureKind {
self.kind
}

pub fn display_kind(&self) -> &'static str {
match self.kind {
CaptureKind::ByRef(k) => match k {
Expand Down
5 changes: 3 additions & 2 deletions crates/hir-ty/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,9 @@ pub use autoderef::autoderef;
pub use builder::{ParamKind, TyBuilder};
pub use chalk_ext::*;
pub use infer::{
closure::CapturedItem, could_coerce, could_unify, Adjust, Adjustment, AutoBorrow, BindingMode,
InferenceDiagnostic, InferenceResult, OverloadedDeref, PointerCast,
closure::{CaptureKind, CapturedItem},
could_coerce, could_unify, Adjust, Adjustment, AutoBorrow, BindingMode, InferenceDiagnostic,
InferenceResult, OverloadedDeref, PointerCast,
};
pub use interner::Interner;
pub use lower::{
Expand Down
26 changes: 26 additions & 0 deletions crates/hir/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2611,6 +2611,10 @@ impl LocalSource {
self.source.file_id.original_file(db.upcast())
}

pub fn file(&self) -> HirFileId {
self.source.file_id
}

pub fn name(&self) -> Option<ast::Name> {
self.source.value.name()
}
Expand Down Expand Up @@ -3232,6 +3236,21 @@ impl ClosureCapture {
Local { parent: self.owner, binding_id: self.capture.local() }
}

pub fn kind(&self) -> CaptureKind {
match self.capture.kind() {
hir_ty::CaptureKind::ByRef(
hir_ty::mir::BorrowKind::Shallow | hir_ty::mir::BorrowKind::Shared,
) => CaptureKind::SharedRef,
hir_ty::CaptureKind::ByRef(hir_ty::mir::BorrowKind::Unique) => {
CaptureKind::UniqueSharedRef
}
hir_ty::CaptureKind::ByRef(hir_ty::mir::BorrowKind::Mut { .. }) => {
CaptureKind::MutableRef
}
hir_ty::CaptureKind::ByValue => CaptureKind::Move,
}
}

pub fn display_kind(&self) -> &'static str {
self.capture.display_kind()
}
Expand All @@ -3241,6 +3260,13 @@ impl ClosureCapture {
}
}

pub enum CaptureKind {
SharedRef,
UniqueSharedRef,
MutableRef,
Move,
}

#[derive(Clone, PartialEq, Eq, Debug)]
pub struct Type {
env: Arc<TraitEnvironment>,
Expand Down
28 changes: 17 additions & 11 deletions crates/ide/src/inlay_hints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,17 @@ use text_edit::TextEdit;

use crate::{navigation_target::TryToNav, FileId};

mod closing_brace;
mod implicit_static;
mod fn_lifetime_fn;
mod closure_ret;
mod adjustment;
mod chaining;
mod param_name;
mod binding_mode;
mod bind_pat;
mod binding_mode;
mod chaining;
mod closing_brace;
mod closure_ret;
mod closure_captures;
mod discriminant;
mod fn_lifetime_fn;
mod implicit_static;
mod param_name;

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct InlayHintsConfig {
Expand All @@ -42,6 +43,7 @@ pub struct InlayHintsConfig {
pub adjustment_hints_mode: AdjustmentHintsMode,
pub adjustment_hints_hide_outside_unsafe: bool,
pub closure_return_type_hints: ClosureReturnTypeHints,
pub closure_capture_hints: bool,
pub binding_mode_hints: bool,
pub lifetime_elision_hints: LifetimeElisionHints,
pub param_names_for_lifetime_elision_hints: bool,
Expand Down Expand Up @@ -88,6 +90,8 @@ pub enum AdjustmentHintsMode {
PreferPostfix,
}

// FIXME: Clean up this mess, the kinds are mainly used for setting different rendering properties in the lsp layer
// We should probably turns this into such a property holding struct. Or clean this up in some other form.
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum InlayKind {
BindingMode,
Expand All @@ -98,6 +102,7 @@ pub enum InlayKind {
Adjustment,
AdjustmentPostfix,
Lifetime,
ClosureCapture,
Parameter,
Type,
Discriminant,
Expand Down Expand Up @@ -444,10 +449,10 @@ fn hints(
ast::Expr::MethodCallExpr(it) => {
param_name::hints(hints, sema, config, ast::Expr::from(it))
}
ast::Expr::ClosureExpr(it) => closure_ret::hints(hints, famous_defs, config, file_id, it),
// We could show reborrows for all expressions, but usually that is just noise to the user
// and the main point here is to show why "moving" a mutable reference doesn't necessarily move it
// ast::Expr::PathExpr(_) => reborrow_hints(hints, sema, config, &expr),
ast::Expr::ClosureExpr(it) => {
closure_captures::hints(hints, famous_defs, config, file_id, it.clone());
closure_ret::hints(hints, famous_defs, config, file_id, it)
},
_ => None,
}
},
Expand Down Expand Up @@ -535,6 +540,7 @@ mod tests {
chaining_hints: false,
lifetime_elision_hints: LifetimeElisionHints::Never,
closure_return_type_hints: ClosureReturnTypeHints::Never,
closure_capture_hints: false,
adjustment_hints: AdjustmentHints::Never,
adjustment_hints_mode: AdjustmentHintsMode::Prefix,
adjustment_hints_hide_outside_unsafe: false,
Expand Down
193 changes: 193 additions & 0 deletions crates/ide/src/inlay_hints/closure_captures.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
//! Implementation of "closure return type" inlay hints.
//!
//! Tests live in [`bind_pat`][super::bind_pat] module.
use ide_db::{base_db::FileId, famous_defs::FamousDefs};
use syntax::ast::{self, AstNode};
use text_edit::{TextRange, TextSize};

use crate::{InlayHint, InlayHintLabel, InlayHintsConfig, InlayKind};

pub(super) fn hints(
acc: &mut Vec<InlayHint>,
FamousDefs(sema, _): &FamousDefs<'_, '_>,
config: &InlayHintsConfig,
_file_id: FileId,
closure: ast::ClosureExpr,
) -> Option<()> {
if !config.closure_capture_hints {
return None;
}
let ty = &sema.type_of_expr(&closure.clone().into())?.original;
let c = ty.as_closure()?;
let captures = c.captured_items(sema.db);

if captures.is_empty() {
return None;
}

let move_kw_range = match closure.move_token() {
Some(t) => t.text_range(),
None => {
let range = closure.syntax().first_token()?.prev_token()?.text_range();
let range = TextRange::new(range.end() - TextSize::from(1), range.end());
acc.push(InlayHint {
range,
kind: InlayKind::ClosureCapture,
label: InlayHintLabel::simple("move", None, None),
text_edit: None,
});
range
}
};
acc.push(InlayHint {
range: move_kw_range,
kind: InlayKind::ClosureCapture,
label: InlayHintLabel::from("("),
text_edit: None,
});
let last = captures.len() - 1;
for (idx, capture) in captures.into_iter().enumerate() {
let local = capture.local();
let source = local.primary_source(sema.db);

// force cache the source file, otherwise sema lookup will potentially panic
_ = sema.parse_or_expand(source.file());

acc.push(InlayHint {
range: move_kw_range,
kind: InlayKind::ClosureCapture,
label: InlayHintLabel::simple(
format!(
"{}{}",
match capture.kind() {
hir::CaptureKind::SharedRef => "&",
hir::CaptureKind::UniqueSharedRef => "&unique ",
hir::CaptureKind::MutableRef => "&mut ",
hir::CaptureKind::Move => "",
},
local.name(sema.db)
),
None,
source.name().and_then(|name| sema.original_range_opt(name.syntax())),
),
text_edit: None,
});

if idx != last {
acc.push(InlayHint {
range: move_kw_range,
kind: InlayKind::ClosureCapture,
label: InlayHintLabel::simple(", ", None, None),
text_edit: None,
});
}
}
acc.push(InlayHint {
range: move_kw_range,
kind: InlayKind::ClosureCapture,
label: InlayHintLabel::from(")"),
text_edit: None,
});

Some(())
}

#[cfg(test)]
mod tests {
use crate::{
inlay_hints::tests::{check_with_config, DISABLED_CONFIG},
InlayHintsConfig,
};

#[test]
fn all_capture_kinds() {
check_with_config(
InlayHintsConfig { closure_capture_hints: true, ..DISABLED_CONFIG },
r#"
//- minicore: copy, derive


#[derive(Copy, Clone)]
struct Copy;

struct NonCopy;

fn main() {
let foo = Copy;
let bar = NonCopy;
let mut baz = NonCopy;
let qux = &mut NonCopy;
|| {
// ^ move
// ^ (
// ^ &foo
// ^ ,
// ^ bar
// ^ ,
// ^ baz
// ^ ,
// ^ qux
// ^ )
foo;
bar;
baz;
qux;
};
|| {
// ^ move
// ^ (
// ^ &foo
// ^ ,
// ^ &bar
// ^ ,
// ^ &baz
// ^ ,
// ^ &qux
// ^ )
&foo;
&bar;
&baz;
&qux;
};
|| {
// ^ move
// ^ (
// ^ &mut baz
// ^ )
&mut baz;
};
// FIXME: &mut qux should be &unique qux
|| {
// ^ move
// ^ (
// ^ &mut baz
// ^ ,
// ^ &mut qux
// ^ )
baz = NonCopy;
*qux = NonCopy;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

@HKalbasi fwiw I still think this is wrong. qux is not a mutable binding so we shouldn't be allowed to take a mutable borrow here. Do you mind pointing me to the part in the rustc code base for this (assuming you have it at hand)?

Copy link
Member

Choose a reason for hiding this comment

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

qux is not mutable, but we are taking a mutable reference to the *qux, not qux itself. For example this code:

    let mut t1 = 2;
    let mut t2 = 5;
    let mut x = &mut t1;
    let mut y = || {
        //&x;
        *x = 3;
    };
    x = &mut t2;
    y();

compiles since y is capturing *x by mutable borrow, so x itself is free to mutate. But if the //&x; line becomes uncommented, y will capture the whole x by unique immutable borrow, and we will get compile error.

Copy link
Member

Choose a reason for hiding this comment

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

I think it will becomes less confusing if we render the whole place.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think my confusion comes from thinking of old capture logic? Also the rust reference I suppose is outdated then. You are certainly right that we should be rendering the place we capture, as with this mere field captures will render incorrectly already I believe.

Copy link
Member

Choose a reason for hiding this comment

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

I think my confusion comes from thinking of old capture logic? Also the rust reference I suppose is outdated then

Yes the rust reference section for closure is pre edition-2021, and is currently in a misleading state.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out there is a PR for this already so I'll dig into that for now rust-lang/reference#1059

Copy link
Member Author

Choose a reason for hiding this comment

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

}
"#,
);
}

#[test]
fn move_token() {
check_with_config(
InlayHintsConfig { closure_capture_hints: true, ..DISABLED_CONFIG },
r#"
//- minicore: copy, derive
fn main() {
let foo = u32;
move || {
// ^^^^ (
// ^^^^ foo
// ^^^^ )
foo;
};
}
"#,
);
}
}
1 change: 1 addition & 0 deletions crates/ide/src/static_index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ impl StaticIndex<'_> {
param_names_for_lifetime_elision_hints: false,
binding_mode_hints: false,
max_length: Some(25),
closure_capture_hints: false,
closing_brace_hints_min_lines: Some(25),
},
file_id,
Expand Down
3 changes: 3 additions & 0 deletions crates/rust-analyzer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,8 @@ config_data! {
/// Minimum number of lines required before the `}` until the hint is shown (set to 0 or 1
/// to always show them).
inlayHints_closingBraceHints_minLines: usize = "25",
/// Whether to show inlay hints for closure captures.
inlayHints_closureCaptureHints_enable: bool = "false",
/// Whether to show inlay type hints for return types of closures.
inlayHints_closureReturnTypeHints_enable: ClosureReturnTypeHintsDef = "\"never\"",
/// Closure notation in type and chaining inlay hints.
Expand Down Expand Up @@ -1312,6 +1314,7 @@ impl Config {
ClosureStyle::WithId => hir::ClosureStyle::ClosureWithId,
ClosureStyle::Hide => hir::ClosureStyle::Hide,
},
closure_capture_hints: self.data.inlayHints_closureCaptureHints_enable,
adjustment_hints: match self.data.inlayHints_expressionAdjustmentHints_enable {
AdjustmentHintsDef::Always => ide::AdjustmentHints::Always,
AdjustmentHintsDef::Never => match self.data.inlayHints_reborrowHints_enable {
Expand Down
Loading