Introduce Semantics API #3222

matklad · 2020-02-18T17:37:05Z

Based on ideas in https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0/topic/Design.20question.3A.20identity.20in.20syntax.20trees

crates/ra_ide/src/goto_type_definition.rs

kjeremy · 2020-02-18T18:36:57Z

crates/ra_hir/src/semantics.rs

+        let prev = cache.insert(root_node, file_id);
+        assert!(prev.is_none());
+    }
+    fn lookup(&self, root_node: &SyntaxNode) -> HirFileId {


I think this name should be more specific.

kjeremy · 2020-02-18T18:38:21Z

Seems like an improvement to me!

matklad · 2020-02-19T18:01:25Z

ported syntax highlighting to Semantics. It does look nicer!

Veetaha · 2020-02-25T08:42:01Z

crates/ra_hir/src/semantics.rs

+
+pub struct Semantics<'db, DB> {
+    pub db: &'db DB,
+    sb: RefCell<SourceBinder<'db, DB>>,


I wish we could avoid RefCell for SourceBinder here...

This is the perfect use-case for RefCell actually. Or, rather, it was an API mistake to not use RefCell in the SourceBinder in the first place.

This type is not thread safe (as it stores syntax nodes) and in general is transient in nature. Additionally, what RefCell guards here are essentially caches. From the consumers pov, this looks like an immutable data structure. And &T APIs are much more convenient to use than &mut T APIs.

So should we use RefCell<FxHashMap<...>> here inside of SourceBinder instead?https://github.com/rust-analyzer/rust-analyzer/blob/1fe48a0115c24240f5a3e1b329e642f18e2715d6/crates/ra_hir/src/source_binder.rs#L23-L26

matklad · 2020-02-25T10:18:25Z

Yeah, both sa and sb should just go away as a result of this refactoring .

…

On Tuesday, 25 February 2020, Veetaha ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/ra_hir/src/semantics.rs <#3222 (comment)> : > +use std::{cell::RefCell, fmt, iter::successors}; + +use ra_db::{FileId, FileRange}; +use ra_syntax::{ast, AstNode, SyntaxNode, SyntaxToken, TextRange}; +use rustc_hash::{FxHashMap, FxHashSet}; + +use crate::{ + db::HirDatabase, source_analyzer::ReferenceDescriptor, source_binder::ToDef, Function, + HirFileId, InFile, MacroDef, Module, Name, Origin, PathResolution, ScopeDef, SourceAnalyzer, + SourceBinder, StructField, Type, VariantDef, +}; +use hir_def::TraitId; + +pub struct Semantics<'db, DB> { + pub db: &'db DB, + sb: RefCell<SourceBinder<'db, DB>>, So should we use RefCell<FxHashMap<...>> here inside of SourceBinder instead?https://github.com/rust-analyzer/rust-analyzer/blob/ 1fe48a0/crates/ra_hir/src/ source_binder.rs#L23-L26 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3222>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3MZMPFFVNMLFQEXOVKDRETV2VANCNFSM4KXJFIGQ> .

matklad · 2020-02-25T10:56:10Z

Another interesting API i've discovered during the work: SemanticScope, which encapsulates scoping info at the given point of code. resolving ast::Path works with Semantics, resolving hir::Path needs a SemanticsScope.

kiljacken · 2020-02-25T11:05:57Z

@matklad SemanticScope seems like it would play nicely with local imports, so 👍 for that

flodiebold · 2020-02-25T12:05:02Z

Another interesting API i've discovered during the work: SemanticScope, which encapsulates scoping info at the given point of code. resolving ast::Path works with Semantics, resolving hir::Path needs a SemanticsScope.

Yeah, that sounds like a good solution 🤔

Veetaha · 2020-02-25T12:12:16Z

crates/ra_assists/src/assist_ctx.rs

 pub(crate) struct AssistCtx<'a> {
+    // FIXME: Rc does not make much sense here
+    pub(crate) sema: Rc<Semantics<'a, RootDatabase>>,


It made sense when you put it here or what did you want to say?

matklad · 2020-02-25T17:13:14Z

I think this is ready for review @flodiebold !

The public API looks good to me: we now have a single Semantics type, which handles all the things. We also don't really use HirFileId in public API anymore, except for HasSource trait.

The implementation is unfortunatelly still split between sa, sb and semantics :(

flodiebold

It looks really nice! Much more straightforward than before.

flodiebold · 2020-02-25T18:48:49Z

crates/ra_assists/src/handlers/fill_match_arms.rs

-
-    let result = expr_ty.autoderef(db).find_map(|ty| match ty.as_adt() {
+fn resolve_enum_def(sema: &Semantics<RootDatabase>, expr: &ast::Expr) -> Option<hir::Enum> {
+    sema.type_of_expr(&expr)?.autoderef(sema.db).find_map(|ty| match ty.as_adt() {


(btw, I think the use of autoderef isn't quite correct here -- since this is about match, we should have something that just strips references. We don't want to apply the Deref trait, for example)

flodiebold · 2020-02-25T18:55:33Z

crates/ra_hir/src/semantics.rs

+    }
+
+    fn analyze2(&self, src: InFile<&SyntaxNode>, offset: Option<TextUnit>) -> SourceAnalyzer {
+        let _p = profile("SourceBinder::analyzer");


Suggested change

let _p = profile("SourceBinder::analyzer");

let _p = profile("Semantics::analyze2");

?

flodiebold · 2020-02-25T18:56:54Z

crates/ra_hir/src/semantics.rs

+        let file_id = self.lookup(&root_node).unwrap_or_else(|| {
+            panic!(
+                "\n\nFailed to lookup {:?} in this Semantics.\n\
+                 Make sure to use only query nodes, derived from this instance of Semantics.\n\


this requirement is the only thing that worries me. I guess we'll see how problematic it is in practice.

Yeah... A path forward is to add that u32 to rowan, as we originally planned, and add an interned query for TreeProvenance or some such.

flodiebold · 2020-02-25T19:00:08Z

crates/ra_hir/src/semantics.rs

+    }
+
+    /// Note: `FxHashSet<TraitId>` should be treated as an opaque type, passed into `Type
+    // FIXME: rename to visible_traits to not repeat scope?


seems a better name, yeah

edwin0cheng · 2020-02-26T03:38:28Z

crates/ra_syntax/src/algo.rs

@@ -56,6 +56,11 @@ pub fn find_covering_element(root: &SyntaxNode, range: TextRange) -> SyntaxEleme
    root.covering_element(range)
 }

+pub fn least_common_ancestor(u: &SyntaxNode, v: &SyntaxNode) -> Option<SyntaxNode> {


This introduces the new type -- Semantics. Semantics maps SyntaxNodes to various semantic info, such as type, name resolution or macro expansions. To do so, Semantics maintains a HashMap which maps every node it saw to the file from which the node originated. This is enough to get all the necessary hir bits just from syntax.

matklad · 2020-02-26T11:57:53Z

bors r+

kjeremy reviewed Feb 18, 2020

View reviewed changes

crates/ra_ide/src/goto_type_definition.rs Outdated Show resolved Hide resolved

kjeremy reviewed Feb 18, 2020

View reviewed changes

matklad force-pushed the identity branch 4 times, most recently from 48ce3ad to ca119f4 Compare February 19, 2020 18:00

matklad force-pushed the identity branch 7 times, most recently from ac91f9c to 3492bb5 Compare February 24, 2020 16:41

Veetaha reviewed Feb 25, 2020

View reviewed changes

matklad force-pushed the identity branch from 2dd2caf to 9bcc744 Compare February 25, 2020 10:15

Veetaha reviewed Feb 25, 2020

View reviewed changes

matklad force-pushed the identity branch 8 times, most recently from 957b2f3 to 1cbd839 Compare February 25, 2020 17:09

matklad force-pushed the identity branch from 1cbd839 to 2f148db Compare February 25, 2020 17:11

matklad changed the title ~~Nth iteration of expression analysis API~~ Introduce Semantics API Feb 25, 2020

matklad force-pushed the identity branch from 2f148db to 75f6015 Compare February 25, 2020 17:21

flodiebold approved these changes Feb 25, 2020

View reviewed changes

edwin0cheng reviewed Feb 26, 2020

View reviewed changes

edwin0cheng mentioned this pull request Feb 26, 2020

Add recursive macro support in original_range #3314

Merged

matklad force-pushed the identity branch from 75f6015 to 03b2b86 Compare February 26, 2020 11:54

matklad force-pushed the identity branch from 03b2b86 to c3a4c44 Compare February 26, 2020 11:55

matklad merged commit 5c64ad2 into rust-lang:master Feb 26, 2020

matklad deleted the identity branch February 26, 2020 12:04

matklad mentioned this pull request Mar 2, 2020

Design: syntax trees vs identity #3129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Semantics API #3222

Introduce Semantics API #3222

matklad commented Feb 18, 2020

kjeremy Feb 18, 2020 •

edited

Loading

kjeremy commented Feb 18, 2020

matklad commented Feb 19, 2020

Veetaha Feb 25, 2020

matklad Feb 25, 2020

Veetaha Feb 25, 2020

matklad commented Feb 25, 2020 via email

matklad commented Feb 25, 2020

kiljacken commented Feb 25, 2020 •

edited

Loading

flodiebold commented Feb 25, 2020

Veetaha Feb 25, 2020

matklad commented Feb 25, 2020

flodiebold left a comment

flodiebold Feb 25, 2020

flodiebold Feb 25, 2020

flodiebold Feb 25, 2020

matklad Feb 26, 2020

flodiebold Feb 25, 2020

edwin0cheng Feb 26, 2020

matklad commented Feb 26, 2020

	let _p = profile("SourceBinder::analyzer");
	let _p = profile("Semantics::analyze2");

Introduce Semantics API #3222

Introduce Semantics API #3222

Conversation

matklad commented Feb 18, 2020

kjeremy Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

kjeremy commented Feb 18, 2020

matklad commented Feb 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Feb 25, 2020 via email

matklad commented Feb 25, 2020

kiljacken commented Feb 25, 2020 • edited Loading

flodiebold commented Feb 25, 2020

Choose a reason for hiding this comment

matklad commented Feb 25, 2020

flodiebold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Feb 26, 2020

kjeremy Feb 18, 2020 •

edited

Loading

kiljacken commented Feb 25, 2020 •

edited

Loading