Bind variable declaration to symbol #9

helixbass · 2021-12-21T19:55:35Z

In this PR:

create symbol table and populate with a symbol corresponding to a variable declaration during binding

To test:
If you have a file named eg tmp.tsx in the repo root directory whose contents are const a: number = true and run cargo run tmp.tsx you should see debug-logging of the (pre-binding) SourceFile, then a post-binding: debug-logging of the SourceFile where the SourceFile node's locals is a symbol table mapping from the __String "a" to a Symbol with escaped_name: "a" and declarations a Vec containing the VariableDeclaration AST node that declared a, and the symbol on the BaseNode inside that VariableDeclaration should be a populated weak reference (to the Symbol) and then it should panic during type-checking

Based on variable-declaration

helixbass · 2021-12-21T19:58:35Z

src/compiler/binder.rs

@@ -1,12 +1,15 @@
 #![allow(non_upper_case_globals)]

 use bitflags::bitflags;
+use parking_lot::RwLock;


It kept feeling like it'd make sense to be able to "map" the contents of a RwLockReadGuard/RwLockWriteGuard so that a method could return eg a RwLockReadGuard wrapping some nested field of the thing the RwLock is wrapping. Per rust-lang/rust#44189, the standard library's RwLock doesn't support this but the parking_lot crate does. So I swapped in parking_lot's RwLock

helixbass · 2021-12-21T19:59:44Z

src/compiler/binder.rs

