Use more selective imports at module imports #5584

wilzbach · 2017-07-09T11:36:06Z

AFAICT selective imports have been fixed in 2.071.0 and now enough time has passed down the road.

FWIW I still hope that we get Dependency-Carrying Declarations (DIP1005) at some point and can completely get rid of these global imports and make Phobos entirely pay for what you need.

dlang-bot · 2017-07-09T11:36:07Z

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.

wilzbach · 2017-07-09T11:54:57Z

😭

ggplotd 1.1.3: building configuration "ggplotd-test-default"...
source/ggplotd/colour.d(30,7): Deprecation: catch statement without an exception specification is deprecated; use catch(Throwable) for old behavior
source/ggplotd/stat.d(511,12): Error: module std.typecons import 'Erase' not found
source/ggplotd/geom.d(1044,25): Error: template instance ggplotd.stat.statDensity2D!(MapResult!(__lambda3, Zip!(MapResult!(__lambda1, Result), MapResult!(__lambda2, Result)))) error instantiating
source/ggplotd/example.d(120,9):        instantiated from here: geomDensity2D!(MapResult!(__lambda3, Zip!(MapResult!(__lambda1, Result), MapResult!(__lambda2, Result))))
source/ggplotd/stat.d(511,12): Error: module std.typecons import 'Erase' not found
source/ggplotd/geom.d(1044,25): Error: template instance ggplotd.stat.statDensity2D!(MapResult!(__lambda1, double[][])) error instantiating
source/ggplotd/example.d(234,21):        instantiated from here: geomDensity2D!(MapResult!(__lambda1, double[][]))
source/ggplotd/stat.d(511,12): Error: module std.typecons import 'Erase' not found
source/ggplotd/stat.d(603,28): Error: template instance ggplotd.stat.statDensity2D!(Aes!(double[], "x", double[], "y")) error instantiating
dmd failed with exit code 1.

MetaLang

LGTM. Are these changes the cause of the the ggplotd errors, or did the latest release just break it?

wilzbach · 2017-07-09T21:06:19Z

Are these changes the cause of the the ggplotd errors, or did the latest release just break it?

I haven't seen this particular ggplotd error on the project tester before, so it's most likely from this PR (I rebased to force a rerun to be sure though).

MetaLang · 2017-07-09T21:35:31Z

Probably because some functions were relying on a combination of nested imports and module-level blanket ones, which changing to selective broke.

wilzbach · 2017-07-10T01:26:52Z

Probably because some functions were relying on a combination of nested imports and module-level blanket ones, which changing to selective broke.

And I foolishly assumed that this was fixed / deprecated, but it seems like it's not:

module foo;
import c;
//import c : NotErase; // <- breaks build of module `bar`

module bar;
unittest
{
    import foo : Erase;
    assert(Erase == 2);
}

module c;
int Erase = 2;
int NotErase = 2;

JackStouffer · 2017-07-10T02:37:34Z

Wait, what? So the import c; is somehow acting as a public import?

Did the fix for 314 make the problem its opposite?

wilzbach · 2017-07-10T03:30:36Z

Wait, what? So the import c; is somehow acting as a public import?

AFAICT yep :/

Did the fix for 314 make the problem its opposite?

I think https://github.com/dlang/dmd/pull/5485/files simply didn't touch this. Even 2.062 treats import c as a public import.

wilzbach · 2017-07-10T03:37:26Z

Wait, what? So the import c; is somehow acting as a public import?
AFAICT yep :/

-> https://issues.dlang.org/show_bug.cgi?id=17630

joakim-noah · 2017-07-12T19:47:26Z

Can you investigate this a bit more and add that info to the bug report, including that it's been around for a while? This seems like a fairly important import leak remaining, would be good to explore how large it is. Ironically, using selective imports at module scope, as you do in this PR, actually plugs the leak, but breaks code that unintentionally relies on leaking symbols.

