www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Warning on template parameter identifier shadowing?

reply Johan <j j.nl> writes:
Hi all,

I just discovered a silly programming error on my side. Stripped 
down, it looks like this:
```
template foo(T) {
     pragma(msg, __LINE__, ": ", T);

     // This should not have the "(T)"
     void foo(T)(T i) {
         pragma(msg, __LINE__, ": ", T);
     }
}

void main()
{
     foo!long(1);
}
```
which outputs
2: long
5: int
(https://d.godbolt.org/z/8K6sjc)

I made a template inside a template, where I intended to make 
just one eponymous template.

What do you think about adding a warning about shadowing of an 
outer template parameter by an inner template parameter name?

cheers,
   Johan
Aug 05 2020
next sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Wed, Aug 05, 2020 at 11:12:53AM +0000, Johan via Digitalmars-d wrote:
[...]

 What do you think about adding a warning about shadowing of an outer
 template parameter by an inner template parameter name?


[...]

Probably a good idea!


T

-- 
The only difference between male factor and malefactor is just a little
emptiness inside.
Aug 05 2020
prev sibling parent reply Paul Backus <snarwin gmail.com> writes:
On Wednesday, 5 August 2020 at 11:12:53 UTC, Johan wrote:

 What do you think about adding a warning about shadowing of an 
 outer template parameter by an inner template parameter name?

 cheers,
   Johan


Not a fan. There are cases where I've done this 
intentionally--not with eponymous template members, but with 
private helper templates:

template foo(T) {
     enum helper(T) = ...; // error?
     void foo(T i) {
         static if (helper!T) ...;
         else ...;
     }
}
Aug 05 2020
parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 8/5/20 12:10 PM, Paul Backus wrote:

 On Wednesday, 5 August 2020 at 11:12:53 UTC, Johan wrote:

 What do you think about adding a warning about shadowing of an outer 
 template parameter by an inner template parameter name?


 
 Not a fan. There are cases where I've done this intentionally--not with 
 eponymous template members, but with private helper templates:
 
 template foo(T) {
      enum helper(T) = ...; // error?
      void foo(T i) {
          static if (helper!T) ...;
          else ...;
      }
 }


I've intentionally done that, and even though it worked, rewrote the 
template parameter names because it's confusing to read.

It would have to be a long deprecation cycle.

I would be in favor of the change.

-Steve
Aug 05 2020
parent reply Paul Backus <snarwin gmail.com> writes:
On Wednesday, 5 August 2020 at 16:22:48 UTC, Steven Schveighoffer 
wrote:

 On 8/5/20 12:10 PM, Paul Backus wrote:

 
 Not a fan. There are cases where I've done this 
 intentionally--not with eponymous template members, but with 
 private helper templates:
 
 template foo(T) {
      enum helper(T) = ...; // error?
      void foo(T i) {
          static if (helper!T) ...;
          else ...;
      }
 }


 I've intentionally done that, and even though it worked, 
 rewrote the template parameter names because it's confusing to 
 read.

 It would have to be a long deprecation cycle.

 I would be in favor of the change.

 -Steve


"Confusing to read" does not justify a compiler warning. The 
compiler accepts lots of code that's confusing to read.

The appropriate place for a rule like this is in a style guide or 
a configurable linter.

(This would also interact very badly with string-based code 
generation, since there's no reliable way to generate unique 
identifiers at compile time.)
Aug 05 2020
next sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Wed, Aug 05, 2020 at 04:35:12PM +0000, Paul Backus via Digitalmars-d wrote:

 [...] there's no reliable way to generate unique identifiers at
 compile time.)


Sure there is. Well, depending on what you need said identifiers for,
that is.  Given a unique tuple, a template instantiated with that tuple
is a unique identifier for that tuple.

I've used this fact to generate unique identifiers for temporaries in
static foreach loops, since static foreach does not introduce a new
scope, so all iterations share the same namespace, and any temporaries
you might declare will collide with other iterations and fail with a
compile error. To work around this, I wrap the desired declaration(s) in
an auxiliary template that gets instantiated with the index variable of
the static foreach, thus guaranteeing uniqueness:

	struct TmpStruct(int iter) {
		...
	}

	int someAcc;
	switch (someVar) {
		static foreach (i; 0 .. 99) {
			case i:
				auto tmp = TmpStruct!i(...);
				doSomething(tmp);
				break;
		}
	}


T

-- 
Long, long ago, the ancient Chinese invented a device that lets them see
through walls. It was called the "window".
Aug 05 2020
prev sibling next sibling parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 8/5/20 12:35 PM, Paul Backus wrote:

 On Wednesday, 5 August 2020 at 16:22:48 UTC, Steven Schveighoffer wrote:

 On 8/5/20 12:10 PM, Paul Backus wrote:

 Not a fan. There are cases where I've done this intentionally--not 
 with eponymous template members, but with private helper templates:

 template foo(T) {
      enum helper(T) = ...; // error?
      void foo(T i) {
          static if (helper!T) ...;
          else ...;
      }
 }


 I've intentionally done that, and even though it worked, rewrote the 
 template parameter names because it's confusing to read.

 It would have to be a long deprecation cycle.

 I would be in favor of the change.


 
 "Confusing to read" does not justify a compiler warning. The compiler 
 accepts lots of code that's confusing to read.