+        self.container.try_read().unwrap().as_ref().unwrap().clone()
+    }
+
+    fn maybe_container(&self) -> Option<Rc<Node>> {


Using maybe_*() naming convention for when I'm returning an Option vs returning the unwrapped field

helixbass · 2021-12-21T20:06:29Z

src/compiler/binder.rs

+
+    fn add_declaration_to_symbol(&self, symbol: Rc<Symbol>, node: Rc<Node /*Declaration*/>) {
+        node.set_symbol(symbol.clone());
+        let declarations = {


The block scoping is to ensure that the RwLockReadGuard returned by symbol.maybe_declarations() is dropped before we call .set_declarations() (which will want write access to the RwLock)

This whole pattern is a little wonky from a Rust perspective - appendIfUnique() mutates the passed array if it exists or else returns a new array, but since I can't give it ownership of the underlying existing symbol.declarations Vec it'd have to take an &mut to the Vec in order to avoid creating a new Vec, but that's not compatible with the passed-None-and-creates-new-Vec use case (since it'd have to return a full-fledged new Vec) (is this somewhere I could use Cow?)?

Hmm, I don't really have any better ideas than what you've done.

helixbass · 2021-12-21T20:08:39Z

src/compiler/binder.rs

+    }
+
+    fn add_declaration_to_symbol(&self, symbol: Rc<Symbol>, node: Rc<Node /*Declaration*/>) {
+        node.set_symbol(symbol.clone());


Symbol's get stored in a symbol table but also as a field on Node's, so I'm wrapping them in Rc's and storing a weak reference in BaseNode's symbol field

helixbass · 2021-12-21T20:10:12Z

src/compiler/binder.rs

+            Some(name) => {
+                if true {
+                    symbol = Some(Rc::new(self.create_symbol(SymbolFlags::None, name.clone())));
+                    symbol_table.insert(name, symbol.as_ref().unwrap().clone());


This is the first time I'd used type aliasing in Rust I think, pretty slick how I can just call methods like .insert() (SymbolTable is aliased to a certain type of HashMap)

helixbass · 2021-12-21T20:13:59Z

src/compiler/parser.rs

    cb_nodes: TNodesCallback,
 ) {
    if node.kind() <= SyntaxKind::LastToken {
        return;
    }
    match &*node {
+        Node::VariableDeclaration(variable_declaration) => {
+            visit_node(&mut cb_node, Some(variable_declaration.name()));


The compiler was complaining that this call of visit_node() takes ownership of the cb_node closure argument (and then I try and call visit_node() with it again right after this), which I guess makes sense, so I modified visit_node() to take a reference to a closure instead of a closure

helixbass · 2021-12-21T20:16:07Z

src/compiler/utilities.rs

+
+pub fn get_escaped_text_of_identifier_or_literal(node: Rc<Node>) -> __String {
+    if is_member_name(&*node) {
+        node.as_member_name().escaped_text()


I like this node.as_*() pattern, eg I exposed a .as_member_name() method on Node that panics unless it can unwrap the Node into one of the AST node sub-variants that implements MemberNameInterface (and it returns a &dyn MemberNameInterface). It's a little bit like what I was thinking of doing via a macro (eg unwrap_ast_node!(node, BinaryExpression)) for unwrapping to concrete AST node types but for unwrapping to a certain AST node interface instead

helixbass · 2021-12-21T20:22:24Z

src/compiler/types.rs

@@ -203,6 +302,8 @@ pub struct BaseNode {
    pub parent: RwLock<Option<Weak<Node>>>,
    pub pos: AtomicUsize,
    pub end: AtomicUsize,
+    pub symbol: RefCell<Option<Weak<Symbol>>>,


I started wondering why I was using RwLock instead of RefCell (since it seems like you see Rc<RefCell<...>>'s more than Rc<RwLock<...>>'s). I'm not totally sure but I think I can replace a lot of the RwLock's with RefCell's and that per eg this article RefCell's sound cheaper/faster

Ah ok, yeah I had no idea that RefCell was just the single-threaded version of RWLock. (The naming doesn't really help there.)

helixbass · 2021-12-21T20:23:28Z

src/compiler/types.rs

+    }
+
+    fn locals(&self) -> MappedRwLockWriteGuard<SymbolTable> {
+        RwLockWriteGuard::map(self.locals.try_write().unwrap(), |option| {


Here's a usage of the RwLockWriteGuard::map() that parking_lot provides

peterstuart · 2021-12-22T12:32:15Z

src/compiler/binder.rs

+
+    fn add_declaration_to_symbol(&self, symbol: Rc<Symbol>, node: Rc<Node /*Declaration*/>) {
+        node.set_symbol(symbol.clone());
+        let declarations = {


Hmm, I don't really have any better ideas than what you've done.

peterstuart · 2021-12-22T12:33:25Z

src/compiler/binder.rs

+            } else {
+                None
+            };
+        }


This looks like it could be expressed with and_then (flat map) instead of an if-statement, but maybe isn't compatible with whatever will replace unimplemented!.

Does a return inside a closure body return from the enclosing function (like a Ruby block) or just from the closure (relevant here because this block that would go inside .and_then() needs to return from the function)?

Just from the closure

peterstuart · 2021-12-22T12:35:24Z

src/compiler/core.rs

+        array
+    } else {
+        vec![to_add]
+    }


I think this could be written using one of the map_or* functions.

peterstuart · 2021-12-22T12:41:08Z

src/compiler/types.rs

@@ -203,6 +302,8 @@ pub struct BaseNode {
    pub parent: RwLock<Option<Weak<Node>>>,
    pub pos: AtomicUsize,
    pub end: AtomicUsize,
+    pub symbol: RefCell<Option<Weak<Symbol>>>,


Ah ok, yeah I had no idea that RefCell was just the single-threaded version of RWLock. (The naming doesn't really help there.)

helixbass added 6 commits December 21, 2021 11:04

use parking_lot

bfa0328

get name

2414c10

creating symbol

a472df4

symbol fields

2c3543d

node symbol

00c2fba

symbol declarations

1ad34c4

helixbass commented Dec 21, 2021

View reviewed changes

peterstuart reviewed Dec 22, 2021

View reviewed changes

Base automatically changed from variable-declaration to master December 22, 2021 17:16

helixbass merged commit 404711b into master Dec 22, 2021

helixbass deleted the variable-binding branch December 22, 2021 17:29

Bind variable declaration to symbol #9

Bind variable declaration to symbol #9

Uh oh!

Conversation

helixbass commented Dec 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!