wilzbach · 2017-07-12T22:01:26Z

Can you investigate this a bit more and add that info to the bug report, including that it's been around for a while?

Yeah I tried to. My DMD foo is pretty low atm unfortunately. So I can only say that the leaked symbols are found in the local symtab table.

This seems like a fairly important import leak remaining, would be good to explore how large it is.

It's pretty large. AFAICT all module-level imports, non-selective imports leak their symbols.
However, even selective imports leak their selected symbols:

import imports.test17630b; // works __falsely__ as public import
import imports.test17630b : Erase; // works __falsely__ as public import

Funnily even private import X; doesn't work.

joakim-noah · 2017-07-13T00:45:41Z

It seems like the entire implementation of selective imports never took visibility into account properly. This is where having so few compiler devs' eyes on the frontend source of such a massive language is hurting D.

codecov · 2017-09-03T04:30:26Z

Codecov Report

Merging #5584 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5584   +/-   ##
=======================================
  Coverage   89.56%   89.56%           
=======================================
  Files         119      119           
  Lines       78145    78145           
=======================================
  Hits        69991    69991           
  Misses       8154     8154

Impacted Files	Coverage Δ
std/typecons.d	`88.92% <ø> (ø)`	⬆️
std/algorithm/comparison.d	`95.12% <ø> (ø)`	⬆️
std/algorithm/sorting.d	`98.18% <ø> (ø)`	⬆️
std/regex/package.d	`91.2% <ø> (ø)`	⬆️
std/uni.d	`91.71% <ø> (ø)`	⬆️
std/net/isemail.d	`83.86% <ø> (ø)`	⬆️
std/math.d	`93.29% <ø> (ø)`	⬆️
std/string.d	`99.71% <ø> (ø)`	⬆️
std/stdio.d	`79.41% <ø> (ø)`	⬆️
std/algorithm/searching.d	`96.79% <ø> (ø)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9ff980...c701e27. Read the comment docs.

MartinNowak · 2017-10-01T12:46:32Z

I plan to remove the current visibility deprecation for 2.077, would be a good timing for this change as well. Still need to check how many projects still haven't fixed their deprecations though.

sed -E 's!import (.*); // :!import \1 :!' -i **/*.d

joakim-noah · 2018-02-01T18:42:24Z

Likely just needs to be rebased and updated, would be good if you added Andrei's commented-out selective imports from #5948 too.

joakim-noah

Oh, I guess you're waiting on the 1-year deprecation period for the selective import leak to pass before getting this in? Didn't think of that, guess this will have to wait.

joakim-noah · 2018-02-01T18:46:40Z

std/algorithm/comparison.d

+import std.functional : unaryFun, binaryFun;
 import std.range.primitives;
 import std.traits;
 // FIXME


Can be removed?

joakim-noah · 2018-02-01T18:47:17Z

std/algorithm/iteration.d

 */
 module std.algorithm.iteration;

 // FIXME


joakim-noah · 2018-02-01T18:47:44Z

std/algorithm/mutation.d


 import std.range.primitives;
 import std.traits : isArray, isBlitAssignable, isNarrowString, Unqual, isSomeChar;
 // FIXME


joakim-noah · 2018-02-01T18:48:24Z

std/algorithm/searching.d

 */
 module std.algorithm.searching;

 // FIXME


I'll stop here, just remove all unneeded comments.

quickfur

FWIW, this is still a step forward, even if it's not perfect. I say we should just move ahead with this, and let future improvements come in their own time.

wilzbach · 2018-02-02T17:41:50Z

Oh, I guess you're waiting on the 1-year deprecation period for the selective import leak to pass before getting this in?

Yes and it hasn't even started yet, see e.g. dlang/dmd#6676

I will close this PR now to avoid other people bumping into it, but once the selective import leak has been deprecated, imports at Phobos can finally made selective.
Also as a note to reviewers: new top-level imports can and should always be selective - there's no need to increase the area of the symbol leakage (and it's also dangerous because we could introduce a new symbol leak during the deprecation period).

wilzbach force-pushed the selective-imports branch from 0d54aa0 to cad303d Compare July 9, 2017 11:41

MetaLang approved these changes Jul 9, 2017

View reviewed changes

wilzbach force-pushed the selective-imports branch 2 times, most recently from 9fb9e63 to c701e27 Compare July 9, 2017 21:04

wilzbach mentioned this pull request Jul 10, 2017

Fix deprecations BlackEdder/ggplotd#40

Merged

wilzbach added the Merge:Blocked label Jul 10, 2017

wilzbach mentioned this pull request Aug 23, 2017

Add public API for regex charset parser #5337

Merged

wilzbach mentioned this pull request Sep 7, 2017

Fix issue 13348 - byGrapheme is not pure #5723

Merged

wilzbach mentioned this pull request Dec 20, 2017

Dramatically faster to import std.datetime (and anything under it) #5948

Merged

dlang-bot added Review:Needs Work Merge:Needs Rebase Merge:stalled labels Dec 29, 2017

wilzbach mentioned this pull request Jan 10, 2018

Fix Issue 18219 - Private import inside struct leaks symbols when used as VarDeclaration types dlang/dmd#7668

Merged

wilzbach mentioned this pull request Jan 25, 2018

Partially Fix Issue 10828 - Add output range version of toString functions to std.datetime.Date #6060

Merged

wilzbach force-pushed the selective-imports branch from c701e27 to 2ed3410 Compare January 25, 2018 12:32

wilzbach requested review from andralex and burner as code owners January 25, 2018 12:32

wilzbach requested review from CyberShadow, DmitryOlshansky, JackStouffer, PetarKirov, dnadlinger, ibuclaw and schveiguy as code owners January 25, 2018 12:32

dlang-bot removed Merge:Needs Rebase Review:Needs Work Merge:stalled labels Jan 25, 2018

Use selective imports for module imports

97ab0c9

sed -E 's!import (.*); // :!import \1 :!' -i **/*.d

wilzbach force-pushed the selective-imports branch from 2ed3410 to 97ab0c9 Compare January 25, 2018 12:36

andralex approved these changes Jan 26, 2018

View reviewed changes

joakim-noah suggested changes Feb 1, 2018

View reviewed changes

quickfur approved these changes Feb 2, 2018

View reviewed changes

wilzbach closed this Feb 2, 2018

Uh oh!

Use more selective imports at module imports #5584

Use more selective imports at module imports #5584

Uh oh!

Conversation

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

Uh oh!

Uh oh!

dlang-bot commented Jul 9, 2017

Bugzilla references

Uh oh!

wilzbach commented Jul 9, 2017

Uh oh!

MetaLang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 9, 2017

Uh oh!

MetaLang commented Jul 9, 2017

Uh oh!

wilzbach commented Jul 10, 2017

Uh oh!

JackStouffer commented Jul 10, 2017

Uh oh!

wilzbach commented Jul 10, 2017

Uh oh!

wilzbach commented Jul 10, 2017

Uh oh!

joakim-noah commented Jul 12, 2017

Uh oh!

wilzbach commented Jul 12, 2017

Uh oh!

joakim-noah commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 3, 2017

Codecov Report

Uh oh!

MartinNowak commented Oct 1, 2017

Uh oh!

joakim-noah commented Feb 1, 2018

Uh oh!

joakim-noah left a comment

Choose a reason for hiding this comment

Uh oh!

joakim-noah Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

joakim-noah Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

joakim-noah Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

joakim-noah Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

quickfur left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Feb 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

wilzbach commented Jul 9, 2017 •

edited by MartinNowak

Loading

MetaLang left a comment •

edited

Loading

joakim-noah commented Jul 13, 2017 •

edited

Loading