After some refactoring I had a series of bugs that came from renaming class members so that the following situation was present.


class foo
{
  float a;

  void doSomething()
  {
    float a;
    a = a * 2.0;
  }
}

The local variable a shadows the member a after the refactoring and therefore this code will no longer work as expected. This was quite time consuming to track down. So I wanted to ask if we want to prevent that with a warning or even an error? D does not allow shadowing of local variables, so why is it allowed for members?

Kind Regards
Benjamin Thaut

On Wednesday, 9 January 2013 at 20:36:16 UTC, Benjamin Thaut wrote:
> After some refactoring I had a series of bugs that came from renaming class members so that the following situation was present.
>
>
> class foo
> {
>   float a;
>
>   void doSomething()
>   {
>     float a;
>     a = a * 2.0;
>   }
> }
>
> The local variable a shadows the member a after the refactoring and therefore this code will no longer work as expected. This was quite time consuming to track down. So I wanted to ask if we want to prevent that with a warning or even an error? D does not allow shadowing of local variables, so why is it allowed for members?
>
> Kind Regards
> Benjamin Thaut

I don't think an error will be appropriate, because this a can be a protected member of the super-super-super class and it won't be nice to be forced to use a different name. But another view on this is that it behaves consistently with the case in which a function parameter shadows a member.
I won't like this code:
class Pu {
   int a, b;
   this(int a, int b) {
       this.a = a;
       this.b = b;
   }
}

to issue warnings for a and b.

On Wednesday, 9 January 2013 at 20:36:16 UTC, Benjamin Thaut wrote:
> <skipped>
> The local variable a shadows the member a after the refactoring and therefore this code will no longer work as expected. This was quite time consuming to track down. So I wanted to ask if we want to prevent that with a warning or even an error? D does not allow shadowing of local variables, so why is it allowed for members?
>
> Kind Regards
> Benjamin Thaut

Because local variable is not a member. However I think this should be posted to Bugzilla as a bug or at least as an enhancement request.

On Wed, Jan 09, 2013 at 10:17:00PM +0100, comco wrote:
> On Wednesday, 9 January 2013 at 20:36:16 UTC, Benjamin Thaut wrote:
> >After some refactoring I had a series of bugs that came from renaming class members so that the following situation was present.
> >
> >
> >class foo
> >{
> >  float a;
> >
> >  void doSomething()
> >  {
> >    float a;
> >    a = a * 2.0;
> >  }
> >}
> >
> >The local variable a shadows the member a after the refactoring and therefore this code will no longer work as expected. This was quite time consuming to track down. So I wanted to ask if we want to prevent that with a warning or even an error? D does not allow shadowing of local variables, so why is it allowed for members?
> >
> >Kind Regards
> >Benjamin Thaut
> 
> I don't think an error will be appropriate, because this a can be a
> protected member of the super-super-super class and it won't be nice
> to be forced to use a different name. But another view on this is
> that it behaves consistently with the case in which a function
> parameter shadows a member.
> I won't like this code:
> class Pu {
>    int a, b;
>    this(int a, int b) {
>        this.a = a;
>        this.b = b;
>    }
> }
> 
> to issue warnings for a and b.

I used to write code like that too, but then I ran into some nasty ambiguity bugs, and now I'm in favor of prohibiting local variables shadowing class members. It's just *too* easy to make a mistake, and have the code write something to a local variable (which is lost upon scope exit) instead of a class member, or vice versa.

Add class template parameters to the mix, and it quickly becomes a minefield. I think this kind of shadowing should not be allowed.

Even if there's a protected member of a super-super-super class, I'd rather know about a name conflict than to write code like this:

	class B {
		protected int a=123;
	}

	class A : B {
		int f(int b) {
			//int a;	// <--- I forgot to write this line
			...
			a = b + 1;	// <---  Oops!
			...
			return a;
		}
	}

Because I forgot to declare 'a', I end up overwriting a base class member which I didn't intend to change. Not good. (Furthermore, if the class hierarchy is big, I may not find out about this until much later -- I may not even be aware that some superclass declares 'a'.)


T

-- 
Without geometry, life would be pointless. -- VS

