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