www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Fun with range deprecations

reply "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
So, a recent Phobos deprecation introduced a fun regression:

	https://issues.dlang.org/show_bug.cgi?id=13257

While the bug was filed specifically for the use case
range.splitter.map, the problem is actually much more general, and far
from obvious how to address.

First, let's consider the following setup.  Start with this example
range:

	struct MyRange(T) {
		// input range
		 property bool empty() { ... }
		 property T front() { ... }
		void popFront() { ... }

		// forward range
		 property MyRange save() { ... }

		// bidirectional range
		 property T back() { ... }
		void popBack() { ... }
	}

Suppose we write an algorithm that operates on ranges in general,
including MyRange. The current convention is to forward as many range
methods as possible, so that if you give it a bidi range, it will, where
possible, give you a wrapped bidirectional range. So we do this:

	auto myAlgo(R)(R range)
		if (isInputRange!R)
	{
		struct Result {
			// input range methods
			 property bool empty() { ... }
			 property U front() { ... }
			void popFront() { ... }

			// N. B. if we got a forward range, and we can
			// implement .save, then do so.
			static if (isForwardRange!R &&
				canDoForwardRange)
			{
				 property Result save() { ... }
			}

			// N. B. if we got a bidirectional range, and we
			// can implement .back and .popBack, then do so.
			static if (isBidirectionalRange!R &&
				canDoBidirectionalRange)
			{
				 property U back() { ... }
				void popBack() { ... }
			}
		}
		return Result(...);
	}

Now suppose, for whatever reason, we decide that implement MyRange as a
bidirectional range was a bad idea, and we decide to deprecate it:

	struct MyRange(T) {
		... // input & forward range API here

		deprecated("Use of MyRange as bidirectional range is now deprecated")
		{
			 property T back() { ... }
			void popBack() { ... }
		}
	}

Now the fun begins. Currently, isBidirectionalRange uses
is(typeof(...)) to check if .back and .popBack exist in target type.
So when myAlgo() checks MyRange, this check passes, because even though
.back and .popBack have been deprecated, they obviously still exist, so
they do have a valid type. So that check passes. Therefore, myAlgo()
will try to export a bidirectional interface in its result.

However, inside Result.back and Result.popBack, when it actually tries
to use MyRange.back and MyRange.popBack, the compiler throws up its
hands and say, Wait!! .back and .popBack are deprecated!!

Note that this happens *regardless* of whether .back and .popBack are
actually used by the user code that calls myAlgo(). This means that if
user code only ever uses forward range capabilities of myAlgo(), users
will *still* get irrelevant deprecation messages -- for bidirectional
range features that they never use! Furthermore, the only reason
myAlgo() added .back and .popBack which trigger the deprecation
messages, is because the is(typeof(...)) check tests positive for
bidirectional range support.

This problem, obviously, is not specific to bidirectional ranges, or,
for that matter, *any* range based code. It's a general problem with
wrapped types, of the form:

	struct Wrapped {
		deprecated void method();
	}

	struct Wrapper(T) {
		T t;
		static if (is(typeof(t.method())))
		{
			void func() {
				t.method();
			}
		}
	}

The problem here is that t.method() is generic, and it has specifically
tested for the usability of t.method(), yet when it tries to actually
use t.method(), it hits a deprecation message. How is it supposed to
know if t.method() has been deprecated? Currently we have no
__traits(deprecated) test. And even if we did, this may not be the best
solution (are we going to now insert __traits(deprecated) all over
generic code, on the off-chance that some random user type somewhere
will get deprecated in the unforeseeable future?).

So the question now is: how do we deal with this issue? Currently, this
problem already exists in Phobos 2.067a, and random user code that calls
splitter(...).map(...) will hit this problem.

So far the following have been suggested, none of which seem
particularly satisfactory:

1) Make Wrapper.func() a template function, so that the deprecation
message is not triggered unless the user actually calls it (in which
case the deprecation message *should* be triggered). The problem is that
when the deprecation message *is* triggered, it comes from deep inside
Phobos, and users may complain, why did you export a bidirectional range
API if that support is already deprecated?

2) Add a __traits(deprecated) or is(T == deprecated) test, and update
all affected sig constraints in Phobos to check for this. Doesn't sound
like an appealing solution.

