www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Missing compiler warning?

reply "Chris" <wendlec tcd.ie> writes:
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?
Oct 18 2013
next sibling parent "Chris" <wendlec tcd.ie> writes:
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;
	}
}
Oct 18 2013
prev sibling parent reply "bearophile" <bearophileHUGS lycos.com> writes:
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
Oct 18 2013
parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
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
Oct 18 2013
next sibling parent reply "bearophile" <bearophileHUGS lycos.com> writes:
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
Oct 18 2013
parent Timon Gehr <timon.gehr gmx.ch> writes:
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.
Oct 18 2013
prev sibling next sibling parent =?UTF-8?B?QWxpIMOHZWhyZWxp?= <acehreli yahoo.com> writes:
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
Oct 18 2013
prev sibling parent reply "Chris" <wendlec tcd.ie> writes:
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.
Oct 19 2013
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
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, =C3=A0 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 resp= onsible=20 programmer, you fix all warnings, so they're ultimately not really any = different=20 from an error except that you can leave them alone temporarily while=20= developing. It's far better to make something an error not have the com= piler=20 complain about it at all. lint-like tools can do additional analysis ba= sed on=20 additional stuff that a programmer decides they want to look for in the= ir code,=20 but the compiler shouldn't be pointing out stuff that "might" be wrong,= because=20 you'll have to "fix" it whether it's wrong or not just to get the compi= ler to=20 shut up about it. And Walter agrees with me on this. Pretty much the only reason that war= nings=20 even exist in dmd is because he got pestered into adding them, and even= then,=20 we don't have very many. Most of them end up being warnings about stuff= that=20 will become errors later (rather than just making them an error immedia= tely=20 and break code). - Jonathan M Davis
Oct 19 2013
parent reply "Chris" <wendlec tcd.ie> writes:
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?
Oct 21 2013
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Monday, October 21, 2013 10:37:57 Chris wrote:
 Usually same-name
 variables are only used in the constructor. Using them in other
 places in the class is not recommendable
Well, that's up to the programmer, and plenty of folks use them in places like setters as well.
 but is it a good solution to categorically rule out warnings?
Honestly? Yes, I think that it is. I am of the opinion that the _only_ warnings that the compiler should have are things that will become errors later but aren't immediately in order to give programmers the chance to change their code before it breaks. For instance, override is now required on virtual functions which override a base class function, whereas before it wasn't - we used a warning to ease the transition, and that makes sense. But ultimately, it became an error. Because it is not good practice to ever leave warnings in your code, I am firmly of the belief that something should be an error or completely legal and nothing in between. Otherwise, you're needlessly forced to change your code just because it _might_ have a problem. Additional lint-like tools can be written and used to check for anything which "might" be wrong, and the compiler can be left to report only stuff that is definitively wrong. And once we have a D lexer and parser in Phobos (and we're getting close to having a lexer), writing such tools will become much easier, and then such tools can warn programmers about what _they_ want to be warned about but which isn't necessarily wrong. - Jonathan M Davisbut is
Oct 21 2013
parent reply "Chris" <wendlec tcd.ie> writes:
On Monday, 21 October 2013 at 09:05:58 UTC, Jonathan M Davis 
wrote:
 On Monday, October 21, 2013 10:37:57 Chris wrote:
 Usually same-name
 variables are only used in the constructor. Using them in other
 places in the class is not recommendable
Well, that's up to the programmer, and plenty of folks use them in places like setters as well.
 but is it a good solution to categorically rule out warnings?
Honestly? Yes, I think that it is. I am of the opinion that the _only_ warnings that the compiler should have are things that will become errors later but aren't immediately in order to give programmers the chance to change their code before it breaks. For instance, override is now required on virtual functions which override a base class function, whereas before it wasn't - we used a warning to ease the transition, and that makes sense. But ultimately, it became an error. Because it is not good practice to ever leave warnings in your code, I am firmly of the belief that something should be an error or completely legal and nothing in between. Otherwise, you're needlessly forced to change your code just because it _might_ have a problem. Additional lint-like tools can be written and used to check for anything which "might" be wrong, and the compiler can be left to report only stuff that is definitively wrong. And once we have a D lexer and parser in Phobos (and we're getting close to having a lexer), writing such tools will become much easier,
 and then such tools can warn programmers about what _they_ want 
 to be warned
 about but which isn't necessarily wrong.

 - Jonathan M Davisbut is