That's not a justification, just my personal experience. I read my code 
again and think, "wait, what T am I talking about? I'll just change it 
to U."

For the same reason, I don't like to name inner struct members the same 
as any function variables, even though that's allowed.

In essence, this is no different than erroring on shadowing of function 
variables, which could also be allowed, but saves a lot of trouble. You 
can look at it more as a "hey D, you have this position on variable 
names, but not on template parameter names, why the inconsistency?"

Note that I think it purely should be for nested template *parameters*, 
not for other names.


 (This would also interact very badly with string-based code generation, 
 since there's no reliable way to generate unique identifiers at compile 
 time.)


Somehow people cope with this for shadowed variable names just fine.

-Steve
Aug 05 2020
parent reply Paul Backus <snarwin gmail.com> writes:
On Wednesday, 5 August 2020 at 16:57:49 UTC, Steven Schveighoffer 
wrote:

 In essence, this is no different than erroring on shadowing of 
 function variables, which could also be allowed, but saves a 
 lot of trouble. You can look at it more as a "hey D, you have 
 this position on variable names, but not on template parameter 
 names, why the inconsistency?"


The case you point out here is the exception, not the rule. D 
allows shadowing for everything *except* local variables in 
functions. The most consistent thing to do would be to allow it 
everywhere, period; presumably an exception was made because that 
specific type of shadowing was thought to be particularly 
error-prone.

Note that the exception does not apply to *parameters* of nested 
functions, which is the analogous case here:

void foo(int x) {
     void bar(int x) {} // ok
}
Aug 05 2020
parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 8/5/20 1:27 PM, Paul Backus wrote:

 On Wednesday, 5 August 2020 at 16:57:49 UTC, Steven Schveighoffer wrote:

 In essence, this is no different than erroring on shadowing of 
 function variables, which could also be allowed, but saves a lot of 
 trouble. You can look at it more as a "hey D, you have this position 
 on variable names, but not on template parameter names, why the 
 inconsistency?"


 
 The case you point out here is the exception, not the rule. D allows 
 shadowing for everything *except* local variables in functions. The most 
 consistent thing to do would be to allow it everywhere, period; 
 presumably an exception was made because that specific type of shadowing 
 was thought to be particularly error-prone.


One reason shadowing of variables is not allowed, but shadowing of 
module-level symbols is fine is because you can explicitly refer to the 
module-level symbols by prepending a dot.

I haven't thought completely through it, but if I could, I'd ban all 
shadowing of anything that could not be accessed via some explicit scope 
expression.


 
 Note that the exception does not apply to *parameters* of nested 
 functions, which is the analogous case here:
 
 void foo(int x) {
      void bar(int x) {} // ok
 }


Yes you are right of course. I happen to believe it should require a 
difference here as well. Again, in my own code, I avoid this case.

I still believe that a) it's very error prone to define nested templates 
with the same parameter names as the parameters of the outer template, 
and b) the burden of requiring everything to have a unique name would be 
minimal.

I happen to do this anyway when I remember. I would appreciate the 
compiler helping me out when I don't. Hence, I'm in favor of such a 
proposal.

-Steve
Aug 05 2020
parent Paul Backus <snarwin gmail.com> writes:
On Wednesday, 5 August 2020 at 17:54:47 UTC, Steven Schveighoffer 
wrote:

 I happen to do this anyway when I remember. I would appreciate 
 the compiler helping me out when I don't. Hence, I'm in favor 
 of such a proposal.

 -Steve


I also have coding style preferences that the compiler doesn't 
help me with. It doesn't follow that I'm in favor of making them 
into compiler warnings. :)
Aug 05 2020
prev sibling parent Johan <j j.nl> writes:
On Wednesday, 5 August 2020 at 16:35:12 UTC, Paul Backus wrote:

 On Wednesday, 5 August 2020 at 16:22:48 UTC, Steven 
 Schveighoffer wrote:

 On 8/5/20 12:10 PM, Paul Backus wrote:

 
 Not a fan. There are cases where I've done this 
 intentionally--not with eponymous template members, but with 
 private helper templates:
 
 template foo(T) {
      enum helper(T) = ...; // error?
      void foo(T i) {
          static if (helper!T) ...;
          else ...;
      }
 }






Can you explain the "intentionally"? What purpose did it serve to 
name the template parameters the same? (in the code given, if you 
only ever used helper!T, you could have made "helper" a 
non-templated enum)

template foo(T) {
      enum helper = ...uses T...;
      void foo(T i) {
          static if (helper) ...;
          else ...;
      }
}


 I've intentionally done that, and even though it worked, 
 rewrote the template parameter names because it's confusing to 
 read.

 It would have to be a long deprecation cycle.

 I would be in favor of the change.

 -Steve


 "Confusing to read" does not justify a compiler warning.


Actually, I think it does. There are not many other reasons I can 
think of for why we have the existing "warnings".

The bigger problem is that warnings are all or nothing, instead 
of being able to disable/enable warnings to what the user likes 
[*]. So there is no way to gently add warnings and get user 
experience and feedback.

-Johan

[*] https://github.com/dlang/dmd/pull/5592
Aug 05 2020