Trigger a hard error on deprecation messages #5546

wilzbach · 2017-07-05T16:18:22Z

Yeah but it doesn't make sense otherwise, does it? Seems antithetical to deprecate something for everyone else but not ourselves.
Any reason not to enable -de on the test suite then?

CC @CyberShadow

dlang-bot · 2017-07-05T16:18:23Z

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

CyberShadow · 2017-07-05T16:24:04Z

std/string.d



+/*
 @safe pure @nogc unittest


Wasn't it possible to mark the unittest as deprecated, which allows testing deprecated code? Or was I dreaming?

Nope, that does seem to work!

CyberShadow · 2017-07-05T16:25:06Z

std/string.d



+/*
 @safe pure @nogc unittest


Nope, that does seem to work!

wilzbach · 2017-07-05T16:29:25Z

Wasn't it possible to mark the unittest as deprecated, which allows testing deprecated code? Or was I dreaming?
Nope, that does seem to work!

Wow sweat - it works on my local machine :)

wilzbach

Not sure how to handle the unittests in std.uni which access private symbols. I undocumented them for now to see whether it would pass all stages.

wilzbach · 2017-07-05T17:32:37Z

std/csv.d

 }

-///
 @safe pure unittest


CsvReader is private

wilzbach · 2017-07-05T17:33:46Z

std/uni.d

-    ///
    @safe unittest
    {
        assert(unicode.Cyrillic.intersect('-').byInterval.empty);


generated/linux/release/64/publictests/std_uni.d(132): Deprecation: std.uni.InversionList!(GcPolicy).InversionList.intersect(U)(U rhs) if (isCodepointSet!U) is not visible from module std_uni

wilzbach · 2017-07-05T17:34:15Z

std/uni.d


-///
 @system pure unittest
 {


generated/linux/release/64/publictests/std_uni.d(192): Deprecation: std.uni.BitPacked(T, ulong sz) if (isIntegral!T || is(T : dchar)) is not visible from module std_uni

wilzbach · 2017-07-05T17:34:32Z

std/uni.d

    }

-    ///
    @safe unittest


generated/linux/release/64/publictests/std_uni.d(242): Deprecation: std.uni.Utf8Matcher!().Impl!(1, 2, 3, 4).Impl.subMatcher(SizesToPick...)() is not visible from module std_uni generated/linux/release/64/publictests/std_uni.d(243): Deprecation: std.uni.Utf8Matcher!().Impl!(1, 2, 3, 4).Impl.subMatcher(SizesToPick...)() is not visible from module std_uni generated/linux/release/64/publictests/std_uni.d(244): Deprecation: std.uni.Utf8Matcher!().Impl!(1, 2, 3, 4).Impl.subMatcher(SizesToPick...)() is not visible from module std_uni generated/linux/release/64/publictests/std_uni.d(245): Deprecation: std.uni.Utf8Matcher!().Impl!(1, 2, 3, 4).Impl.subMatcher(SizesToPick...)() is not visible from module std_uni generated/linux/release/64/publictests/std_uni.d(250): Deprecation: std.uni.Utf16Matcher!().Impl!(1, 2).Impl.subMatcher(SizesToPick...)() is not visible from module std_uni

CyberShadow · 2017-07-05T17:44:52Z

I undocumented them for now to see whether it would pass all stages.

Yep, I think undocumenting them is the way to go for now. If the examples are not actually usable by users, then it's misleading to even show them. CI will prevent adding documented unittests that aren't usable outside the module, right?

CyberShadow · 2017-07-05T17:45:58Z

CC @DmitryOlshansky

wilzbach · 2017-07-05T17:50:25Z

If the examples are not actually usable by users, then it's misleading to even show them. CI will prevent adding documented unittests that aren't usable outside the module, right?

Yup. Any idea on how we could tackle the deprecation warnings for Windows?
The problem is that toUTF8/16 got only partially deprecated and these are legit uses:

std\windows\registry.d(48): Deprecation: function std.utf.toUTF8 is deprecated - To be removed November 2017. Please use std.utf.encode instead.
std\windows\registry.d(48): Deprecation: function std.utf.toUTF16 is deprecated - To be removed November 2017. Please use std.utf.encode instead.
std\file.d(2971): Deprecation: function std.utf.toUTF8 is deprecated - To be removed November 2017. Please use std.utf.encode instead.

Maybe by(W)Char.array.assumeUnique?

CyberShadow · 2017-07-05T17:52:28Z

Correct me if I'm wrong, but couldn't those uses simply be replaced with std.conv.to!string / to!wstring?

wilzbach · 2017-07-05T17:57:13Z

Correct me if I'm wrong, but couldn't those uses simply be replaced with std.conv.to!string / to!wstring?

Let's see what the auto-tester says ;-)

jmdavis · 2017-07-05T18:36:41Z

Yep, I think undocumenting them is the way to go for now. If the examples are not actually usable by users, then it's misleading to even show them.

Continuing to show the examples makes it easier for folks to figure out how the functions work so that they can more easily figure out how to fix their code so that it's not using the deprecated functions anymore - especially in cases where we're not providing a simple replacement (as is the case here). So, I think that it's better to keep the examples in the documentation if we can.

CyberShadow · 2017-07-05T18:49:16Z

Continuing to show the examples makes it easier for folks to figure out how the functions work so that they can more easily figure out how to fix their code so that it's not using the deprecated functions anymore - especially in cases where we're not providing a simple replacement (as is the case here). So, I think that it's better to keep the examples in the documentation if we can.

In this particular case the problem was that the examples were using private symbols, so the examples were ~~never actually usable~~ unable only by accident. I think this coincides with enabling deprecation errors because previously it was possible to use private symbols in other modules under some circumstances.

To see for yourself, copy one of the unittests undocumented here to a new D file and try to compile them. E.g.:

import std.uni;

void main()
{
	auto m = utfMatcher!char(unicode.Number);
	string square = "2²";
	// about sub-matchers
	assert(!m.subMatcher!(2,3,4).test(square)); // ASCII no covered
	assert(m.subMatcher!1.match(square)); // ASCII-only, works
	assert(!m.subMatcher!1.test(square)); // unicode '²'
	assert(m.subMatcher!(2,3,4).match(square));  //
	assert(square == "");
	wstring wsquare = "2²";
	auto m16 = utfMatcher!wchar(unicode.Number);
	// may keep ref, but the orignal (m16) must be kept alive
	auto bmp = m16.subMatcher!1;
	assert(bmp.match(wsquare)); // Okay, in basic multilingual plan
	assert(bmp.match(wsquare)); // And '²' too
}

This gives deprecation warnings because subMatcher is a private symbol, so there is something wrong either with the unittest or the declaration of subMatcher.

fgsfds

jmdavis · 2017-07-05T18:51:28Z

Ah, okay. That makes sense then.

jmdavis · 2017-07-05T18:52:10Z

fgsfds

Did github not let you provide an empty comment when dismissing your review?

CyberShadow · 2017-07-05T18:52:36Z

Exactomundo.

CyberShadow · 2017-07-05T20:24:58Z

Some remaining ones (from the autotester):

std\file.d(2647): Deprecation: function std.utf.toUTF8 is deprecated - To be removed November 2017. Please use std.utf.encode instead.
std\file.d(3433): Deprecation: Comparison between different enumeration types `Flag` and `Flag`; If this behavior is intended consider using `std.conv.asOriginalType`
std\file.d(4289): Deprecation: function std.utf.toUTF8 is deprecated - To be removed November 2017. Please use std.utf.encode instead.
std\datetime\timezone.d(2994): Deprecation: function std.utf.toUTF16 is deprecated - To be removed November 2017. Please use std.utf.encode instead.
std\windows\charset.d(117): Deprecation: function std.utf.toUTF8 is deprecated - To be removed November 2017. Please use std.utf.encode instead.

wilzbach · 2017-07-05T21:12:25Z

std/windows/charset.d

 private import std.conv;
 private import std.string;
-private import std.utf;
 private import std.windows.syserror;


Btw what's the reason for private imports? Aren't they private by default?

I think very very old versions of D (well into 0.x) had imports public by default, like C header files.

Wow -> #5553

wilzbach · 2017-07-05T23:02:01Z

Yeah it's finally passing on Windows:

(I rebased my three attempts to get Windows working into one commit)

CyberShadow · 2017-07-05T23:07:58Z

LGTM. @DmitryOlshansky Please have a look at the std.uni unit tests when you find the time.

ghost · 2017-07-06T07:24:20Z

Pardon me but why did you ( @wilzbach @CyberShadow @jmdavis ) make and/or accept this completely stupid thing ? Now when the goal of a DMD pull request is to deprecate something everything goes red and the auto tester cant be used anymore to fix the PR (e.g platform specific failure missed locally) because the tests don't run anymore.

see the tests here dlang/dmd#6925

👏

wilzbach · 2017-07-06T11:38:43Z

Now when the goal of a DMD pull request is to deprecate something everything goes red and the auto tester cant be used anymore to fix the PR (e.g platform specific failure missed locally) because the tests don't run anymore.

That's exactly why we made this PRs in case you haven't noticed as there have been continuous incidents (aka regressions) of deprecations within Phobos showing up in the user build due to Posix being the most popular developer platform.

See also: #5083 (comment)

ghost · 2017-07-06T11:49:49Z

But it can't work. Phobos and DMD are two different repositories !

mathias-lang-sociomantic · 2017-07-06T13:12:29Z

I wholeheartedly agree with @bbasile here, but for different reason. If you have code which have a certain behavior, and you want to deprecate one symbol, but keep the same behavior, there are places where you have no choices but to use a deprecated symbol.

One example which happened recently is here. I ended up using a hack with extern(C), but really we should have a way to allow library users to use deprecated feature when they really need to.

CyberShadow · 2017-07-06T13:46:24Z

I wholeheartedly agree with @bbasile here, but for different reason. If you have code which have a certain behavior, and you want to deprecate one symbol, but keep the same behavior, there are places where you have no choices but to use a deprecated symbol.

I suggest to wait until we run into a situation that warrants reconsidering this before reverting it.

One example which happened recently is here. I ended up using a hack with extern(C), but really we should have a way to allow library users to use deprecated feature when they really need to.

Why not:

Move the implementation of the public function that is to be deprecated to a private / package xxxImpl function
Add a public deprecated wrapper or alias which forwards to the xxxImpl function
Change all internal references of xxx to xxxImpl ?

mathias-lang-sociomantic · 2017-07-06T16:43:34Z

Why not: [...]

It will work, but you are basically duplicating all your public symbols, as you need to do it for everything you are deprecating. It feels very wrong that you have to do such a large patchset for a simple deprecation. But yes, it's possible if you are willing to go through that pain.

CyberShadow · 2017-07-06T16:49:46Z

I don't know; it seems more wrong to me to be hypocritical about deprecations.

If you absolutely need to use public symbols from inside your implementations so much that deprecating such public symbols becomes an issue, it may indicate that there is a problem with the separation of interface and implementation in your code base. Futhermore, projects that wish to use the -de switch will not be able to directly use (e.g. via rdmd) libraries which rely on this behavior.

Anyway, the requirements and internal structure of Ocean and Phobos are undoubtedly very different, so I suggest to wait and see if/when we run into an actual problem.

PetarKirov · 2017-07-06T17:52:03Z

@mathias-lang-sociomantic I don't understand your reaction. What's the problem with making deprecations errors inside Phobos? How does this effect anyone else? Of course, if we reach a place where this is a problem, this could easily be disabled, but for the time being, I believe that the more rigorous testing is well worth it.

@bbasile what's the problem with fixing the errors caused by deprecations first and then merging PRs that actually introduce those deprecations? Either way, the hardest thing is getting the proposal to deprecate something accepted rather than fixing the stuff themselves, IMO (though your specific case is a bit more involved because people were casting string literals to static arrays).

CyberShadow · 2017-07-08T17:24:33Z

Case in point:
https://issues.dlang.org/show_bug.cgi?id=17621

Trigger a hard error on deprecation messages

16431f1

wilzbach force-pushed the deprecation-halt branch from 5df0313 to 6369d3b Compare July 5, 2017 16:20

wilzbach mentioned this pull request Jul 5, 2017

Deprecate Obsolete Encoding Functions in std.utf #5083

Merged

CyberShadow reviewed Jul 5, 2017

View reviewed changes

CyberShadow previously requested changes Jul 5, 2017

View reviewed changes

std/string.d

/*

@safe pure @nogc unittest

Copy link

Member

CyberShadow Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, that does seem to work!

wilzbach force-pushed the deprecation-halt branch from 6369d3b to 16b9188 Compare July 5, 2017 16:28

Fix deprecations

16b9188

wilzbach mentioned this pull request Jul 5, 2017

Fix Issue 8472 - Replace walkLength() with an improved count() #5545

Closed

wilzbach added 2 commits July 5, 2017 19:02

Update tools to get a deprecation-free DScanner

4b31727

Undocument failing unittests

5b456a1

wilzbach commented Jul 5, 2017

View reviewed changes

wilzbach mentioned this pull request Jul 5, 2017

Remove old, redundant private import access specifier #5553

Merged

wilzbach force-pushed the deprecation-halt branch from 4408b6d to d895932 Compare July 5, 2017 23:01

Fix deprecations on Windows

d895932

CyberShadow approved these changes Jul 5, 2017

View reviewed changes

CyberShadow added the Merge:auto-merge label Jul 5, 2017

dlang-bot merged commit e0fc939 into dlang:master Jul 5, 2017

ghost mentioned this pull request Jul 6, 2017

deprecate hex string literals dlang/dmd#6925

Closed

wilzbach deleted the deprecation-halt branch July 6, 2017 11:35

This was referenced Dec 2, 2017

Make deprecation warnings compile errors dlang/dub#1296

Closed

Make deprecation warnings compile errors dlang/druntime#1982

Merged

Make deprecation warnings compile errors dlang/dmd#7389

Merged

wilzbach mentioned this pull request Dec 16, 2017

Fix intentional discarding of return value in topN #5925

Merged

JinShil mentioned this pull request Apr 4, 2019

Add -de to posix.mak dlang/druntime#2542

Closed

Uh oh!

Trigger a hard error on deprecation messages #5546

Trigger a hard error on deprecation messages #5546

Uh oh!

Conversation

wilzbach commented Jul 5, 2017

Uh oh!

dlang-bot commented Jul 5, 2017

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 5, 2017

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Jul 5, 2017

Uh oh!

CyberShadow commented Jul 5, 2017

Uh oh!

wilzbach commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyberShadow commented Jul 5, 2017

Uh oh!

wilzbach commented Jul 5, 2017

Uh oh!

jmdavis commented Jul 5, 2017

Uh oh!

CyberShadow commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmdavis commented Jul 5, 2017

Uh oh!

jmdavis commented Jul 5, 2017

Uh oh!

CyberShadow commented Jul 5, 2017

Uh oh!

CyberShadow commented Jul 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 5, 2017

Uh oh!

CyberShadow commented Jul 5, 2017

Uh oh!

ghost commented Jul 6, 2017

Uh oh!

wilzbach commented Jul 6, 2017

Uh oh!

ghost commented Jul 6, 2017

Uh oh!

mathias-lang-sociomantic commented Jul 6, 2017

Uh oh!

CyberShadow commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathias-lang-sociomantic commented Jul 6, 2017

Uh oh!

CyberShadow commented Jul 6, 2017

Uh oh!

PetarKirov commented Jul 6, 2017

Uh oh!

wilzbach commented Jul 5, 2017 •

edited

Loading

CyberShadow commented Jul 5, 2017 •

edited

Loading

CyberShadow commented Jul 6, 2017 •

edited

Loading