I had a stupid bug:

class Base {
  SomeStruct someStruct;
  // ...
}

class BaseSub {
  // ...
  override {
    public void doThis() {
      auto someStruct = checkSomething("input"); // Bug is here, of course,
                                                 // a leftover from old code
    }
  }

  private void doSomethingElse() {
    if (someStruct.hasValue) {
       auto val = someStruct.value; // Segmentation fault (core dumped)
       // ...
    }
  }

  private auto someCheck(string input) {
    return SomeStruct(input);
  }
}

dmd 2.63

SomeStruct is declared outside the class.

It compiled and dmd didn't say anything about "local variable declaration someStruct is hiding class variable someStruct."

Is it because a) I use it in "override" and/or b) because I declare it in the base class but only use it in the subclass (for now)? Am I making a mess of scopes and stuff? Or is it a bug?

Ok. Here's a version to work with, if anyone wants to reproduce the behavior.

import std.stdio;

void main() {
	auto sub = new BaseSub();
	sub.doSomething();
}

class Base {
	SomeStruct someStruct;
	this() {
		
	}
	
	public void doSomething() {
		writeln("On strike today");
	}
}

class BaseSub : Base {
	this () {
		super();
	}
	
	override {
		public void doSomething() {
			auto someStruct = checkSomething("input"); // Bug here. Remove auto and "Same here" will be printed.
			doSomethingElse();
		}
	}
	
	private void doSomethingElse() {
		if (someStruct.equal) {
			writeln("Same here!");
		}
	}
	
	private auto checkSomething(string s) {
		return SomeStruct(s);
	}
}

struct SomeStruct {
	private:
	string str;
	bool isEqual;
	this(string s) {
		str = s;
		checkInput();
	}
	
	private void checkInput() {
		if (str == "input") {
			isEqual = true;
		}
	}
	
	@property bool equal() {
		return isEqual;
	}
}

Chris:

> I had a stupid bug:
>
> class Base {
>   SomeStruct someStruct;
>   // ...
> }
>
> class BaseSub {
>   // ...
>   override {
>     public void doThis() {
>       auto someStruct = checkSomething("input"); // Bug is here, of course,
>                                                  // a leftover from old code
>     }
>   }
>
>   private void doSomethingElse() {
>     if (someStruct.hasValue) {
>        auto val = someStruct.value; // Segmentation fault (core dumped)
>        // ...
>     }
>   }
>
>   private auto someCheck(string input) {
>     return SomeStruct(input);
>   }
> }
>
> dmd 2.63
>
> SomeStruct is declared outside the class.
>
> It compiled and dmd didn't say anything about "local variable declaration someStruct is hiding class variable someStruct."


If I understand correctly, you are suggesting to generate a warning here:


class Foo {
    int x;
    void bar() {
        auto x = 1; // ?
    }
    void spam(int x) { // ?
    }
}
void main() {}


Currently D doesn't give an error or warning if in a method you declare a local variable with the same name of a instance member. This is indeed a source of bugs and I have had similar problems. Generally D prefers to minimize the number of warnings, so according to the D style that should become an error.

I presume D is designed this way for compatibility with Java code. But I don't like much this part of the D design. In other cases (with for/foreach loops, the with statement) D has chosen to break the C++ way and introduce a variable shadowing error.

Bye,
bearophile

On Friday, October 18, 2013 18:32:32 bearophile wrote:
> Currently D doesn't give an error or warning if in a method you declare a local variable with the same name of a instance member. This is indeed a source of bugs and I have had similar problems. Generally D prefers to minimize the number of warnings, so according to the D style that should become an error.

There's no way that that's ever going to become an error. It's just too useful. The issue has come up before, and it's not going away. The prime example is constructors.

this(int a, string b)
{
 this.a = a;
 this.b = b;
 ...
}

Too many people would get annoyed if they were required to name their local variables differently - especially in a case like this. If you want to avoid the problem, then just don't name your local variables the same as your member variables - the most obvious solution being to do name your member variables differently - e.g. by prepending them with _.

- Jonathan M Davis

Jonathan M Davis:

> The prime example is constructors.
>
> this(int a, string b)
> {
>  this.a = a;
>  this.b = b;
>  ...
> }

That example of yours shows a possible intermediate rule: when in a method you define a local variable that has the same name as an instance member, then the instance member must be referred with the "this." prefix inside that function. So that code will compile.

Bye,
bearophile

On 10/18/2013 10:42 AM, Jonathan M Davis wrote:

