Turn deprecation into error for issue 313 by RazvanN7 · Pull Request #8443 · dlang/dmd

RazvanN7 · 2018-07-03T08:39:05Z

No description provided.

dlang-bot · 2018-07-03T08:39:07Z

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✗	313	major	[module] Fully qualified names bypass private imports

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8443"

wilzbach · 2018-07-03T08:46:05Z

Looks like more tests need updating:

Test runnable/testdstress.d failed. The logged output: /home/braddr/sandbox/at-client/pull-3231085-Linux_64_32/dmd/generated/linux/release/64/dmd -conf= -m32 -Irunnable -odgenerated/runnable -ofgenerated/runnable/testdstress_0 runnable/testdstress.d runnable/testdstress.d(674): Deprecation: Built-in hex string literals are deprecated, use `std.conv.hexString` instead. runnable/testdstress.d(851): Deprecation: class allocators have been deprecated, consider moving the allocation strategy outside of the class runnable/testdstress.d(173): Error: package `dstress.run` is not accessible here runnable/testdstress.d(174): Error: package `dstress.run` is not accessible here runnable/testdstress.d(175): Error: package `dstress.run` is not accessible here runnable/testdstress.d(659): Deprecation: The `delete` keyword has been deprecated. Use `object.destroy()` (and `core.memory.GC.free()` if applicable) instead. ============================== Test runnable/testdstress.d failed:

JinShil · 2018-07-03T09:49:27Z

test/compilable/testDIP37.d

    static import pkgDIP37.datetime;
    static assert(!__traits(compiles, def()));
    pkgDIP37.datetime.def();
-    pkgDIP37.datetime.common.def();


Rather than deleting this, should we instead add static import pkgDIP32.datetime.common, perhaps in a different scope?

I don't see the point in that: the code will error if that different scope does not include test7's scope.