That would be a good solution, so ideally programmers could have a list of "bad practice" and check against that too. I only wonder which other use cases there are for same name variables other than constructors and setters. void setValue(string input) { this.input = input; } But there you have this. But a function (in the same class) like void processInput() { auto input = // ... // 20 lines later input = std.string.format("Hello %s!", someString); } Why would one want to write code like this? Why should a short-lived local variable assume the name of a class variable? This is a source of confusion and I don't consider this good practice. Maybe it's just personal taste.
Oct 21 2013
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Monday, October 21, 2013 11:24:06 Chris wrote:
 But there you have this. But a function (in the same class) like
 
 void processInput() {
    auto input = // ...
    // 20 lines later
    input = std.string.format("Hello %s!", someString);
 }
 
 Why would one want to write code like this? Why should a
 short-lived local variable assume the name of a class variable?
 This is a source of confusion and I don't consider this good
 practice. Maybe it's just personal taste.
That may very well be bad practice (I certainly wouldn't advise writing code like that), but it's also not definitively wrong. Unless it's definitely a bug, I think that the compiler should keep quiet about it, because anything that it yells about has to be treated as a bug, and we shouldn't be forcing people to change their code just because there _might_ be a bug in it. - Jonathan M Davis
Oct 21 2013
parent reply "Chris" <wendlec tcd.ie> writes:
On Monday, 21 October 2013 at 09:36:48 UTC, Jonathan M Davis 
wrote:
 On Monday, October 21, 2013 11:24:06 Chris wrote:
 But there you have this. But a function (in the same class) 
 like
 
 void processInput() {
    auto input = // ...
    // 20 lines later
    input = std.string.format("Hello %s!", someString);
 }
 
 Why would one want to write code like this? Why should a
 short-lived local variable assume the name of a class variable?
 This is a source of confusion and I don't consider this good
 practice. Maybe it's just personal taste.
That may very well be bad practice (I certainly wouldn't advise writing code like that), but it's also not definitively wrong. Unless it's definitely a bug, I think that the compiler should keep quiet about it, because anything that it yells about has to be treated as a bug, and we shouldn't be forcing people to change their code just because there _might_ be a bug in it. - Jonathan M Davis
So the solution would be an additional layer that can be turned on / off. Warnings like "unused variable" or "shadowing class declaration" are quite useful. I don't see any other cases where a warning would be nice (at the moment).
Oct 21 2013
next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Monday, October 21, 2013 12:50:44 Chris wrote:
 So the solution would be an additional layer that can be turned
 on / off. Warnings like "unused variable" or "shadowing class
 declaration" are quite useful. I don't see any other cases where
 a warning would be nice (at the moment).
Perhaps, but Walter is against adding much in the way of switches to dmd, and he has pretty much the same position on warnings that I do. Also, with a 3rd party tool, it'll be a lot easier to get warnings on exactly what you want, whereas with the compiler, you have to convince Walter that it should be a warning. Having a configurable tool for additional checks is just more flexible and avoids all of the arguments over what should and should be a warning. Also, because of -w (which makes a warning an error), it would actually be _very_ bad for something like "unused variable" to be a warning, because then it would be an error, which would result in a _lot_ of traits failing to compile, because it's _very_ common for those to have unused variables. It also would cause problems with RAII. Ideally, -w wouldn't even exist, since it essentially changes what is and isn't legal in the language, but the fact that it does exist makes it that much more precarious to add warnings to the compiler. - Jonathan M Davis
Oct 21 2013
prev sibling parent "Dicebot" <public dicebot.lv> writes:
On Monday, 21 October 2013 at 10:50:45 UTC, Chris wrote:
 So the solution would be an additional layer that can be turned 
 on / off. Warnings like "unused variable" or "shadowing class 
 declaration" are quite useful. I don't see any other cases 
 where a warning would be nice (at the moment).
And this additional layer is called "static analysis tool", should be configurable and does not belong to compiler. Warnings must die.
Oct 21 2013