3) Have std.algorithm.map specifically check for a specific overload of
std.algorithm.splitter, and omit .back and .popBack in that one case.
Very hackish, solves the immediate problem, but leaves the general
problem unsolved. User code that wraps around splitter in a similar way
to map will *still* be broken (even if they never actually use
bidirectional features).

4) Shorten the deprecation cycle of splitter. Doesn't even solve the
immediate problem, just shortens it, and still leaves the general
problem unsolved. User code that wraps around splitter is still broken
(even if they never actually use bidirectional features).

Since no obvious acceptable solution seems forthcoming, I thought we
should bring this to the forum for discussion to see if anyone has a
better idea.


T

-- 
If you're not part of the solution, you're part of the precipitate.
Aug 11 2014
next sibling parent reply "Fra" <a b.it> writes:
On Monday, 11 August 2014 at 22:59:34 UTC, H. S. Teoh via 
Digitalmars-d wrote:

 So, a recent Phobos deprecation introduced a fun regression:

 	https://issues.dlang.org/show_bug.cgi?id=13257

 While the bug was filed specifically for the use case
 range.splitter.map, the problem is actually much more general, 
 and far
 from obvious how to address.

 First, let's consider the following setup.  Start with this 
 example
 range:

 	struct MyRange(T) {
 		// input range
 		 property bool empty() { ... }
 		 property T front() { ... }
 		void popFront() { ... }

 		// forward range
 		 property MyRange save() { ... }

 		// bidirectional range
 		 property T back() { ... }
 		void popBack() { ... }
 	}

 Suppose we write an algorithm that operates on ranges in 
 general,
 including MyRange. The current convention is to forward as many 
 range
 methods as possible, so that if you give it a bidi range, it 
 will, where
 possible, give you a wrapped bidirectional range. So we do this:

 	auto myAlgo(R)(R range)
 		if (isInputRange!R)
 	{
 		struct Result {
 			// input range methods
 			 property bool empty() { ... }
 			 property U front() { ... }
 			void popFront() { ... }

 			// N. B. if we got a forward range, and we can
 			// implement .save, then do so.
 			static if (isForwardRange!R &&
 				canDoForwardRange)
 			{
 				 property Result save() { ... }
 			}

 			// N. B. if we got a bidirectional range, and we
 			// can implement .back and .popBack, then do so.
 			static if (isBidirectionalRange!R &&
 				canDoBidirectionalRange)
 			{
 				 property U back() { ... }
 				void popBack() { ... }
 			}
 		}
 		return Result(...);
 	}

 Now suppose, for whatever reason, we decide that implement 
 MyRange as a
 bidirectional range was a bad idea, and we decide to deprecate 
 it:

 	struct MyRange(T) {
 		... // input & forward range API here

 		deprecated("Use of MyRange as bidirectional range is now 
 deprecated")
 		{
 			 property T back() { ... }
 			void popBack() { ... }
 		}
 	}

 Now the fun begins. Currently, isBidirectionalRange uses
 is(typeof(...)) to check if .back and .popBack exist in target 
 type.
 So when myAlgo() checks MyRange, this check passes, because 
 even though
 .back and .popBack have been deprecated, they obviously still 
 exist, so
 they do have a valid type. So that check passes. Therefore, 
 myAlgo()
 will try to export a bidirectional interface in its result.

 However, inside Result.back and Result.popBack, when it 
 actually tries
 to use MyRange.back and MyRange.popBack, the compiler throws up 
 its
 hands and say, Wait!! .back and .popBack are deprecated!!

 Note that this happens *regardless* of whether .back and 
 .popBack are
 actually used by the user code that calls myAlgo(). This means 
 that if
 user code only ever uses forward range capabilities of 
 myAlgo(), users
 will *still* get irrelevant deprecation messages -- for 
 bidirectional
 range features that they never use! Furthermore, the only reason
 myAlgo() added .back and .popBack which trigger the deprecation
 messages, is because the is(typeof(...)) check tests positive 
 for
 bidirectional range support.

 This problem, obviously, is not specific to bidirectional 
 ranges, or,
 for that matter, *any* range based code. It's a general problem 
 with
 wrapped types, of the form:

 	struct Wrapped {
 		deprecated void method();
 	}

 	struct Wrapper(T) {
 		T t;
 		static if (is(typeof(t.method())))
 		{
 			void func() {
 				t.method();
 			}
 		}
 	}

 The problem here is that t.method() is generic, and it has 
 specifically
 tested for the usability of t.method(), yet when it tries to 
 actually
 use t.method(), it hits a deprecation message. How is it 
 supposed to
 know if t.method() has been deprecated? Currently we have no
 __traits(deprecated) test. And even if we did, this may not be 
 the best
 solution (are we going to now insert __traits(deprecated) all 
 over
 generic code, on the off-chance that some random user type 
 somewhere
 will get deprecated in the unforeseeable future?).

 So the question now is: how do we deal with this issue? 
 Currently, this
 problem already exists in Phobos 2.067a, and random user code 
 that calls
 splitter(...).map(...) will hit this problem.

 So far the following have been suggested, none of which seem
 particularly satisfactory:

 1) Make Wrapper.func() a template function, so that the 
 deprecation
 message is not triggered unless the user actually calls it (in 
 which
 case the deprecation message *should* be triggered). The 
 problem is that
 when the deprecation message *is* triggered, it comes from deep 
 inside
 Phobos, and users may complain, why did you export a 
 bidirectional range
 API if that support is already deprecated?

 2) Add a __traits(deprecated) or is(T == deprecated) test, and 
 update
 all affected sig constraints in Phobos to check for this. 
 Doesn't sound
 like an appealing solution.

 3) Have std.algorithm.map specifically check for a specific 
 overload of
 std.algorithm.splitter, and omit .back and .popBack in that one 
 case.
 Very hackish, solves the immediate problem, but leaves the 
 general
 problem unsolved. User code that wraps around splitter in a 
 similar way
 to map will *still* be broken (even if they never actually use
 bidirectional features).

 4) Shorten the deprecation cycle of splitter. Doesn't even 
 solve the
 immediate problem, just shortens it, and still leaves the 
 general
 problem unsolved. User code that wraps around splitter is still 
 broken
 (even if they never actually use bidirectional features).

 Since no obvious acceptable solution seems forthcoming, I 
 thought we
 should bring this to the forum for discussion to see if anyone 
 has a
 better idea.


 T


