Skip to content
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.

Sign up for GitHub

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

Parameter inlay hint separate from variable type inlay? #2876 #3543

Merged
merged 5 commits into from
Mar 12, 2020
Merged

Parameter inlay hint separate from variable type inlay? #2876 #3543

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e98aff1
Parameter inlay hint separate from variable type inlay? #2876
slyngbaek Mar 10, 2020
cfb48df
Address Issues from Github
slyngbaek Mar 10, 2020
974ed71
Deduplicate some Inlay definitions
slyngbaek Mar 11, 2020
58248e2
Switch from Vec<InlayKind> to object with props
slyngbaek Mar 12, 2020
a153b90
Make maxLength nullable again
slyngbaek Mar 12, 2020
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
112 changes: 94 additions & 18 deletions crates/ra_ide/src/inlay_hints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,19 @@ use ra_syntax::{

use crate::{FileId, FunctionSignature};

#[derive(Debug, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct InlayConfig {
pub display_type: Vec<InlayKind>,
pub max_length: Option<usize>,
}

impl Default for InlayConfig {
fn default() -> Self {
Self { display_type: vec![InlayKind::TypeHint, InlayKind::ParameterHint], max_length: None }
}
}

#[derive(Clone, Debug, PartialEq, Eq)]
pub enum InlayKind {
TypeHint,
ParameterHint,
Expand All @@ -26,7 +38,7 @@ pub struct InlayHint {
pub(crate) fn inlay_hints(
db: &RootDatabase,
file_id: FileId,
max_inlay_hint_length: Option<usize>,
inlay_hint_opts: &InlayConfig,
) -> Vec<InlayHint> {
let _p = profile("inlay_hints");
let sema = Semantics::new(db);
Expand All @@ -36,9 +48,9 @@ pub(crate) fn inlay_hints(
for node in file.syntax().descendants() {
match_ast! {
match node {
ast::CallExpr(it) => { get_param_name_hints(&mut res, &sema, ast::Expr::from(it)); },
ast::MethodCallExpr(it) => { get_param_name_hints(&mut res, &sema, ast::Expr::from(it)); },
ast::BindPat(it) => { get_bind_pat_hints(&mut res, &sema, max_inlay_hint_length, it); },
ast::CallExpr(it) => { get_param_name_hints(&mut res, &sema, inlay_hint_opts, ast::Expr::from(it)); },
ast::MethodCallExpr(it) => { get_param_name_hints(&mut res, &sema, inlay_hint_opts, ast::Expr::from(it)); },
ast::BindPat(it) => { get_bind_pat_hints(&mut res, &sema, inlay_hint_opts, it); },
_ => (),
}
}
Expand All @@ -49,8 +61,13 @@ pub(crate) fn inlay_hints(
fn get_param_name_hints(
acc: &mut Vec<InlayHint>,
sema: &Semantics<RootDatabase>,
inlay_hint_opts: &InlayConfig,
expr: ast::Expr,
) -> Option<()> {
if !inlay_hint_opts.display_type.contains(&InlayKind::ParameterHint) {
return None;
}

let args = match &expr {
ast::Expr::CallExpr(expr) => expr.arg_list()?.args(),
ast::Expr::MethodCallExpr(expr) => expr.arg_list()?.args(),
Expand Down Expand Up @@ -84,9 +101,13 @@ fn get_param_name_hints(
fn get_bind_pat_hints(
acc: &mut Vec<InlayHint>,
sema: &Semantics<RootDatabase>,
max_inlay_hint_length: Option<usize>,
inlay_hint_opts: &InlayConfig,
pat: ast::BindPat,
) -> Option<()> {
if !inlay_hint_opts.display_type.contains(&InlayKind::TypeHint) {
return None;
}

let ty = sema.type_of_pat(&pat.clone().into())?;

if should_not_display_type_hint(sema.db, &pat, &ty) {
Expand All @@ -96,7 +117,7 @@ fn get_bind_pat_hints(
acc.push(InlayHint {
range: pat.syntax().text_range(),
kind: InlayKind::TypeHint,
label: ty.display_truncated(sema.db, max_inlay_hint_length).to_string().into(),
label: ty.display_truncated(sema.db, inlay_hint_opts.max_length).to_string().into(),
});
Some(())
}
Expand Down Expand Up @@ -202,10 +223,65 @@ fn get_fn_signature(sema: &Semantics<RootDatabase>, expr: &ast::Expr) -> Option<

#[cfg(test)]
mod tests {
use crate::inlay_hints::{InlayConfig, InlayKind};
use insta::assert_debug_snapshot;

use crate::mock_analysis::single_file;

#[test]
fn param_hints_only() {
let (analysis, file_id) = single_file(
r#"
fn foo(a: i32, b: i32) -> i32 { a + b }
fn main() {
let _x = foo(4, 4);
}"#,
);
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig{ display_type: vec![InlayKind::ParameterHint], max_length: None}).unwrap(), @r###"
[
InlayHint {
range: [106; 107),
kind: ParameterHint,
label: "a",
},
InlayHint {
range: [109; 110),
kind: ParameterHint,
label: "b",
},
]"###);
}

#[test]
fn hints_disabled() {
let (analysis, file_id) = single_file(
r#"
fn foo(a: i32, b: i32) -> i32 { a + b }
fn main() {
let _x = foo(4, 4);
}"#,
);
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig{ display_type: vec![], max_length: None}).unwrap(), @r###"[]"###);
}

#[test]
fn type_hints_only() {
let (analysis, file_id) = single_file(
r#"
fn foo(a: i32, b: i32) -> i32 { a + b }
fn main() {
let _x = foo(4, 4);
}"#,
);
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig{ display_type: vec![InlayKind::TypeHint], max_length: None}).unwrap(), @r###"
[
InlayHint {
range: [97; 99),
kind: TypeHint,
label: "i32",
},
]"###);
}
#[test]
fn default_generic_types_should_not_be_displayed() {
let (analysis, file_id) = single_file(
Expand All @@ -221,7 +297,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, None).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig::default()).unwrap(), @r###"
[
InlayHint {
range: [69; 71),
Expand Down Expand Up @@ -278,7 +354,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, None).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig::default()).unwrap(), @r###"
[
InlayHint {
range: [193; 197),
Expand Down Expand Up @@ -358,7 +434,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, None).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig::default()).unwrap(), @r###"
[
InlayHint {
range: [21; 30),
Expand Down Expand Up @@ -422,7 +498,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, None).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig::default()).unwrap(), @r###"
[
InlayHint {
range: [21; 30),
Expand Down Expand Up @@ -472,7 +548,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, None).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig::default()).unwrap(), @r###"
[
InlayHint {
range: [188; 192),
Expand Down Expand Up @@ -567,7 +643,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, None).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig::default()).unwrap(), @r###"
[
InlayHint {
range: [188; 192),
Expand Down Expand Up @@ -662,7 +738,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, None).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig::default()).unwrap(), @r###"
[
InlayHint {
range: [252; 256),
Expand Down Expand Up @@ -734,7 +810,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, Some(8)).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig { display_type: vec![InlayKind::TypeHint, InlayKind::ParameterHint], max_length: Some(8) }).unwrap(), @r###"
[
InlayHint {
range: [74; 75),
Expand Down Expand Up @@ -822,7 +898,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, None).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig::default()).unwrap(), @r###"
[
InlayHint {
range: [798; 809),
Expand Down Expand Up @@ -944,7 +1020,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, Some(8)).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig { display_type: vec![InlayKind::TypeHint, InlayKind::ParameterHint], max_length: Some(8) }).unwrap(), @r###"
[]
"###
);
Expand All @@ -970,7 +1046,7 @@ fn main() {
}"#,
);

assert_debug_snapshot!(analysis.inlay_hints(file_id, Some(8)).unwrap(), @r###"
assert_debug_snapshot!(analysis.inlay_hints(file_id, &InlayConfig { display_type: vec![InlayKind::TypeHint, InlayKind::ParameterHint], max_length: Some(8) }).unwrap(), @r###"
[]
"###
);
Expand Down
6 changes: 3 additions & 3 deletions crates/ra_ide/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ pub use crate::{
expand_macro::ExpandedMacro,
folding_ranges::{Fold, FoldKind},
hover::HoverResult,
inlay_hints::{InlayHint, InlayKind},
inlay_hints::{InlayConfig, InlayHint, InlayKind},
references::{Declaration, Reference, ReferenceAccess, ReferenceKind, ReferenceSearchResult},
runnables::{Runnable, RunnableKind, TestId},
source_change::{FileSystemEdit, SourceChange, SourceFileEdit},
Expand Down Expand Up @@ -318,9 +318,9 @@ impl Analysis {
pub fn inlay_hints(
&self,
file_id: FileId,
max_inlay_hint_length: Option<usize>,
inlay_hint_opts: &InlayConfig,
) -> Cancelable<Vec<InlayHint>> {
self.with_db(|db| inlay_hints::inlay_hints(db, file_id, max_inlay_hint_length))
self.with_db(|db| inlay_hints::inlay_hints(db, file_id, inlay_hint_opts))
}

/// Returns the set of folding ranges.
Expand Down
23 changes: 23 additions & 0 deletions crates/ra_project_model/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use anyhow::{bail, Context, Result};
use ra_cfg::CfgOptions;
use ra_db::{CrateGraph, CrateName, Edition, Env, FileId};
use rustc_hash::FxHashMap;
use serde::Deserialize;
use serde_json::from_reader;

pub use crate::{
Expand All @@ -24,6 +25,28 @@ pub use crate::{
sysroot::Sysroot,
};

#[derive(Deserialize, Clone, Debug, PartialEq, Eq)]
#[serde(rename_all = "lowercase")]
pub enum InlayHintDisplayType {
slyngbaek marked this conversation as resolved.
Show resolved Hide resolved
Off,
TypeHints,
ParameterHints,
Full,
}

#[derive(Deserialize, Clone, Debug, PartialEq, Eq)]
#[serde(rename_all = "camelCase", default)]
pub struct InlayHintOptions {
pub display_type: InlayHintDisplayType,
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore Mar 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

#2741 have plans to add more various inlay hint types later.

I think we'd better allow to have various combinations of the types turned on simultaneously, since there will be requests for those.
So should we go with a set of InlayKind's instead of this enum?

Copy link
Contributor

Choose a reason for hiding this comment

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

A bitset I guess?

Copy link
Contributor

Choose a reason for hiding this comment

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

Imo, just the HashSet<InlayKind> is enough.
Or its fx counterpart, since we use FxHashMap in the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recall @matklad once said it's better not to use HashSet where Vec is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still a bit of a beginner with rust, but @matklad suggested it be a POD (I'm assuming meaning that it implements Copy?). If I use a Vec or any of these structures it can't be a Copy type.

pub max_length: Option<usize>,
}

impl Default for InlayHintOptions {
slyngbaek marked this conversation as resolved.
Show resolved Hide resolved
fn default() -> Self {
Self { display_type: InlayHintDisplayType::Full, max_length: None }
}
}

#[derive(Clone, PartialEq, Eq, Hash, Debug)]
pub struct CargoTomlNotFoundError {
pub searched_at: PathBuf,
Expand Down
34 changes: 32 additions & 2 deletions crates/rust-analyzer/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,40 @@
//! configure the server itself, feature flags are passed into analysis, and
//! tweak things like automatic insertion of `()` in completions.

use ra_ide::{InlayConfig, InlayKind};
use rustc_hash::FxHashMap;

use ra_project_model::CargoFeatures;
use serde::{Deserialize, Deserializer};

#[derive(Deserialize)]
#[serde(remote = "InlayKind")]
pub enum InlayKindDef {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's already an enum for this entity:
https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/rust-analyzer/src/req.rs#L199

so let's not create a copy of it.
A few more observations, feel free to implement any (or none) of them, if you want to:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense to deduplicate the InlayKind as mentioned and then just use the serde remote feature so we don't even need to do the match in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I'd prefer using match, but if your way works, it's also good, thanks for the dedup.

TypeHint,
ParameterHint,
}

// Work-around until better serde support is added
// https://github.com/serde-rs/serde/issues/723#issuecomment-382501277
fn vec_inlay_kind<'de, D>(deserializer: D) -> Result<Vec<InlayKind>, D::Error>
where
D: Deserializer<'de>,
{
#[derive(Deserialize)]
struct Wrapper(#[serde(with = "InlayKindDef")] InlayKind);

let v = Vec::deserialize(deserializer)?;
Ok(v.into_iter().map(|Wrapper(a)| a).collect())
}

#[derive(Deserialize)]
#[serde(remote = "InlayConfig")]
pub struct InlayConfigDef {
#[serde(deserialize_with = "vec_inlay_kind")]
pub display_type: Vec<InlayKind>,
pub max_length: Option<usize>,
}

/// Client provided initialization options
#[derive(Deserialize, Clone, Debug, PartialEq, Eq)]
#[serde(rename_all = "camelCase", default)]
Expand All @@ -30,7 +59,8 @@ pub struct ServerConfig {

pub lru_capacity: Option<usize>,

pub max_inlay_hint_length: Option<usize>,
#[serde(with = "InlayConfigDef")]
pub inlay_hint_opts: InlayConfig,

pub cargo_watch_enable: bool,
pub cargo_watch_args: Vec<String>,
Expand All @@ -57,7 +87,7 @@ impl Default for ServerConfig {
exclude_globs: Vec::new(),
use_client_watching: false,
lru_capacity: None,
max_inlay_hint_length: None,
inlay_hint_opts: Default::default(),
cargo_watch_enable: true,
cargo_watch_args: Vec::new(),
cargo_watch_command: "check".to_string(),
Expand Down
2 changes: 1 addition & 1 deletion crates/rust-analyzer/src/main_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ pub fn main_loop(
.and_then(|it| it.folding_range.as_ref())
.and_then(|it| it.line_folding_only)
.unwrap_or(false),
max_inlay_hint_length: config.max_inlay_hint_length,
inlay_hint_opts: config.inlay_hint_opts,
cargo_watch: CheckOptions {
enable: config.cargo_watch_enable,
args: config.cargo_watch_args,
Expand Down
2 changes: 1 addition & 1 deletion crates/rust-analyzer/src/main_loop/handlers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -997,7 +997,7 @@ pub fn handle_inlay_hints(
let analysis = world.analysis();
let line_index = analysis.file_line_index(file_id)?;
Ok(analysis
.inlay_hints(file_id, world.options.max_inlay_hint_length)?
.inlay_hints(file_id, &world.options.inlay_hint_opts)?
.into_iter()
.map(|api_type| InlayHint {
label: api_type.label.to_string(),
Expand Down
Loading