Forbid duplicated function definition

[Champii]

We currently don't fail if a function is defined twice.
We must.

[oraqlle]

Marked in code. I will have a look tomorrow.

[oraqlle]

Potential Solution

Use the experimental ::HashMap::try_insert(). Like insert but fails if the key already exists rather then over-writing the old value. Could be helpful for error messages as well as if the key already exists it returns an error with details of what happened, conveying duplication of function declaration.

Feature activation via: #![feature(map_try_insert)].

• Rust Doc
• Tracked Issue from Rust Repository

[Champii]

I think a simple HashMap::insert() would suffice, as it returns an Option<T> being Some(old_value) if the key existed. We don't need to care if the new value override the old, as we would want to return an error here and stop the process.

[oraqlle]

But HashMap::insert() overrides the old value meaning if you declare a function foo twice, the second is going to be the one used by the program. The HashMap::try_insert means if a function already exists with that name, it will fail and return an error context with the existing value, the key used and the attempted value trying to replace the existing. However the feature is unstable and can't really be used.

A map.entry(K).or_insert(new_val) pattern could work as it checks if the entry exists at the key (K) and inserts new_val it if doesn't. Looking into it atm.

[Champii]

You rightfully changed my mind a while back, when you proposed to have a compiler that runs on rust stable, and I still think that we should stick to it now that it is nightly-feature-free :)

Also, I'm not very comfortable with returning a &mut V when we can override the value anyway and check the returned Option to return a hard error. We don't really want to act on existing values mutably.
Like I said, we don't care about replacing the existing value, as the whole program is declared invalid by this construct and should not be analyzed further (at least not in the current state of the compiler). When the insert returns a Some, we add an error Diagnostic to the parsing context (a new DiagnosticKind should be created for the occasion), that will automatically stop the compilation and print it to the stderr.

We could use the entry(k).or_insert(v) pattern but it also returns a &mut that don't makes sense if we don't intend to act on the existing value.

[oraqlle]

Fair enough and I agree with not wanting to use experimental features, particularly unstable ones. We could compare the insert methods return value and match it to see if it is not None because we only want new keys and HashMap::insert returns None if the key didn't exist or Some(old_value) if it did and thus any Some(_) would be invalid.
i.e

/// In the scopes.rs for Scopes type

pub fn add(&mut self, s: K, val: T) -> std::result::Result<(), String> {
        match self.scopes.last_mut().unwrap().insert(s, val) {
            Some(_) => { Err("Key already exists!".to_string()) },
            None => { Ok(()) }
        }
    }

Just not sure how to cascade the error properly.

[oraqlle]

Checkout the WIP I have for #150

[Champii]

I've checked it out, It's nice !
I think you have created a branch from master, that is the release/stable branch. For anything you should start from develop that has all the last changes :) (There is #127 that should fix/clarify that)

You can take example from the fn visit_identifier() on how to add a diagnostic to the context.

I realize now that the resolver does not abort the analysis right away when an error diagnostic is pushed, but still try to resolve the rest of the program before printing the diags and halting, to maximize the fixes a developer could do before re-running the compiler again.
This is totally fine by me, and that means you just have to push the error diagnostic in the parsing context and you are basically done :)