Skip to content

Commit

Permalink
[WIP] Forbid duplicated function definition #150
Browse files Browse the repository at this point in the history
None working example of new `Scopes::add` for context.
  • Loading branch information
oraqlle committed Jul 19, 2022
1 parent d614238 commit 692fa51
Show file tree
Hide file tree
Showing 6 changed files with 154 additions and 126 deletions.
26 changes: 18 additions & 8 deletions src/lib/helpers/scopes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,25 @@ where
None
}

pub fn add(&mut self, s: K, val: T) {
self.scopes.last_mut().unwrap().insert(s, val);
/// Note: Does not check for existing key, replaces old value
pub fn add(&mut self, s: K, val: T) -> std::result::Result<(), &str> {

This comment has been minimized.

Copy link
@Champii

Champii Jul 19, 2022

Owner

The Result here is nice, but we don't really care about a descriptive error at this point, we could just return the Option<T> from the insert and check if its a Some or None in the add_to_current_scope() method.

Note that we could use a Result with a nice Error, but that would mean to add an internal error just for that (I don't like to have a raw &str as error type)

In the end, this is up to you :)

This comment has been minimized.

Copy link
@Champii

Champii Jul 19, 2022

Owner

To go a little further, I'd say that returning the existing Option<T> (that is an Option<NodeId> in this context) would allow later to handle some pattern matching on function declaration like in Haskell, where we would need to get-and-replace an existing declaration with another.

This is quite not for tomorrow, so you should take that like a wish or a dream, not like an actual technical advice ^^

This comment has been minimized.

Copy link
@oraqlle

oraqlle Jul 20, 2022

Author Collaborator

I'll remove the Result, it was more conveyance rather than practicality. I will take a look add adding proper error diagnostic from Rocks API

match self.scopes.last_mut().unwrap().insert(s, val) {
Some(prev) => { Err("Key '{prev}', already exists!") },
None => { Ok(()) },
}
}

pub fn extend(&mut self, other: &Scope<K, T>) {
self.scopes.last_mut().unwrap().extend(other.clone())
}

/// `push` should take a scope object

This comment has been minimized.

Copy link
@Champii

Champii Jul 19, 2022

Owner

The whole point of the Scopes class is to hide the complexity of managing nested scopes. Maybe the name push is too obscure for what this method really do, i.e creating a new empty scope at the top of the stack.
If you have any idea about that, maybe something like push_empty or push_new ?

This comment has been minimized.

Copy link
@oraqlle

oraqlle Jul 20, 2022

Author Collaborator

That's mostly what I was noting. Many of the comments are just API design issues I had given the semantics of a ::push member function (idk what they're called in Rust). A ::push_empty I think would hold better but the _new suffix works as well.

This comment has been minimized.

Copy link
@Champii

Champii Jul 20, 2022

Owner

All in for the ::push_empty :)

pub fn push(&mut self) {
self.scopes.push(Scope::new())
}

/// `pop` should return the popped value
/// in a `Option<V>` or a `Result<V, E>`

This comment has been minimized.

Copy link
@Champii

Champii Jul 19, 2022

Owner

I agree 💯

This comment has been minimized.

Copy link
@Champii

Champii Jul 19, 2022

Owner

To stay in phase with the self.scopes I'd say we should directly return the Option<V> from the pop() method

This comment has been minimized.

Copy link
@oraqlle

oraqlle Jul 20, 2022

Author Collaborator

Same. We have the error handling capabilities might as well use them.

pub fn pop(&mut self) {
self.scopes.pop();
}
Expand All @@ -68,23 +75,26 @@ mod tests {
fn basic_scope() {
let mut scopes = Scopes::default();

scopes.add("a", 1);
scopes.add("b", 2);
assert_eq!(scopes.add("a", 1), Ok(()) );
assert_eq!(scopes.add("b", 2), Ok(()) );

assert_eq!(scopes.get("a").unwrap(), 1);
assert_eq!(scopes.get("b").unwrap(), 2);

/// Push new scope
scopes.push();

scopes.add("b", 4);
assert_eq!(scopes.add("b", 4), Ok(()) );

assert_eq!(scopes.get("a").unwrap(), 1);
assert_eq!(scopes.get("b").unwrap(), 4);

scopes.add("a", 3);
assert_eq!(scopes.add("a", 3), Ok(()) );

assert_eq!(scopes.get("a").unwrap(), 3);
assert_eq!(scopes.get("b").unwrap(), 4);
assert_eq!(scopes.get("a").unwrap(), 1);
assert_eq!(scopes.get("b").unwrap(), 2);

assert_eq!(scopes.add("a", 4), Err("Key 'a', already exists!"));

scopes.pop();

Expand Down
13 changes: 11 additions & 2 deletions src/lib/resolver/resolve_ctx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,14 @@ impl<'a> ResolveCtx<'a> {
}

/// TODO: GitHub Issue #150
/// Could use #![feature(map_try_insert)]
/// <<: Ensures unique entry into the map.
pub fn add_to_current_scope(&mut self, name: String, node_id: NodeId) {
if let Some(ref mut scopes) = self.scopes.get_mut(&self.cur_scope) {
scopes.add(name, node_id);
match scopes.add(name, node_id) {
Ok(_) => { },
Err(_err) => { unimplemented!{} },
}

This comment has been minimized.

Copy link
@Champii

Champii Jul 19, 2022

Owner

Here you could refactor the whole function body with a more functional approach:

if self.scopes.get_mut(&self.cur_scope).unwrap().add(name, node_id).is_err() {
  // push diagnostic
}

This comment has been minimized.

Copy link
@Champii

Champii Jul 19, 2022

Owner

The unwrap here is kind of acceptable, because the self.scopes should always contain the key self.cur_scope

This comment has been minimized.

Copy link
@oraqlle

oraqlle Jul 20, 2022

Author Collaborator

Forgot that, you are correct about the .unwrap() and I will fix that up. I was not aware of .is_err() so I will have a look at that but I think a match might work better for creating the error diagnostic but if we return the Option but that might be unnecessary as the error but be pushed by the call to .add().

}
}

Expand All @@ -36,10 +41,14 @@ impl<'a> ResolveCtx<'a> {
struct_scope_name.path.push(Identifier::new(struct_name, 0));

if let Some(ref mut scopes) = self.scopes.get_mut(&struct_scope_name) {
scopes.add(name.clone(), node_id);
match scopes.add(name.clone(), node_id) {
Ok(_) => { },
Err(_err) => { unimplemented!{} },
}
}
}


pub fn new_struct(&mut self, name: Identifier) {
let mut struct_scope_name = self.cur_scope.clone();
struct_scope_name.path.push(name);
Expand Down
4 changes: 4 additions & 0 deletions src/lib/testcases/fails/dupl_fun_decl/main.rk
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
foo: a, b = a + b
foo: a = a + 2

This comment has been minimized.

Copy link
@Champii

Champii Jul 19, 2022

Owner

This is the old syntax for function declaration, the proper syntax would be

foo: a, b -> a + b
foo: a -> a + 2

I've pushed a fix recently on some tests that failed because I forgot to update them after the big 0.4.0 syntax update. It is possible that you copied one of the few that has escaped me until now ? :)

This comment has been minimized.

Copy link
@oraqlle

oraqlle Jul 20, 2022

Author Collaborator

Right, thats all good. I could rebase the branch off the 0.4.0 work and continue from there.


main: -> foo 2, 5
1 change: 1 addition & 0 deletions src/lib/testcases/fails/dupl_fun_decl/main.rk.out
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-1
Empty file.
Loading

0 comments on commit 692fa51

Please sign in to comment.