Give more helpful error when shadowing `const`

[IGI-111]

Shadowing constants is forbidden, but the compiler will currently give an unhelpful error message like so:

The name `VAL` is defined multiple times

We should give a clearer error message in that particular case.

[ironcev]

Hi, this looks like a great first issue, not only a good one :-) I would love to contribute to it. I have two questions, before starting with the implementation. This is the TL;DR version of the questions. I will also provide a longer one in the comment below. The longer one is more for me, and as a checklist once the feature is implemented. I've started diving deeper into Sway to get a better feeling for the language and the design decisions already done. So I systematically piled up all the cases realted to the shadowing consts.

TL;DR

(The terms item-style shadowing and sequential shadowing have the meaning defined in the Rust RFC 116 (No module shadowing).)

:question: Should the new message apply only to sequential shadowing, and for the item-style shadowing we keep the existing one?

This is how I understand the motivation behind this issue - to give hint to the programmer that unlike variables consts cannot be shadowed on the same or nested scope level.

Having a separate error message for const in case of item-style shadowing makes no sense for me, because I don't see why would const be an exceptional item and deserve an error message different then other items.

Applied to the shadowed consts and vars tests this would mean the following test output:

// const shadowing an imported const
use lib::X;
const X = 6; // <existing error> The name `X` is defined multiple times

// const shadowing a local const
const Y = 7;
const Y = 8; // <existing error> The name `X` is defined multiple times
fn main() {
    // variable shadowing an imported const
    let X = 9; // <new error>

    // variable shadowing a const
    const Z = 3;
    let Z = 4; // <new error>

    // const shadowing a variable
    let W = 2;
    const W = 1; // <new error>

    // Variable shadowing a variable - this is okay!
    let P = 7;
    let P = 8;
}

The detailed reasoning is in the comment below.

:question: How should the new error message exactly look like?

I have difficulties formulating a good single message that would cover all three cases. E.g.:

The name `VAL` shadows a constant or a variable with the same name.

Or should we go for three different error messages? E.g.:

The constant `VAL` shadows the constant of the same name.
The constant `VAL` shadows the variable of the same name.
The variable `VAL` shadows the constant of the same name.

[ironcev]

As explained in the comment above, the detailed list of item-style shadowing and sequential shadowing cases possible in Sway. I hope I didn't miss something :-) Parts marked bold are interesting to look at.

The definition of the item-style shadowing and sequential shadowing from the Rust RFC 116 (No module shadowing):

Shadowing While the principle of shadowing exists in all namespaces, there are different forms of it:

• item-style: Declarations shadow names from outer scopes, and are visible everywhere in their own, including lexically before their own definition. This requires there to be only one definition with the same name and namespace per scope level. Types, modules, item values and lifetimes fall under these rules.
• sequentially: Declarations shadow names that are lexically before them, both in parent scopes and their own. This means you can reuse the same name in the same scope, but a definition will not be visibly before itself. This is how local values and macros work. (Due to sequential code execution and parsing, respectively)

Sequential shadowing -> report the new error message

• [x] Shadowing constants on the same scope level

fn f() -> () {
    const X: u32 = 2; 
    const X: u32 = 2; // <new error>
}

• [x] Shadowing constants from the parent scope

const X: u32 = 1;
fn f() -> () {
    const X: u32 = 2; // <new error>
    {
        const X: u32 = 3; // !! Currently no error message here.
     }
}

• [x] Other examples from the fun main() in shadowed_consts_and_vars tests

// ...
fn main() {
    // variable shadowing an imported const
    let X = 9;
        
    // variable shadowing a const
    const Z = 3;
    let Z = 4;
        
    // const shadowing a variable
    let W = 2;  
    const W = 1;
   
     // ...
}

Here, the interesting case is perhaps this one:

const X: u32 = 1;
fn f() -> () {
    let X: u32 = 2; // Currently error: The name `X` is defined multiple times but will be <new error>
    {
         let X: u32 = 3; // !! Currently no error message here.
     }
}

Right now, only the first shadowing produces an error. The second is obviously interpreted as a valid shadowing of a variable. This looks fine to me and I assume it should stay that way.

Item-style shadowing -> proposal is to keep the current error message

Why?

Module constants in Sway like in Rust are visible in the whole module scope no matter where they are defined. E.g., this compiles without errors:

fn before_x() -> () {
    poke(X);
}
    
const X: u32 = 1;

Which is, unlike Rust this time ;-), not the case for inner scopes:

pub fn play() -> () {
    poke(X); // ERR: Variable "X" does not exist in this scope.
    
    const X: u32 = 2;
}

Means consts in top scope behave same as the other items in terms of visibility. And for all other items we use the existing error message:

struct SomeItem { }
enum SomeItem { } // ERR: The name `SomeItem` is defined multiple times

So I don't see why treat consts differently in this regard and report a different message:

const SOME_CONST = 1;
const SOME_CONST = 2; // ERR should stay the same like for other items.

const is a bit of a maverick considering that it can clash only with other consts, e.g., this compiles:

struct X { }
const X = 1;

But this IMO does not justify a special treatment in case of a name clash error.

• [x] Module const "shadows" other module const

const SOME_CONST = 1;
const SOME_CONST = 2; // ERR: The name `SOME_CONST` is defined multiple times

• [x] Module const "shadows" imported const

use lib::SOME_CONST;
const SOME_CONST = 2; // ERR: The name `SOME_CONST` is defined multiple times

Associated constants -> keep the current error message

For structs and traits. E.g.:

impl Struct {
    const X: u32 = 1;
    const X: u32 = 2; // ERR: The name `X` is defined multiple times
}

Update documentation

• [ ] Unify terminology in the SPL Book and the Ref Book. E.g., impl self consts.

• [ ] In the Ref Book chapter on shadowing add a note that consts cannot be shadowed.

[IGI-111]

I think the reasoning here is sound, the new error seems indeed most appropriate for sequential shadowing.

I think it's probably clearer to go with distinct error messages for each case, but It might also be worth it restating the rule that constants cannot be shadowed as part of each message so as to make it clear why this is erroneous.

[ironcev]

Great, thanks for clarification! I'll proceed accordingly.

[ironcev]

Another one edge-case to consider. use importing has completely different error message at the moment:

The name "LIB_X" shadows another symbol with the same name.

In the case of item-style shadowing this message should stay the same. But when we use identifiers within functions, importing acts sequentially. Applying the above logic we would have the following:

use libA::X;
use libB::X; // <existing error> The name "X" shadows another symbol with the same name.

fn f() -> () {
    use libC::X;
    use libD::X; // <new error>
}

Does this sound good or I missed some detail?

Side note: we already have a few similar error messages for multiple definitions on names, specific messages for multiple imported definitions, an even a specific error message for multiple definitions of constants ;-) I see a potential for a good clean up here. I'll open a separate issue for that.

[ironcev]

It turned out that in also in this case, there is no error message at the moment for this case, same like in the comment above with lets:

const X: u32 = 1;
fn f() -> () {
    const X: u32 = 2; // <new error>
    {
        const X: u32 = 3; // !! Currently no error message here.
     }
}

Since this issue deals only with turning the existing error messages into more helpful once, we will keep not having the error in this case.