Noob question... shouldn't __traits(compile) enable us to handle 
the situation correctly?
Aug 12 2014
parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Tue, Aug 12, 2014 at 08:20:09AM +0000, Fra via Digitalmars-d wrote:
[..]

 Noob question... shouldn't __traits(compile) enable us to handle the
 situation correctly?


It does, in a sense. A deprecated feature still compiles, so
__traits(compiles) is returning the correct value (true). Except that it
doesn't tell you if a deprecation warning will happen if you actually
use the tested feature.

A scarier thought is what happens what __traits(compiles) returns /
ought to return when compiling with -de (abort compilation on using
deprecated features). Code that use __traits(compiles) would change
behaviour depending on compiler flags, which is something Walter frowns
on.


T

-- 
One reason that few people are aware there are programs running the
internet is that they never crash in any significant way: the free
software underlying the internet is reliable to the point of
invisibility. -- Glyn Moody, from the article "Giving it all away"
Aug 12 2014
prev sibling parent reply "Dicebot" <public dicebot.lv> writes:
On Monday, 11 August 2014 at 22:59:34 UTC, H. S. Teoh via 
Digitalmars-d wrote:

 1) Make Wrapper.func() a template function, so that the 
 deprecation
 message is not triggered unless the user actually calls it (in 
 which
 case the deprecation message *should* be triggered). The 
 problem is that
 when the deprecation message *is* triggered, it comes from deep 
 inside
 Phobos, and users may complain, why did you export a 
 bidirectional range
 API if that support is already deprecated?


After some thinking about it I am leaning towards this option 
again. If there is some user code that relies on bidirectional 
API exposed then it probably should get deprecation warning after 
all.

I may change my mind again though :)
Aug 12 2014
parent reply "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Tue, Aug 12, 2014 at 06:42:56PM +0000, Dicebot via Digitalmars-d wrote:

 On Monday, 11 August 2014 at 22:59:34 UTC, H. S. Teoh via Digitalmars-d
 wrote:

