On Sunday, 24 November 2013 at 15:55:25 UTC, ilya-stromberg wrote:
> On Sunday, 24 November 2013 at 15:39:20 UTC, Maxim Fomin wrote:
>
>> No way compiler could guess that you have bool f in mind.
>
> Sorry if I didn't explain the example properly.
>
> The `bool f` variable was in my code and it works correctly, but compiler didn't put attention that I have identifiers shadowing issue. After big code refactoring I wrongly commented the `bool f` variable.

OK, root of the issue is identifier shadowing.

>
> So, nothing happens if compiler provides identifier shadowing error - I'll just rename variable. Is it possible to implement?

Yes, it is possible, but AFAIK Walter is opposite because he 1) thinks shadowing is OK (and taking into account compability with C, this is dabatable), 2) does not like warnings.

On 11/24/13 6:32 AM, ilya-stromberg wrote:
> On Sunday, 24 November 2013 at 14:16:39 UTC, bearophile wrote:
>> Maxim Fomin:
>>
>>> This is neither bug not a terribale feature.
>>
>> I think the implicit question of ilya-stromberg was: how much
>> bug-prone is this language feature?
>
> Yes, exactly. I personally was VERY surprised. My code example from real
> life:
>
> class Foo
> {
> }
>
> class Bar
> {
>      Foo f;
>
>      void bar()
>      {
>          //variable was wrongly commented here
>          //bool f = true;
>
>          if(f)
>          {
>              //Oops!
>          }
>      }
> }

If uncommented that would make for a shadow definition error. In brief I think you'll have a hard time finding evidence that "if (p)" is dangerous.


Andrei

Andrei Alexandrescu:

>> class Foo
>> {
>> }
>>
>> class Bar
>> {
>>     Foo f;
>>
>>     void bar()
>>     {
>>         //variable was wrongly commented here
>>         //bool f = true;
>>
>>         if(f)
>>         {
>>             //Oops!
>>         }
>>     }
>> }
>
> If uncommented that would make for a shadow definition error.

Are you saying this code gives or should give errors?


class Foo {}
class Bar {
     Foo f;
     void bar() {
         bool f = true;
         if (f) {}
     }
}
void main() {}


Bye,
bearophile

On 11/24/13 9:39 AM, bearophile wrote:
> Are you saying this code gives or should give errors?
>
>
> class Foo {}
> class Bar {
>       Foo f;
>       void bar() {
>           bool f = true;
>           if (f) {}
>       }
> }
> void main() {}

Sorry, I misread the example - thought it's a function local variable. I'm not sure whether shadowing globals or members would be a good idea. gcc has a -W flag for that, and someone tried to turn it on at Facebook but with debatable results. We ended up not using that warning.

Andrei

On 11/24/2013 9:45 AM, Andrei Alexandrescu wrote:
> Sorry, I misread the example - thought it's a function local variable. I'm not
> sure whether shadowing globals or members would be a good idea. gcc has a -W
> flag for that, and someone tried to turn it on at Facebook but with debatable
> results. We ended up not using that warning.

Shadowing globals is definitely a bad idea. Shadowing members, it's debatable. In any case, I don't think it is some sort of terrible bug generator. In fact, I'll often write:

struct S {
    int m;
    this(int m) { this.m = m; }
}

because I like to make it clear I am initializing m. I find code like this to be unduly wretched:

struct S {
    int m;
    this(int _m) { m = _m; }
}

Andrei Alexandrescu:

> I'm not sure whether shadowing globals or members would be a good idea.

I understand. It's not a clear cut topic. There are real reasons both for and against this idea. But surely it should be considered. I have not yet (re)opened an enhancement request on this.


> gcc has a -W flag for that, and someone tried to turn it on at Facebook but with debatable results. We ended up not using that warning.

In the example I have shown it's with() that is uncovering a struct field with the same name of a module-level name. So it's not exactly the same as a local variable shadowing a module-level name (and currently with has anti-hijacking for local variables). I think that shadowing module-level names with with() is not good. Also because of this idea:
https://d.puremagic.com/issues/show_bug.cgi?id=6917

Regarding Facebook, I presume that Gcc flag was applied to plenty of already written code. If you apply it since the start of a new project perhaps (probably) its effects are different. This detail is important, because lot of D code is yet to be written.

Regarding more generally the topic of shadowing module-level names with local names, the attribute "pure" helps avoid some cases, because you can only shadow immutable global names, and this is a little less dangerous/troublesome.

Time ago I suggested an optional @outer() attribute, that's useful to specify what names from outer scopes a function/method is allowed to see and use. It looks a bit excessive, but it shows that I have hated bugs caused by shadowing globals silently.

An IDE could underline the shadowing variables, turning the silent shadowing into a visible one, without giving warnings (it's a kind of much noisy warning), but unfortunately a compiler like dmd can't do that.

Another problem is that a command like "import std.algorithm" imports lot of names in the current module, and this could cause many false alarms. But this could be a good thing, and it can push Phobos devs to add more "private" tags to Phobos names, and D developers to add more qualified imports in their programs.

Bye,
bearophile

On 11/24/13 10:31 AM, bearophile wrote:
> Regarding Facebook, I presume that Gcc flag was applied to plenty of
> already written code. If you apply it since the start of a new project
> perhaps (probably) its effects are different. This detail is important,
> because lot of D code is yet to be written.

This is just speculation. It would have been helpful if enabling -Wshadow uncovered at least a few bugs. Instead, it just asked for a lot of code to be changed for no visible benefit.

Andrei

On Sunday, 24 November 2013 at 18:23:51 UTC, Walter Bright wrote:
> Shadowing globals is definitely a bad idea. Shadowing members, it's debatable. In any case, I don't think it is some sort of terrible bug generator. In fact, I'll often write:
>
> struct S {
>     int m;
>     this(int m) { this.m = m; }
> }

We can allow shadowing members only for function parameters or, maybe, only for constructor. Probably, your example is the most often case usage of shadowing members. In other hand, I can always rename local function variable because it isn't part of public API and should not confuse any users.

On Sunday, 24 November 2013 at 18:41:32 UTC, Andrei Alexandrescu wrote:
> This is just speculation. It would have been helpful if enabling -Wshadow uncovered at least a few bugs. Instead, it just asked for a lot of code to be changed for no visible benefit.

No, isn't just speculation.
If you already have working and tested code without `-Wshadow` flag it's really doesn't provide any solid benefits.
But in my case it helps because compiler prints error about missing identifier and I have to uncomment `bool` variable.

Walter Bright:

> Shadowing globals is definitely a bad idea. Shadowing members, it's debatable.

So are you saying D here should give an error for the shadowing of the module-level x?


struct Foo { int x; }
int x;
void main() {
    Foo f;
    with (f) {
        x++;
    }
}




> In any case, I don't think it is some sort of terrible bug generator. In fact, I'll often write:
>
> struct S {
>     int m;
>     this(int m) { this.m = m; }
> }
>
> because I like to make it clear I am initializing m.

From my experience that's quite bug-prone:
http://d.puremagic.com/issues/show_bug.cgi?id=3878

Bye,
bearophile