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