1) Make Wrapper.func() a template function, so that the deprecation
message is not triggered unless the user actually calls it (in which
case the deprecation message *should* be triggered). The problem is
that when the deprecation message *is* triggered, it comes from deep
inside Phobos, and users may complain, why did you export a
bidirectional range API if that support is already deprecated?


 
 After some thinking about it I am leaning towards this option again.
 If there is some user code that relies on bidirectional API exposed
 then it probably should get deprecation warning after all.
 
 I may change my mind again though :)


Well, it does seem to be the least evil among the other option, even if
it's not ideal!

It does raise the question, though: should *all* range algos that return
range wrappers that optionally forward functionality like forward range
or bidirectional range functionality, be templatized? Obviously, we must
always expose an input range API since that's the minimum functionality;
but should all forward / bidirectional / random access / length methods
be templatized? Otherwise this story will just repeat itself in the
future, when something else needs to have, say, .length deprecated, or
.opIndex, etc..

On the positive side, if these "optional" range methods are templatized,
they will reduce template bloat in code that doesn't need to use them.
AFAIK, if you have a template struct with 10 methods, all 10 will get
instantiated when you instantiate the struct, even if you only ever use
3 of them in your code. So it seems to be a good thing to templatize the
"additional" methods like .save, .back, .popBack, .opIndex, which may
never get used if the user only ever needs the input range methods.

Note that this still doesn't fully solve the problem, because if you use
something like std.range.InputRangeObject on the result of splitter(),
then it's going to spew deprecation messages no matter what, because
InputRangeObject uses reflection to determine whether to export .back
and .popBack.

