Shadowing of variables

[MerlinEgalite]

Kickstarting a discussion for a general solution for the shadowing of variables.

Here are a few options already mentioned:

• Do nothing
• Add Mem to the end of a var.
• Adding an "_" to the beginning or the end of a var.

[Rubilmax]

@QGarchery also suggested (#1545 (comment)):

• append cached suffix

I would suggest:

• $ prefix for local, shadowing variables

[QGarchery]

I don't like the $ at all, it breaks my IDE assumptions

The IDE makes wrong assumptions in this case, as the $ sign is in the regexp defining identifiers. This problem could probably be solved by raising an issue on the IDE side. That being said I think we both use vscode and I agree that this is annoying :/

[QGarchery]

I like prefixing with _ as I think the occurrences of needing both a local variable and input variable of the same name are pretty rare. It would also make our code look less like a christmas tree, with different weird symbols hooked all over the place :D

[pakim249CAL]

I don't think that case is very rare in our case. For example, take the entryPositionsManager set in the Morpho storage contracts.

This storage variable is currently named entryPositionManager. If it was made internal, this would be _entryPositionsManager. This would clash with the input variable in a setter, as the natural way to create a setter would be:

setEntryPositionsManager(address _entryPositionsManager)

So in this case, we can either use something like newEntryPositionsManager or _newEntryPositionsManager. Of those two options, I think I would prefer the former.

[QGarchery]

One of the reason input parameters are prefixed with _ is so that they don't clash with storage variables. So I don't think we want any storage variable (internal or not) to be prefixed with _
What do you think @morpho-dao/solidity @MathisGD ?

[pakim249CAL]

It can be known that input parameters if we do not add prefixes to these, would not clash with storage variables if ALL storage vars and internal functions are prefixed. So I am proposing actually that we abandon our practice of prefixing input variables.

[QGarchery]

The only problem I see with shadowing variables is that state variables may not be changed (because changes will apply to the shadowing variable), whereas the developer might expect changes . If the shadowing variable is not of the same type then I think it is fine to shadow, because the compiler will not accept wrong code. Do we agree that it is ok to shadow a state variable name with another variable of a different type ?

[QGarchery]

I'm not worried about the compiler as he's doing a better job than humans. I'm more worried about humans having a harder time reviewing the code

I'm suggesting to rely more on the compiler, so do that mean you agree with my suggestion ?

[MerlinEgalite]

I think that we should first avoid declaring 2 variables with the same name even if their type differs.

[QGarchery]

Ok, then I guess I don't understand the link between this discussion and the comparison you make between humans and compilers :D

I think it would be nice to have a second opinion on this from another reviewer

[MerlinEgalite]

I'm saying that the issue with shadowing is not because of the compiler but because it's harder for the human to read and it can be misleading too.

[QGarchery]

Personally I like to have short names, and if we are backed by the compiler in doing so (because it won't accept using the shadowing variable with the wrong type), it seems like a good idea to allow to keep the same name