> If you want to avoid the problem, then just don't name your local variables
> the same as your member variables

Like OP, when I was bitten by this before, I had not intended to define a local variable. It is too easy for the fingers to drop an unintentional 'auto' in there, and the meaning becomes defining a local variable.

Ali

On 10/18/2013 08:26 PM, bearophile wrote:
>
> That example of yours shows a possible intermediate rule: when in a
> method you define a local variable that has the same name as an instance
> member, then the instance member must be referred with the "this."
> prefix inside that function. So that code will compile.

We already have this rule.

On Friday, 18 October 2013 at 17:42:22 UTC, Jonathan M Davis wrote:
> On Friday, October 18, 2013 18:32:32 bearophile wrote:
>> Currently D doesn't give an error or warning if in a method you
>> declare a local variable with the same name of a instance member.
>> This is indeed a source of bugs and I have had similar problems.
>> Generally D prefers to minimize the number of warnings, so
>> according to the D style that should become an error.
>
> There's no way that that's ever going to become an error. It's just too
> useful. The issue has come up before, and it's not going away. The prime
> example is constructors.
>
> this(int a, string b)
> {
>  this.a = a;
>  this.b = b;
>  ...
> }
>
> Too many people would get annoyed if they were required to name their local
> variables differently - especially in a case like this. If you want to avoid
> the problem, then just don't name your local variables the same as your member
> variables - the most obvious solution being to do name your member variables
> differently - e.g. by prepending them with _.
>
> - Jonathan M Davis

A warning would be enough. The thing is I didn't want to give it the same name. It was meant to be the class variable but the auto was a leftover from a test. A warning would have been nice, à la "do you really want this?". I would have seen it immediately.

On Saturday, October 19, 2013 18:50:16 Chris wrote:
> A warning would be enough. The thing is I didn't want to give it the same name. It was meant to be the class variable but the auto was a leftover from a test. A warning would have been nice, à la "do you really want this?". I would have seen it immediately.

Warnings are pretty much always a bad idea IMHO. If you're being a responsible programmer, you fix all warnings, so they're ultimately not really any different from an error except that you can leave them alone temporarily while developing. It's far better to make something an error not have the compiler complain about it at all. lint-like tools can do additional analysis based on additional stuff that a programmer decides they want to look for in their code, but the compiler shouldn't be pointing out stuff that "might" be wrong, because you'll have to "fix" it whether it's wrong or not just to get the compiler to shut up about it.

And Walter agrees with me on this. Pretty much the only reason that warnings even exist in dmd is because he got pestered into adding them, and even then, we don't have very many. Most of them end up being warnings about stuff that will become errors later (rather than just making them an error immediately and break code).

- Jonathan M Davis

On Sunday, 20 October 2013 at 01:15:12 UTC, Jonathan M Davis wrote:
> On Saturday, October 19, 2013 18:50:16 Chris wrote:
>> A warning would be enough. The thing is I didn't want to give it
>> the same name. It was meant to be the class variable but the auto
>> was a leftover from a test. A warning would have been nice, à la
>> "do you really want this?". I would have seen it immediately.
>
> Warnings are pretty much always a bad idea IMHO. If you're being a responsible
> programmer, you fix all warnings, so they're ultimately not really any different
> from an error except that you can leave them alone temporarily while
> developing. It's far better to make something an error not have the compiler
> complain about it at all. lint-like tools can do additional analysis based on
> additional stuff that a programmer decides they want to look for in their code,
> but the compiler shouldn't be pointing out stuff that "might" be wrong, because
> you'll have to "fix" it whether it's wrong or not just to get the compiler to
> shut up about it.
>
> And Walter agrees with me on this. Pretty much the only reason that warnings
> even exist in dmd is because he got pestered into adding them, and even then,
> we don't have very many. Most of them end up being warnings about stuff that
> will become errors later (rather than just making them an error immediately
> and break code).
>
> - Jonathan M Davis

I can see your point. However, in this case a warning would really make sense, in my opinion, because it is potentially harmful and very likely not what the programmer intended, except in a case like

this (string a) {
 this.a = a;
}

which could be ignored by the compiler. Usually same-name variables are only used in the constructor. Using them in other places in the class is not recommendable, and I don't mind useful warnings like "Listen, there is a variable shadowing the class variable, do you really really want this?" To spam the command line with compiler warnings is not very helpful, I agree, but is it a good solution to categorically rule out warnings?