On Wednesday, 9 January 2013 at 21:30:32 UTC, H. S. Teoh wrote:
> Because I forgot to declare 'a', I end up overwriting a base class
> member which I didn't intend to change. Not good. (Furthermore, if the
> class hierarchy is big, I may not find out about this until much later
> -- I may not even be aware that some superclass declares 'a'.)
>
>
> T

At least this will be consistent behaviour. If it is an error, then a perfectly fine code now will stop compiling after someone (evil person!) adds a protected member 'i' to one of the super-super-super classes. Now not only my loop will stop to compile, but the inheritance will have the nice side effect of not allowing to use i for iteration in __any__ method.
Of course, this is an extreme example, but valid.

On Wed, Jan 09, 2013 at 10:59:21PM +0100, comco wrote:
> On Wednesday, 9 January 2013 at 21:30:32 UTC, H. S. Teoh wrote:
> >Because I forgot to declare 'a', I end up overwriting a base class member which I didn't intend to change. Not good. (Furthermore, if the class hierarchy is big, I may not find out about this until much later -- I may not even be aware that some superclass declares 'a'.)
[...]
> At least this will be consistent behaviour. If it is an error, then
> a perfectly fine code now will stop compiling after someone (evil
> person!) adds a protected member 'i' to one of the super-super-super
> classes. Now not only my loop will stop to compile, but the
> inheritance will have the nice side effect of not allowing to use i
> for iteration in __any__ method.
> Of course, this is an extreme example, but valid.

Hmm, you're right. I withdraw my argument. :-P


T

-- 
I am not young enough to know everything. -- Oscar Wilde

On 1/9/2013 1:22 PM, Maxim Fomin wrote:
> Because local variable is not a member. However I think this should be posted to
> Bugzilla as a bug or at least as an enhancement request.

It is not a bug.

As for an enhancement request, I think comco has made some strong points against it.

On 1/9/13, Walter Bright <newshound2@digitalmars.com> wrote:
> It is not a bug.

Something related I want to ask you about:

struct S
{
    int _m;
    this(int m)
    {
        this._m = _m;  // meant "this._m = m;"
    }
}

I'd like to add a warning for identity assignments when it involves a parameter and a field of the aggregate which is the parent of the function. This was filed as http://d.puremagic.com/issues/show_bug.cgi?id=4407.

Personally I've had this happen to me quite a few times, and I've seen it reported on IRC by other people. We could add a check when warnings are turned on. Thought I'd get a taste of that "preapproved" tag ;).

On Wednesday, January 09, 2013 21:36:16 Benjamin Thaut wrote:
> After some refactoring I had a series of bugs that came from renaming class members so that the following situation was present.
> 
> 
> class foo
> {
> float a;
> 
> void doSomething()
> {
> float a;
> a = a * 2.0;
> }
> }
> 
> The local variable a shadows the member a after the refactoring and therefore this code will no longer work as expected. This was quite time consuming to track down. So I wanted to ask if we want to prevent that with a warning or even an error? D does not allow shadowing of local variables, so why is it allowed for members?

Personally, I would find it very annoying to not be able to shadow member variables, particularly with POD types. Stuff as simple as constructor parameters would become annoying

struct S
{
 this(int i)
 {
 this.i = i; //this shouldn't produce an error or warning
 }

 int i;
}

Local variables can't be shadowed by local variables, because there's no way to access the variable that's been shadowed. That's the only case where shadowing prevents you from accessing the shadowed variable, and it's the only case that's currently illegal.

If you want to avoid this sort of problem, then name your member variables differently (e.g. prepend _ to them). Then it generally doesn't even risk being an issue except with POD types where you don't make the members private (and therefore don't prepend _ to them).

- Jonathan M Davis

On Wednesday, January 09, 2013 23:22:19 Andrej Mitrovic wrote:
> On 1/9/13, Walter Bright <newshound2@digitalmars.com> wrote:
> > It is not a bug.
> 
> Something related I want to ask you about:
> 
> struct S
> {
> int _m;
> this(int m)
> {
> this._m = _m; // meant "this._m = m;"
> }
> }
> 
> I'd like to add a warning for identity assignments when it involves a parameter and a field of the aggregate which is the parent of the function. This was filed as http://d.puremagic.com/issues/show_bug.cgi?id=4407.

Why not just prevent identity assignments altogether? What would be the purpose of even allowing a variable to be assigned itself? I see no reason to single out parameters or member variables for that.

- Jonathan M Davis