1652: Improve type hints behavior r=matklad a=SomeoneToIgnore

This PR fixed the following type hints issues:

* Restructures the `InlayKind` enum contents based on the discussion here: https://github.com/rust-analyzer/rust-analyzer/pull/1606#issuecomment-515968055
* Races described in #1639 
* Caches the latest decorations received for each file to show them the next time the file is opened (instead of a new server request)

Co-authored-by: Kirill Bulatov <mail4score@gmail.com>
This commit is contained in:
bors[bot] 2019-08-06 16:50:49 +00:00
commit 7e12422fa2
5 changed files with 100 additions and 114 deletions

View file

@ -9,14 +9,9 @@ use ra_syntax::{
SmolStr, SyntaxKind, SyntaxNode, TextRange, SmolStr, SyntaxKind, SyntaxNode, TextRange,
}; };
#[derive(Debug, PartialEq, Eq, Clone)] #[derive(Debug, PartialEq, Eq)]
pub enum InlayKind { pub enum InlayKind {
LetBindingType, TypeHint,
ClosureParameterType,
ForExpressionBindingType,
IfExpressionType,
WhileLetExpressionType,
MatchArmType,
} }
#[derive(Debug)] #[derive(Debug)]
@ -46,7 +41,7 @@ fn get_inlay_hints(
} }
let pat = let_statement.pat()?; let pat = let_statement.pat()?;
let analyzer = SourceAnalyzer::new(db, file_id, let_statement.syntax(), None); let analyzer = SourceAnalyzer::new(db, file_id, let_statement.syntax(), None);
Some(get_pat_hints(db, &analyzer, pat, InlayKind::LetBindingType, false)) Some(get_pat_type_hints(db, &analyzer, pat, false))
}) })
.visit(|closure_parameter: LambdaExpr| { .visit(|closure_parameter: LambdaExpr| {
let analyzer = SourceAnalyzer::new(db, file_id, closure_parameter.syntax(), None); let analyzer = SourceAnalyzer::new(db, file_id, closure_parameter.syntax(), None);
@ -55,15 +50,7 @@ fn get_inlay_hints(
.params() .params()
.filter(|closure_param| closure_param.ascribed_type().is_none()) .filter(|closure_param| closure_param.ascribed_type().is_none())
.filter_map(|closure_param| closure_param.pat()) .filter_map(|closure_param| closure_param.pat())
.map(|root_pat| { .map(|root_pat| get_pat_type_hints(db, &analyzer, root_pat, false))
get_pat_hints(
db,
&analyzer,
root_pat,
InlayKind::ClosureParameterType,
false,
)
})
.flatten() .flatten()
.collect() .collect()
}) })
@ -71,17 +58,17 @@ fn get_inlay_hints(
.visit(|for_expression: ForExpr| { .visit(|for_expression: ForExpr| {
let pat = for_expression.pat()?; let pat = for_expression.pat()?;
let analyzer = SourceAnalyzer::new(db, file_id, for_expression.syntax(), None); let analyzer = SourceAnalyzer::new(db, file_id, for_expression.syntax(), None);
Some(get_pat_hints(db, &analyzer, pat, InlayKind::ForExpressionBindingType, false)) Some(get_pat_type_hints(db, &analyzer, pat, false))
}) })
.visit(|if_expr: IfExpr| { .visit(|if_expr: IfExpr| {
let pat = if_expr.condition()?.pat()?; let pat = if_expr.condition()?.pat()?;
let analyzer = SourceAnalyzer::new(db, file_id, if_expr.syntax(), None); let analyzer = SourceAnalyzer::new(db, file_id, if_expr.syntax(), None);
Some(get_pat_hints(db, &analyzer, pat, InlayKind::IfExpressionType, true)) Some(get_pat_type_hints(db, &analyzer, pat, true))
}) })
.visit(|while_expr: WhileExpr| { .visit(|while_expr: WhileExpr| {
let pat = while_expr.condition()?.pat()?; let pat = while_expr.condition()?.pat()?;
let analyzer = SourceAnalyzer::new(db, file_id, while_expr.syntax(), None); let analyzer = SourceAnalyzer::new(db, file_id, while_expr.syntax(), None);
Some(get_pat_hints(db, &analyzer, pat, InlayKind::WhileLetExpressionType, true)) Some(get_pat_type_hints(db, &analyzer, pat, true))
}) })
.visit(|match_arm_list: MatchArmList| { .visit(|match_arm_list: MatchArmList| {
let analyzer = SourceAnalyzer::new(db, file_id, match_arm_list.syntax(), None); let analyzer = SourceAnalyzer::new(db, file_id, match_arm_list.syntax(), None);
@ -90,9 +77,7 @@ fn get_inlay_hints(
.arms() .arms()
.map(|match_arm| match_arm.pats()) .map(|match_arm| match_arm.pats())
.flatten() .flatten()
.map(|root_pat| { .map(|root_pat| get_pat_type_hints(db, &analyzer, root_pat, true))
get_pat_hints(db, &analyzer, root_pat, InlayKind::MatchArmType, true)
})
.flatten() .flatten()
.collect(), .collect(),
) )
@ -100,11 +85,10 @@ fn get_inlay_hints(
.accept(&node)? .accept(&node)?
} }
fn get_pat_hints( fn get_pat_type_hints(
db: &RootDatabase, db: &RootDatabase,
analyzer: &SourceAnalyzer, analyzer: &SourceAnalyzer,
root_pat: Pat, root_pat: Pat,
kind: InlayKind,
skip_root_pat_hint: bool, skip_root_pat_hint: bool,
) -> Vec<InlayHint> { ) -> Vec<InlayHint> {
let original_pat = &root_pat.clone(); let original_pat = &root_pat.clone();
@ -118,7 +102,7 @@ fn get_pat_hints(
}) })
.map(|(range, pat_type)| InlayHint { .map(|(range, pat_type)| InlayHint {
range, range,
kind: kind.clone(), kind: InlayKind::TypeHint,
label: pat_type.display(db).to_string().into(), label: pat_type.display(db).to_string().into(),
}) })
.collect() .collect()
@ -232,52 +216,52 @@ fn main() {
assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[ assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[
InlayHint { InlayHint {
range: [193; 197), range: [193; 197),
kind: LetBindingType, kind: TypeHint,
label: "i32", label: "i32",
}, },
InlayHint { InlayHint {
range: [236; 244), range: [236; 244),
kind: LetBindingType, kind: TypeHint,
label: "i32", label: "i32",
}, },
InlayHint { InlayHint {
range: [275; 279), range: [275; 279),
kind: LetBindingType, kind: TypeHint,
label: "&str", label: "&str",
}, },
InlayHint { InlayHint {
range: [539; 543), range: [539; 543),
kind: LetBindingType, kind: TypeHint,
label: "(i32, char)", label: "(i32, char)",
}, },
InlayHint { InlayHint {
range: [566; 567), range: [566; 567),
kind: LetBindingType, kind: TypeHint,
label: "i32", label: "i32",
}, },
InlayHint { InlayHint {
range: [570; 571), range: [570; 571),
kind: LetBindingType, kind: TypeHint,
label: "i32", label: "i32",
}, },
InlayHint { InlayHint {
range: [573; 574), range: [573; 574),
kind: LetBindingType, kind: TypeHint,
label: "i32", label: "i32",
}, },
InlayHint { InlayHint {
range: [584; 585), range: [584; 585),
kind: LetBindingType, kind: TypeHint,
label: "i32", label: "i32",
}, },
InlayHint { InlayHint {
range: [577; 578), range: [577; 578),
kind: LetBindingType, kind: TypeHint,
label: "f64", label: "f64",
}, },
InlayHint { InlayHint {
range: [580; 581), range: [580; 581),
kind: LetBindingType, kind: TypeHint,
label: "f64", label: "f64",
}, },
]"# ]"#
@ -299,12 +283,12 @@ fn main() {
assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[ assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[
InlayHint { InlayHint {
range: [21; 30), range: [21; 30),
kind: LetBindingType, kind: TypeHint,
label: "i32", label: "i32",
}, },
InlayHint { InlayHint {
range: [57; 66), range: [57; 66),
kind: ClosureParameterType, kind: TypeHint,
label: "i32", label: "i32",
}, },
]"# ]"#
@ -326,12 +310,12 @@ fn main() {
assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[ assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[
InlayHint { InlayHint {
range: [21; 30), range: [21; 30),
kind: LetBindingType, kind: TypeHint,
label: "i32", label: "i32",
}, },
InlayHint { InlayHint {
range: [44; 53), range: [44; 53),
kind: ForExpressionBindingType, kind: TypeHint,
label: "i32", label: "i32",
}, },
]"# ]"#
@ -364,7 +348,7 @@ fn main() {
if let CustomOption::Some(Test { a: CustomOption::Some(x), b: y }) = &test {}; if let CustomOption::Some(Test { a: CustomOption::Some(x), b: y }) = &test {};
if let CustomOption::Some(Test { a: CustomOption::None, b: y }) = &test {}; if let CustomOption::Some(Test { a: CustomOption::None, b: y }) = &test {};
if let CustomOption::Some(Test { b: y, .. }) = &test {}; if let CustomOption::Some(Test { b: y, .. }) = &test {};
if test == CustomOption::None {} if test == CustomOption::None {}
}"#, }"#,
); );
@ -372,27 +356,27 @@ fn main() {
assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[ assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[
InlayHint { InlayHint {
range: [166; 170), range: [166; 170),
kind: LetBindingType, kind: TypeHint,
label: "CustomOption<Test>", label: "CustomOption<Test>",
}, },
InlayHint { InlayHint {
range: [334; 338), range: [334; 338),
kind: IfExpressionType, kind: TypeHint,
label: "&Test", label: "&Test",
}, },
InlayHint { InlayHint {
range: [389; 390), range: [389; 390),
kind: IfExpressionType, kind: TypeHint,
label: "&CustomOption<u32>", label: "&CustomOption<u32>",
}, },
InlayHint { InlayHint {
range: [392; 393), range: [392; 393),
kind: IfExpressionType, kind: TypeHint,
label: "&u8", label: "&u8",
}, },
InlayHint { InlayHint {
range: [531; 532), range: [531; 532),
kind: IfExpressionType, kind: TypeHint,
label: "&u32", label: "&u32",
}, },
]"# ]"#
@ -425,7 +409,7 @@ fn main() {
while let CustomOption::Some(Test { a: CustomOption::Some(x), b: y }) = &test {}; while let CustomOption::Some(Test { a: CustomOption::Some(x), b: y }) = &test {};
while let CustomOption::Some(Test { a: CustomOption::None, b: y }) = &test {}; while let CustomOption::Some(Test { a: CustomOption::None, b: y }) = &test {};
while let CustomOption::Some(Test { b: y, .. }) = &test {}; while let CustomOption::Some(Test { b: y, .. }) = &test {};
while test == CustomOption::None {} while test == CustomOption::None {}
}"#, }"#,
); );
@ -433,7 +417,7 @@ fn main() {
assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[ assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[
InlayHint { InlayHint {
range: [166; 170), range: [166; 170),
kind: LetBindingType, kind: TypeHint,
label: "CustomOption<Test>", label: "CustomOption<Test>",
}, },
]"# ]"#
@ -445,7 +429,7 @@ fn main() {
let (analysis, file_id) = single_file( let (analysis, file_id) = single_file(
r#" r#"
#[derive(PartialEq)] #[derive(PartialEq)]
enum CustomOption<T> { enum CustomOption<T> {
None, None,
Some(T), Some(T),
} }
@ -473,23 +457,23 @@ fn main() {
assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[ assert_debug_snapshot_matches!(analysis.inlay_hints(file_id).unwrap(), @r#"[
InlayHint { InlayHint {
range: [312; 316), range: [311; 315),
kind: MatchArmType, kind: TypeHint,
label: "Test", label: "Test",
}, },
InlayHint { InlayHint {
range: [359; 360), range: [358; 359),
kind: MatchArmType, kind: TypeHint,
label: "CustomOption<u32>", label: "CustomOption<u32>",
}, },
InlayHint { InlayHint {
range: [362; 363), range: [361; 362),
kind: MatchArmType, kind: TypeHint,
label: "u8", label: "u8",
}, },
InlayHint { InlayHint {
range: [485; 486), range: [484; 485),
kind: MatchArmType, kind: TypeHint,
label: "u32", label: "u32",
}, },
]"# ]"#

View file

@ -885,14 +885,7 @@ pub fn handle_inlay_hints(
label: api_type.label.to_string(), label: api_type.label.to_string(),
range: api_type.range.conv_with(&line_index), range: api_type.range.conv_with(&line_index),
kind: match api_type.kind { kind: match api_type.kind {
ra_ide_api::InlayKind::LetBindingType => InlayKind::LetBindingType, ra_ide_api::InlayKind::TypeHint => InlayKind::TypeHint,
ra_ide_api::InlayKind::ClosureParameterType => InlayKind::ClosureParameterType,
ra_ide_api::InlayKind::ForExpressionBindingType => {
InlayKind::ForExpressionBindingType
}
ra_ide_api::InlayKind::IfExpressionType => InlayKind::IfExpressionType,
ra_ide_api::InlayKind::WhileLetExpressionType => InlayKind::WhileLetExpressionType,
ra_ide_api::InlayKind::MatchArmType => InlayKind::MatchArmType,
}, },
}) })
.collect()) .collect())

View file

@ -213,12 +213,7 @@ pub struct InlayHintsParams {
#[derive(Debug, PartialEq, Eq, Deserialize, Serialize)] #[derive(Debug, PartialEq, Eq, Deserialize, Serialize)]
pub enum InlayKind { pub enum InlayKind {
LetBindingType, TypeHint,
ClosureParameterType,
ForExpressionBindingType,
IfExpressionType,
WhileLetExpressionType,
MatchArmType,
} }
#[derive(Debug, Deserialize, Serialize)] #[derive(Debug, Deserialize, Serialize)]

View file

@ -22,53 +22,56 @@ const typeHintDecorationType = vscode.window.createTextEditorDecorationType({
export class HintsUpdater { export class HintsUpdater {
private displayHints = true; private displayHints = true;
public async loadHints( public async toggleHintsDisplay(displayHints: boolean): Promise<void> {
editor: vscode.TextEditor | undefined if (this.displayHints !== displayHints) {
): Promise<void> { this.displayHints = displayHints;
if ( return this.refreshVisibleEditorsHints(
this.displayHints && displayHints ? undefined : []
editor !== undefined &&
this.isRustDocument(editor.document)
) {
await this.updateDecorationsFromServer(
editor.document.uri.toString(),
editor
); );
} }
} }
public async toggleHintsDisplay(displayHints: boolean): Promise<void> { public async refreshHintsForVisibleEditors(
if (this.displayHints !== displayHints) { cause?: TextDocumentChangeEvent
this.displayHints = displayHints; ): Promise<void> {
if (displayHints) {
return this.updateHints();
} else {
const editor = vscode.window.activeTextEditor;
if (editor != null) {
return editor.setDecorations(typeHintDecorationType, []);
}
}
}
}
public async updateHints(cause?: TextDocumentChangeEvent): Promise<void> {
if (!this.displayHints) { if (!this.displayHints) {
return; return;
} }
const editor = vscode.window.activeTextEditor; if (
if (editor == null) { cause !== undefined &&
(cause.contentChanges.length === 0 ||
!this.isRustDocument(cause.document))
) {
return; return;
} }
const document = cause == null ? editor.document : cause.document; return this.refreshVisibleEditorsHints();
if (!this.isRustDocument(document)) { }
return;
private async refreshVisibleEditorsHints(
newDecorations?: vscode.DecorationOptions[]
) {
const promises: Array<Promise<void>> = [];
for (const rustEditor of vscode.window.visibleTextEditors.filter(
editor => this.isRustDocument(editor.document)
)) {
if (newDecorations !== undefined) {
promises.push(
Promise.resolve(
rustEditor.setDecorations(
typeHintDecorationType,
newDecorations
)
)
);
} else {
promises.push(this.updateDecorationsFromServer(rustEditor));
}
} }
return await this.updateDecorationsFromServer( for (const promise of promises) {
document.uri.toString(), await promise;
editor }
);
} }
private isRustDocument(document: vscode.TextDocument): boolean { private isRustDocument(document: vscode.TextDocument): boolean {
@ -76,11 +79,10 @@ export class HintsUpdater {
} }
private async updateDecorationsFromServer( private async updateDecorationsFromServer(
documentUri: string,
editor: TextEditor editor: TextEditor
): Promise<void> { ): Promise<void> {
const newHints = await this.queryHints(documentUri); const newHints = await this.queryHints(editor.document.uri.toString());
if (newHints != null) { if (newHints !== null) {
const newDecorations = newHints.map(hint => ({ const newDecorations = newHints.map(hint => ({
range: hint.range, range: hint.range,
renderOptions: { after: { contentText: `: ${hint.label}` } } renderOptions: { after: { contentText: `: ${hint.label}` } }

View file

@ -151,15 +151,27 @@ export function activate(context: vscode.ExtensionContext) {
if (Server.config.displayInlayHints) { if (Server.config.displayInlayHints) {
const hintsUpdater = new HintsUpdater(); const hintsUpdater = new HintsUpdater();
hintsUpdater.loadHints(vscode.window.activeTextEditor).then(() => { hintsUpdater.refreshHintsForVisibleEditors().then(() => {
// vscode may ignore top level hintsUpdater.refreshHintsForVisibleEditors()
// so update the hints once when the focus changes to guarantee their presence
let editorChangeDisposable: vscode.Disposable | null = null;
editorChangeDisposable = vscode.window.onDidChangeActiveTextEditor(
_ => {
if (editorChangeDisposable !== null) {
editorChangeDisposable.dispose();
}
return hintsUpdater.refreshHintsForVisibleEditors();
}
);
disposeOnDeactivation( disposeOnDeactivation(
vscode.window.onDidChangeActiveTextEditor(editor => vscode.window.onDidChangeVisibleTextEditors(_ =>
hintsUpdater.loadHints(editor) hintsUpdater.refreshHintsForVisibleEditors()
) )
); );
disposeOnDeactivation( disposeOnDeactivation(
vscode.workspace.onDidChangeTextDocument(e => vscode.workspace.onDidChangeTextDocument(e =>
hintsUpdater.updateHints(e) hintsUpdater.refreshHintsForVisibleEditors(e)
) )
); );
disposeOnDeactivation( disposeOnDeactivation(