The alternative would be static assert(!__traits(compiles, ...)

Geod24 · 2018-07-03T09:53:10Z

test/fail_compilation/fail313.d

@@ -2,11 +2,11 @@
 REQUIRED_ARGS: -de


Remove -de

Geod24 · 2018-07-03T09:55:35Z

Do you plan to do the rest of the protection code as well ? Because the time as come, and we might as well just remove the protection code instead of issuing a specific error... The former is more user friendly, though.

RazvanN7 · 2018-07-03T10:00:59Z

@Geod24 Do you refer to : #7241 ? I was thinking of adopting that PR

Geod24 · 2018-07-03T10:04:42Z

Forgot there was a PR for it already... Yeah that was what I was thinking about.

RazvanN7 · 2018-07-03T17:16:00Z

Looks like FreeBSD is spuriously failing again.

wilzbach · 2018-07-03T17:28:06Z

changelog/fqn_imports_bypass_private_imports_error.dd

+The deprecation phase for fully qualified names that bypassed private imports is finished
+
+---
+=========================


FWIW run.dlang.io does support HAR

wilzbach · 2018-07-03T17:28:30Z

test/compilable/testDIP37.d

    static import pkgDIP37.datetime;
    static assert(!__traits(compiles, def()));
    pkgDIP37.datetime.def();
-    pkgDIP37.datetime.common.def();


The alternative would be static assert(!__traits(compiles, ...)

wilzbach · 2018-07-03T17:29:05Z

test/runnable/testdstress.d

 // PERMUTE_ARGS:

-module dstress.run.module_01;
+module run.module_01;


Why was this change necessary?

I'm not entirely sure why, but for dstress.run.module_01 you would get that dstress.run is not accessible from the current module; that is probably because there is no directory structure that matches the module declaration.

But the folder run doesn't exist either, does it?

actually its runnable

That module declaration looks like a leftover from when code was copied into the test/runnable directory. Can't we just get rid of it?

wilzbach · 2018-07-05T10:16:16Z

changelog/fqn_imports_bypass_private_imports_error.dd

+{
+    std.stdio.writefln("foo");         // deprecation before patch, now errors
+}
+==========================


Maybe use two separate file sections in the changelog entry?

wilzbach · 2018-07-05T12:08:15Z

@RazvanN7 looks like this needs more work. I can reproduce failure in ae locally.

ae 0.0.2230: building configuration "ae-test-library"...
utils/array.d(434,18): Error: template ae.utils.array.indexOf cannot deduce function from argument types !()(string, string), candidates are:
utils/array.d(168,12):        ae.utils.array.indexOf(T, D)(in T[] arr, in D val) if (!isSomeChar!T && is(typeof(arr.countUntil(val))) && is(typeof(arr[0] == val)))
utils/array.d(176,12):        ae.utils.array.indexOf(T)(in T[] arr, in T[] val) if (!isSomeChar!T && is(typeof(arr.countUntil(val))))
utils/array.d(465,20): Error: template instance `ae.utils.array.skipUntil!(immutable(char), string)` error instantiating
utils/array.d(474,19):        instantiated from here: eatUntil!(immutable(char), string)
utils/array.d(434,18): Error: template ae.utils.array.indexOf cannot deduce function from argument types !()(string, char), candidates are:
utils/array.d(168,12):        ae.utils.array.indexOf(T, D)(in T[] arr, in D val) if (!isSomeChar!T && is(typeof(arr.countUntil(val))) && is(typeof(arr[0] == val)))
utils/array.d(176,12):        ae.utils.array.indexOf(T)(in T[] arr, in T[] val) if (!isSomeChar!T && is(typeof(arr.countUntil(val))))
utils/array.d(465,20): Error: template instance `ae.utils.array.skipUntil!(immutable(char), char)` error instantiating
utils/array.d(476,19):        instantiated from here: eatUntil!(immutable(char), char)
utils/array.d(479,38): Error: template instance `ae.utils.array.eatUntil!cast(OnEof)0.eatUntil!(immutable(char), string)` error instantiating
utils/array.d(480,43): Error: template instance `ae.utils.array.eatUntil!cast(OnEof)1.eatUntil!(immutable(char), string)` error instantiating
utils/text/package.d(71,18): Error: template instance `ae.utils.text.eatLine!(immutable(char))` error instantiating
/home/seb/dlang/dmd/generated/linux/release/64/dmd failed with exit code 1.

wilzbach · 2018-07-05T12:20:24Z

Looked a bit closer and the (string, string) part is the reason for the failure.
The two overloads have !isSomeChar!T, s.t. they don't conflict with std.string. However, the import of std.string is missing and the errors are about calls to the string variant.
This would solve the problem:

public import std.string;

But as there was never a deprecation triggered, I'm not sure whether we can go ahead with this directly.
CC @CyberShadow what do you think? It's your library that is blocking this PR ;-)

Maybe related: CyberShadow/ae#33

CyberShadow · 2018-07-06T13:42:31Z

But as there was never a deprecation triggered, I'm not sure whether we can go ahead with this directly.

Yes, that bug ought to be fixed first.

JinShil · 2018-07-09T01:31:47Z

Yes, that bug ought to be fixed first.

I assume you're referring to a DMD bug. What specifically is the bug? Is it in bugzilla yet? Do you have a minimal test case to trigger the bug?

JinShil · 2018-08-01T03:15:02Z

Is Jenkins dead now?

wilzbach · 2018-08-01T03:30:52Z

Yes.

wilzbach · 2018-08-01T04:18:29Z

Setting this to blocked, s.t. we remember that this can't be merged before the 2.082 has happened (~15.8).

Regarding the project failures:

Ocean (genuine DMD segfault - I wonder why the stacktrace doesn't have line numbers though.)
Vibe.d / diet-ng / dlang-tour / dlang-bot / dub (looks awfully similar to the recent regression - Vibe.d fails to compile with 2.081 vibe-d/vibe.d#2181)

JinShil · 2018-08-20T08:57:54Z

I wonder why the stacktrace doesn't have line numbers

Might be fixed with dlang/druntime#2278

ibuclaw · 2018-09-20T21:20:28Z

Setting this to blocked, s.t. we remember that this can't be merged before the 2.082 has happened (~15.8).

I do believe that we can unblock this now.

PetarKirov · 2018-09-20T22:07:36Z

@RazvanN7 rebase pls

RazvanN7 · 2018-09-21T13:29:18Z

@ZombineDev Done.

wilzbach · 2018-09-22T12:35:59Z

The ae failure at buildkite still looks genuine.

PetarKirov · 2018-09-23T06:52:25Z

@CyberShadow can you take a look at the ae errors?

CyberShadow · 2018-09-23T07:52:54Z

I literally can't. Could you please repaste the log?

If I build this PR with Digger, ae builds and passes tests successfully with the resulting DMD.

Geod24 · 2018-09-23T08:03:22Z


~> bash
--
  | Generating test runner configuration 'ae-test-library' for 'library' (sourceLibrary).
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
  | Performing "unittest" build using dmd for x86_64.
  | ae 0.0.2281: building configuration "ae-test-library"...
  | utils/array.d(434,18): Error: template `ae.utils.array.indexOf` cannot deduce function from argument types `!()(string, string)`, candidates are:
  | utils/array.d(168,12):        `ae.utils.array.indexOf(T, D)(in T[] arr, in D val) if (!isSomeChar!T && is(typeof(arr.countUntil(val))) && is(typeof(arr[0] == val)))`
  | utils/array.d(176,12):        `ae.utils.array.indexOf(T)(in T[] arr, in T[] val) if (!isSomeChar!T && is(typeof(arr.countUntil(val))))`
  | utils/array.d(465,20): Error: template instance `ae.utils.array.skipUntil!(immutable(char), string)` error instantiating
  | utils/array.d(474,19):        instantiated from here: `eatUntil!(immutable(char), string)`
  | utils/array.d(434,18): Error: template `ae.utils.array.indexOf` cannot deduce function from argument types `!()(string, char)`, candidates are:
  | utils/array.d(168,12):        `ae.utils.array.indexOf(T, D)(in T[] arr, in D val) if (!isSomeChar!T && is(typeof(arr.countUntil(val))) && is(typeof(arr[0] == val)))`
  | utils/array.d(176,12):        `ae.utils.array.indexOf(T)(in T[] arr, in T[] val) if (!isSomeChar!T && is(typeof(arr.countUntil(val))))`
  | utils/array.d(465,20): Error: template instance `ae.utils.array.skipUntil!(immutable(char), char)` error instantiating
  | utils/array.d(476,19):        instantiated from here: `eatUntil!(immutable(char), char)`
  | utils/array.d(479,38): Error: template instance `ae.utils.array.eatUntil!cast(OnEof)0.eatUntil!(immutable(char), string)` error instantiating
  | utils/array.d(480,43): Error: template instance `ae.utils.array.eatUntil!cast(OnEof)1.eatUntil!(immutable(char), string)` error instantiating
  | utils/graphics/color.d(498,45): Deprecation: integral promotion not done for `~b`, use '-transition=intpromote' switch or `~cast(int)(b)`
  | utils/graphics/color.d(498,45): Deprecation: integral promotion not done for `~b`, use '-transition=intpromote' switch or `~cast(int)(b)`
  | utils/graphics/color.d(498,45): Deprecation: integral promotion not done for `~b`, use '-transition=intpromote' switch or `~cast(int)(b)`
  | utils/graphics/color.d(498,45): Deprecation: integral promotion not done for `~b`, use '-transition=intpromote' switch or `~cast(int)(b)`
  | utils/graphics/color.d(498,45): Deprecation: integral promotion not done for `~b`, use '-transition=intpromote' switch or `~cast(int)(b)`
  | utils/text/package.d(71,18): Error: template instance `ae.utils.text.eatLine!(immutable(char))` error instantiating
  | dmd failed with exit code 1.
  | 🚨 Error: The command exited with status 2

CyberShadow · 2018-09-23T08:12:26Z

~~Those errors don't make sense, look at the surrounding code.~~

If I build this PR with Digger, ae builds and passes tests successfully with the resulting DMD.

Oops, forgot to add --compiler=dmd.

Allows building with dlang/dmd#8443.

CyberShadow · 2018-09-23T08:48:29Z

Pushed fix & tag.

RazvanN7 · 2018-10-23T08:22:11Z

Ping. I think this is ready to be merged.

RazvanN7 requested a review from MartinNowak as a code owner July 3, 2018 08:39

wilzbach approved these changes Jul 3, 2018

View reviewed changes

RazvanN7 force-pushed the checkAccess_err branch from dbe4ddc to a23a97a Compare July 3, 2018 08:46

wilzbach added the Severity:Deprecation -> Error label Jul 3, 2018

RazvanN7 added the Review:Easy Review label Jul 3, 2018

JinShil reviewed Jul 3, 2018

View reviewed changes

Geod24 reviewed Jul 3, 2018

View reviewed changes

test/fail_compilation/fail313.d Outdated

@@ -2,11 +2,11 @@

REQUIRED_ARGS: -de

Copy link

Member

Geod24 Jul 3, 2018

Choose a reason for hiding this comment

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

Remove -de

RazvanN7 force-pushed the checkAccess_err branch from a23a97a to 6ab2f49 Compare July 3, 2018 10:04

RazvanN7 force-pushed the checkAccess_err branch from 6ab2f49 to 48f39e1 Compare July 3, 2018 10:12

wilzbach added the Merge:auto-merge label Jul 3, 2018

wilzbach reviewed Jul 3, 2018

View reviewed changes

wilzbach removed the Merge:auto-merge label Jul 3, 2018

wilzbach reviewed Jul 5, 2018

View reviewed changes

RazvanN7 force-pushed the checkAccess_err branch 3 times, most recently from 6c57dd0 to 6af0031 Compare July 5, 2018 10:25

wilzbach added Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Merge:auto-merge labels Jul 5, 2018

wilzbach force-pushed the checkAccess_err branch from 6af0031 to e1d2952 Compare August 1, 2018 03:30

wilzbach added the Merge:Blocked label Aug 1, 2018

PetarKirov removed the Merge:Blocked label Sep 20, 2018

Turn deprecation into error for issue 313

aca10e8

RazvanN7 force-pushed the checkAccess_err branch from e1d2952 to aca10e8 Compare September 21, 2018 13:28

jacob-carlborg approved these changes Sep 22, 2018

View reviewed changes

Geod24 closed this Sep 23, 2018

Geod24 reopened this Sep 23, 2018

CyberShadow added a commit to CyberShadow/ae that referenced this pull request Sep 23, 2018

ae.utils.array: Fix DMD 2.082 import visibility warning

d03c405

Allows building with dlang/dmd#8443.

RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Oct 23, 2018

jacob-carlborg approved these changes Oct 23, 2018

View reviewed changes

PetarKirov approved these changes Oct 24, 2018

View reviewed changes

RazvanN7 added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Oct 25, 2018

dlang-bot merged commit 01a6a5e into dlang:master Oct 25, 2018

JinShil mentioned this pull request Dec 10, 2018

Fix Issue 19471 - Duplicate error messages when trying to use an inaccessible package symbol #9056

Closed

Uh oh!

Comments

Conversation

RazvanN7 commented Jul 3, 2018

Uh oh!

dlang-bot commented Jul 3, 2018

Bugzilla references

Testing this PR locally

Uh oh!

wilzbach commented Jul 3, 2018

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!

Geod24 commented Jul 3, 2018

Uh oh!

RazvanN7 commented Jul 3, 2018

Uh oh!

Geod24 commented Jul 3, 2018

Uh oh!

RazvanN7 commented Jul 3, 2018

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!

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!

wilzbach commented Jul 5, 2018

Uh oh!

wilzbach commented Jul 5, 2018

Uh oh!

CyberShadow commented Jul 6, 2018

Uh oh!

JinShil commented Jul 9, 2018

Uh oh!

JinShil commented Aug 1, 2018

Uh oh!

wilzbach commented Aug 1, 2018

Uh oh!

wilzbach commented Aug 1, 2018

Uh oh!

JinShil commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented Sep 20, 2018

Uh oh!

PetarKirov commented Sep 20, 2018

Uh oh!

RazvanN7 commented Sep 21, 2018

Uh oh!

wilzbach commented Sep 22, 2018

Uh oh!

PetarKirov commented Sep 23, 2018

Uh oh!

CyberShadow commented Sep 23, 2018

Uh oh!

Geod24 commented Sep 23, 2018

Uh oh!

CyberShadow commented Sep 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyberShadow commented Sep 23, 2018

Uh oh!

RazvanN7 commented Oct 23, 2018

JinShil commented Aug 20, 2018 •

edited

Loading

CyberShadow commented Sep 23, 2018 •

edited

Loading