So basically, deprecation and generic code don't seem to get along very
well. :-(


T

-- 
What do you get if you drop a piano down a mineshaft? A flat minor.
Aug 12 2014
next sibling parent reply "Dicebot" <public dicebot.lv> writes:
On Tuesday, 12 August 2014 at 19:15:48 UTC, H. S. Teoh via 
Digitalmars-d wrote:

 It does raise the question, though: should *all* range algos 
 that return
 range wrappers that optionally forward functionality like 
 forward range
 or bidirectional range functionality, be templatized? 
 Obviously, we must
 always expose an input range API since that's the minimum 
 functionality;
 but should all forward / bidirectional / random access / length 
 methods
 be templatized? Otherwise this story will just repeat itself in 
 the
 future, when something else needs to have, say, .length 
 deprecated, or
 .opIndex, etc..


I think it is ok to go with templatization as default deprecation 
handling approach but just changing all signatures for the sake 
of consistency is not worth it - not until you actually need to 
deprecate stuff.

Lets wait a few days if any new ideas appear on this topic and 
proceed with this solution.
Aug 12 2014
next sibling parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Wed, Aug 13, 2014 at 01:10:36AM +0000, Dicebot via Digitalmars-d wrote:

 On Tuesday, 12 August 2014 at 19:15:48 UTC, H. S. Teoh via Digitalmars-d
 wrote:

It does raise the question, though: should *all* range algos that
return range wrappers that optionally forward functionality like
forward range or bidirectional range functionality, be templatized?
Obviously, we must always expose an input range API since that's the
minimum functionality; but should all forward / bidirectional /
random access / length methods be templatized? Otherwise this story
will just repeat itself in the future, when something else needs to
have, say, .length deprecated, or .opIndex, etc..


 
 I think it is ok to go with templatization as default deprecation
 handling approach but just changing all signatures for the sake of
 consistency is not worth it - not until you actually need to deprecate
 stuff.


Yeah, I wouldn't do it for existing code just for the sake of doing it.
But I'd seriously consider writing future code in this way, though.



 Lets wait a few days if any new ideas appear on this topic and proceed
 with this solution.


Sounds good.


T

-- 
Programming is not just an act of telling a computer what to do: it is
also an act of telling other programmers what you wished the computer to
do. Both are important, and the latter deserves care. -- Andrew Morton
Aug 13 2014
prev sibling parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Wed, Aug 13, 2014 at 01:10:36AM +0000, Dicebot via Digitalmars-d wrote:

 On Tuesday, 12 August 2014 at 19:15:48 UTC, H. S. Teoh via Digitalmars-d
 wrote:

It does raise the question, though: should *all* range algos that
return range wrappers that optionally forward functionality like
forward range or bidirectional range functionality, be templatized?
Obviously, we must always expose an input range API since that's the
minimum functionality; but should all forward / bidirectional /
random access / length methods be templatized? Otherwise this story
will just repeat itself in the future, when something else needs to
have, say, .length deprecated, or .opIndex, etc..


 
 I think it is ok to go with templatization as default deprecation
 handling approach but just changing all signatures for the sake of
 consistency is not worth it - not until you actually need to deprecate
 stuff.
 
 Lets wait a few days if any new ideas appear on this topic and proceed
 with this solution.


Speaking of which, should this only be done for std.algorithm.map, or
should we also templatize the other algos that export a bidirectional
interface, on the off chance that somebody will try to use splitter with
them?


T

-- 
Маленькие детки - маленькие бедки.
Aug 13 2014
prev sibling parent reply "John Colvin" <john.loughran.colvin gmail.com> writes:
On Tuesday, 12 August 2014 at 19:15:48 UTC, H. S. Teoh via 
Digitalmars-d wrote:

 On the positive side, if these "optional" range methods are 
 templatized,
 they will reduce template bloat in code that doesn't need to 
 use them.
 AFAIK, if you have a template struct with 10 methods, all 10 
 will get
 instantiated when you instantiate the struct, even if you only 
 ever use
 3 of them in your code. So it seems to be a good thing to 
 templatize the
 "additional" methods like .save, .back, .popBack, .opIndex, 
 which may
 never get used if the user only ever needs the input range 
 methods.



templates to _reduce_ code size, I'd never thought of that before 
(although really this is work that a linker can do).
Aug 13 2014
parent reply "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Wed, Aug 13, 2014 at 08:54:09AM +0000, John Colvin via Digitalmars-d wrote:

 On Tuesday, 12 August 2014 at 19:15:48 UTC, H. S. Teoh via Digitalmars-d
 wrote:

On the positive side, if these "optional" range methods are
templatized, they will reduce template bloat in code that doesn't
need to use them.  AFAIK, if you have a template struct with 10
methods, all 10 will get instantiated when you instantiate the
struct, even if you only ever use 3 of them in your code. So it seems
to be a good thing to templatize the "additional" methods like .save,
.back, .popBack, .opIndex, which may never get used if the user only
ever needs the input range methods.


 
 
 templates to _reduce_ code size, I'd never thought of that before
 (although really this is work that a linker can do).


Having the linker do this is kinda patching things over after the
problem has already occurred IMO. Not emitting the code in the first
place beats emitting it only to discard it later.


T

-- 
Ruby is essentially Perl minus Wall.
Aug 13 2014
parent reply "Dicebot" <public dicebot.lv> writes:
On Wednesday, 13 August 2014 at 16:38:10 UTC, H. S. Teoh via 
Digitalmars-d wrote:

 templates to _reduce_ code size, I'd never thought of that 
 before
 (although really this is work that a linker can do).


 Having the linker do this is kinda patching things over after 
 the
 problem has already occurred IMO. Not emitting the code in the 
 first
 place beats emitting it only to discard it later.


It is easier that way though and is already implemented (all 
praise ldc!)
Aug 13 2014
parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Wed, Aug 13, 2014 at 04:39:58PM +0000, Dicebot via Digitalmars-d wrote:

 On Wednesday, 13 August 2014 at 16:38:10 UTC, H. S. Teoh via Digitalmars-d
 wrote:

templates to _reduce_ code size, I'd never thought of that before
(although really this is work that a linker can do).


Having the linker do this is kinda patching things over after the
problem has already occurred IMO. Not emitting the code in the first
place beats emitting it only to discard it later.


 
 It is easier that way though and is already implemented (all praise
 ldc!)


I can't wait for the day when the frontend becomes fully independent,
and dmd, gdc, ldc, can all be up-to-date with the latest git HEAD by
just recompiling. :)


T

-- 
To err is human; to forgive is not our policy. -- Samuel Adler
Aug 13 2014